Skip to content

Add config tooling to check for orphan keys - #705

Merged
nichollsh merged 7 commits into
mainfrom
hn/configparse
Jun 15, 2026
Merged

nichollsh merged 7 commits into
mainfrom
hn/configparse

Conversation

@nichollsh

@nichollsh nichollsh commented Jun 14, 2026

Copy link
Copy Markdown
Member

Description

Currently, it is possible to include "orphan" keys in the config file which don't map to any thing in the Config object of PROTEUS. This module provides helpers that recursively compare the raw dict, read from the PROTEUS toml-formatted config files, against the Config schema.

E.g. if you were to pass atmos_clim.agni.spectral_group="Honeyside" it will do nothing to warn you, at the moment, but you could then write a paper saying you used these opacities.

These are important to identify because a user could set these with intent, but they
will be ignored if not mapped by the parser. I have been caught out by this before, so am making this PR to prevent it happening in the future.

Closes #666

Validation of changes

  • add tooling in a new validate.py file which checks for orphaned keys
  • add tests to check for this behaviour

Tested on Fedora 44, Python 3.13

Checklist

  • I have followed the contributing guidelines
  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • My changes generate no new warnings or errors
  • I have checked that the tests still pass on my computer
  • I have updated the docs, as appropriate
  • I have added tests for these changes, as appropriate
  • I have checked that all dependencies have been updated, as required

@codecov

codecov Bot commented Jun 14, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.19608% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.45%. Comparing base (bae50e2) to head (5877290).

Files with missing lines Patch % Lines
src/proteus/config/orphans.py 88.63% 4 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #705      +/-   ##
==========================================
+ Coverage   91.36%   91.45%   +0.09%     
==========================================
  Files         109      110       +1     
  Lines       14723    14772      +49     
  Branches     2630     2642      +12     
==========================================
+ Hits        13451    13510      +59     
+ Misses       1272     1260      -12     
- Partials        0        2       +2     
Flag Coverage Δ
unit-tests 79.95% <90.19%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@nichollsh
nichollsh marked this pull request as ready for review June 14, 2026 11:06
@nichollsh
nichollsh requested a review from a team as a code owner June 14, 2026 11:06

@timlichtenberg timlichtenberg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding this. Orphan keys have caught me out before, so I'm glad to have a guard for them.

I ran the validator against every bundled TOML using the installed Config schema. Good news first: zero false positives across the full option surface. input/all_options.toml (which contains every valid key) and all of the standard, tutorial, and intercomp configs parse clean, which is strong evidence that the schema mapping is complete. The grid and inference files use a different top-level schema and do get flagged by the raw function, but they never reach read_config_object (grid reads the resolved ref_config, inference loads the .infer.toml separately), so no real workflow regresses.

Two things to fix before merge:

  1. auto/status is committed by accident. Its contents are byte-for-byte what check_for_unknown_keys writes on the orphan path (status 20), so it was produced by a local validator run and slipped in. Please git rm auto/status.

  2. The status-file write resolves to the wrong place, and it is what created that stray file. read_config_object passes outdir=obj.params.out.path, which is the bare run name (default "auto", still an unresolved sentinel at parse time). UpdateStatusfile runs os.path.abspath on it, so the file lands at <cwd>/auto/status rather than the canonical output/<name>/status that the rest of the code builds in SetDirectories. I reproduced <cwd>/auto/status with identical contents in a sandbox. Beyond the wrong path, read_config_object is called from three sites and is otherwise a pure read/validate, so writing files and creating directories at parse time is a surprising side effect. Simplest fix: drop the status write from check_for_unknown_keys and let the caller report status once the directories are set up.

Smaller points are inline. Once the artifact and the status write are sorted out, this is good to go.

Comment thread auto/status Outdated
Comment thread src/proteus/config/__init__.py Outdated
Comment thread src/proteus/config/validate.py Outdated
Comment thread src/proteus/config/orphans.py
Comment thread tests/config/test_validate.py Outdated
@nichollsh
nichollsh requested a review from timlichtenberg June 14, 2026 13:52
@nichollsh

Copy link
Copy Markdown
Member Author

Thanks for the constructive review, @timlichtenberg. I have adjusted the functionality in a few ways:

  • consolidated phrasing to refer to 'orphans' rather than 'unknown keys' etc
  • the main check_config_orphan_free() function now just returns true/false rather than raising
  • handling the presence of orphan keys is now done in a more Pythonic fashion, separately to the check, in proteus.py
  • updates the status file appropriately
  • does not write mistaken auto file to output dir
  • caught some issues that would arise on nightly integration runs (due to orphan keys in config files)
  • fixed the failing tests

timlichtenberg
timlichtenberg previously approved these changes Jun 14, 2026

@timlichtenberg timlichtenberg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the rework, this addresses everything from my earlier pass and the result is cleaner than the first version.

The key change is the right one: check_config_orphan_free is now a pure predicate (no I/O, no directory creation), and the status-20 write plus the raise moved into Proteus.__init__, where the resolved self.directories['output'] is available. That is why the status file now lands in output/<name>/ instead of the working directory, and it removes the surprising side effect from config parsing. I re-checked the path resolution and ordering (read_config_object drops orphans silently, init_directories resolves the output dir, then the gate fires and writes status 20 before raising) and it is correct.

Also confirmed:

  • auto/status is gone.
  • Zero false positives across the full option surface (all_options.toml and every standard/tutorial/intercomp config parse clean).
  • The three integration TOMLs are now orphan-free; nice to see the tool flush real dead keys (author, condensation) out of our own fixtures.
  • The new Proteus.__init__ tests exercise the real path and the status-file write, which is what the previous test could not do.

Approving. A couple of optional follow-ups inline, neither blocking.

Comment thread src/proteus/config/orphans.py Outdated
Comment thread src/proteus/proteus.py
@nichollsh
nichollsh requested a review from timlichtenberg June 15, 2026 07:03
@nichollsh
nichollsh merged commit f55b0a9 into main Jun 15, 2026
11 checks passed
@nichollsh
nichollsh deleted the hn/configparse branch June 15, 2026 19:02
@egpbos

egpbos commented Jun 16, 2026

Copy link
Copy Markdown
Member

Very nice!

timlichtenberg added a commit that referenced this pull request Jul 31, 2026
A key the schema does not define was discarded in silence, so a typo left the parameter sitting at its default with nothing reported. The orphan-key walk added in #705 now runs wherever a configuration is loaded rather than only in `Proteus.__init__`, which brings the grid script, the CLI and the rest of the entry points under it.

It also reports the one fault a name comparison cannot see. A section written as `[[name]]` rather than `[name]` leaves a correctly spelled name in place, so nothing notices, while structuring discards the whole section and every parameter inside it falls back to its default.

The entry point is `read_config_object`, composing `read_config`, `find_key_problems` and `structure_config`, and it always refuses a configuration it cannot account for.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ensure that all user-provided config options are meaningful and actioned

3 participants