Apply the orphan-key check wherever a configuration is loaded - #799
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #799 +/- ##
==========================================
+ Coverage 92.70% 92.75% +0.05%
==========================================
Files 112 112
Lines 16261 16313 +52
Branches 2891 2897 +6
==========================================
+ Hits 15074 15131 +57
+ Misses 1163 1158 -5
Partials 24 24
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
egpbos
left a comment
There was a problem hiding this comment.
Very nice addition! Good test coverage.
I seem to remember that we did discuss this somewhere, @nichollsh I think flagged this issue at some point too, but maybe the discussion was offline.
A few suggestions below to possibly make this slightly more compact.
Agree, we discussed this multiple times already. I couldn't find an issue to link this PR to though. |
Every configuration parameter has a default, and the parser discards any TOML key it does not recognise, so a misspelled or outdated option name takes no effect at all: the run proceeds on the default while the file appears to say otherwise. A detector for these keys existed but was reachable only from the runner, which left every other way of loading a configuration without it. The parameter grid is where that hurts most. It loads the base configuration, copies it per grid point, and writes each case file back out from the parsed object, so an unrecognised key in the base file is erased before any case file is written. The per-case check then sees a clean file, and every case in the ensemble runs on a default nobody chose. Loading a configuration now refuses one that carries keys outside the schema, and the refusal names every offending key. The runner keeps its own check until after the output directory is resolved so that the refusal is still recorded in the run's status file, and it reports through the same message, which previously named no keys at all: a run stopped without saying which key was at fault, because the names only ever went to a log call made before any log handler exists. Where a file has both an unrecognised key and something else wrong, the key is named first, since it is often the cause, and the other complaint is carried along with it rather than replaced. A section written as an array of tables is refused too. Spelling `[[planet]]` instead of `[planet]` leaves a name the schema does define, so comparing names alone finds nothing wrong, while structuring drops the section whole and every parameter inside it falls back to its default. That is the same silent substitution as a misspelling, over a wider area, and it is caught at any depth. No field in the schema accepts both a table and a value that TOML can express, so a section that is not a table is always a mistake rather than a permitted alternative. The two faults are reported in separate blocks because the remedies differ: one asks for a spelling check, the other for a bracket to be removed. Resolving the output directory needs a configured environment and can fail on its own, most often because FWL_DATA is unset. A key already found unrecognised is reported alongside that failure rather than dropped, because someone installing for the first time can easily have both and being told only about the environment leaves the typo to surface on the next attempt. A file that cannot be structured at all has nowhere to record anything, since the output directory is named inside it, so there the message is the whole of the report. The refusal has its own type, so the command line can present it in the same style as any other user error while an unrelated failure still surfaces with its traceback. Round-tripping a parsed configuration back out through the writer stays inside the schema, so grid case files and the resolved configuration saved into each run's output folder continue to load.
The schema walk was split across four functions, two of which nothing outside the tests called any more once both faults were collected in a single pass. They are gone, and `find_key_problems` is now the whole of the public surface: it walks the schema once and returns the unknown names and the misdeclared sections together. The class and path arguments carry the recursion and default to the schema root, so a caller passes only the raw dict. Reading a configuration no longer takes a parameter that decides whether the keys are checked. `read_config_object` always refuses a file the schema cannot accept, and the structuring step it used to skip is now `structure_config`, which the runner calls directly. The runner is the only thing that needs a configuration built from a file it is about to reject, because the output directory it records the refusal in is named inside that file. Composing the two steps there rather than passing a flag also means the file is read once instead of twice. The optional module sections, declared as a class or None, reach their class through the union branch of the shape check, and a schema whose annotations cannot be resolved still has its own field names compared rather than being waved through. Both are now covered.
|
Could you clarify how this is different to the config validator functions I added a few weeks ago? These were meant to check if the config included untracked keys. Maybe the main difference here is just expanding it to the grid script? |
b222d18 to
2895269
Compare
|
All three taken, thanks.
I did keep The While in there I covered the union branch of the shape check, which is the path optional sections like |
Good point, and it is yours: the walk from #705 is still what does the work, I only built on it. The difference is where it runs. Yours was called from The grid is not just one more caller though. It writes each case config out from the parsed object, so an unknown key in the base file is gone before any case file exists, and the per-case check then sees clean files. Misspell Two smaller ones: the key names never actually reached the user, since the I have retitled the PR and rewritten the description to make the #705 heritage explicit, since it read as if this were new tooling rather than a continuation of yours. |
I knew I had seen it before! 😄
Ah, of course, my bad! |
|
Thanks for addressing the comments @timlichtenberg, I dismissed my blocking review. One additional general comment that maybe I mentioned somewhere before as well: this whole configuration machinery might be a good candidate for making a separate generic package out of; could be useful beyond PROTEUS. Or perhaps just fwl-config to start with ;) Beyond the scope of this PR, of course. |
nichollsh
left a comment
There was a problem hiding this comment.
Great! Thanks for expanding the orphan-keys functionality in this way. It's easier to maintain and covers the other CLI commands.
I've tested this on my laptop by adding/removing/modifying various keys in the all_options.toml config. This prints the appropriate message and exits. I've also tested it through the grid utility (one of the main aims of this PR) and this works as described. The status file is updated appropriately.
Some phrasing/expositional suggestions below.
Good idea in the abstract, so I went and looked for who would consume it. MORS, CALLIOPE and ZEPHYRUS read no configuration file at all. JANUS and Zalmoxis both That leaves aragog, which does have attrs configuration classes. Even there it is half the problem: So I would keep it here for now. The shared surface is the 220-line walk and nothing else, and configuration loading is the one path where a version-pin mismatch stops everything from starting. fwl-io earns its keep because every module downloads reference data; this does not have the same shape yet. |
The message explaining why a configuration was refused now says what is wrong in one sentence and what to do about it in the next, instead of also explaining why refusing is the right response. It closes with a link to the parameter reference alongside the pointer to input/all_options.toml, so someone meeting the message for the first time has somewhere to go. The misdeclared-section case points at double brackets as the thing to look for rather than spelling out the [[name]] against [name] case.
|
All three taken, thanks, and glad the grid path checked out on your side. One adjustment inside the first suggestion. Two tests pinned the old phrasing, one on the singular and plural agreeing with the key count and one on the bracket advice appearing only when a section is actually misdeclared. Both now key on the new wording rather than the old. The reference link resolves. |
nichollsh
left a comment
There was a problem hiding this comment.
Thanks! Good with me to merge.
Description
Builds on the orphan-key tooling from #705. That added the walk that compares a config against the schema; this puts it where every config load goes through, and adds the one fault it cannot see.
Config parameters all have defaults and the parser drops any key it does not recognise, so a misspelled or outdated option silently does nothing and the run carries on with the default. The check from #705 was called from
Proteus.__init__only, soproteus grid, the download commands and the install and update paths all bypassed it. It now lives inread_config_object.The grid is the worst of it, and not just because it was one more caller. Case files are written out from the parsed base config, so an unrecognised key in the base is erased before any case file exists and the per-case check sees clean files. Misspell
maximumintests/grid/base.tomland every case in the ensemble runs on the 1e7 default instead of the 3e7 asked for, silently.Two other things:
log.errorcalls that run before a handler is attached, so theRuntimeErrorsaid only that unknown keys existed.[[planet]]instead of[planet]is spelled correctly, so comparing names finds nothing wrong, while the whole section is dropped and every parameter inside it reverts to its default. Caught now, at any depth.Validation of changes
macOS, Python 3.12. Unit tier green.
input/andtests/still loads, so nothing anyone already has stops working. Grid and inference definition files use their own schemas and do not go through this loader.Config.write()and byGrid.write_config_files()reload under the new rule, so grids and saved run configs are unaffected.One thing worth a look: the check runs where
read_config_objectruns, which is not everywhere.proteus inferonly checks that its reference config exists, andtools/migrate_config_v2_to_v3.pyvalidates without it. I will file those two separately.Checklist