From d77f5669cb15fc540891fff9b212ebee1999d943 Mon Sep 17 00:00:00 2001 From: timlichtenberg Date: Tue, 28 Jul 2026 07:25:42 +0200 Subject: [PATCH 1/3] Reject configuration keys that the schema does not define 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. --- docs/How-to/config.md | 18 ++ src/proteus/cli.py | 21 +- src/proteus/config/__init__.py | 54 ++++- src/proteus/config/orphans.py | 194 ++++++++++++--- src/proteus/proteus.py | 71 +++++- tests/config/test_config.py | 151 ++++++++++++ tests/config/test_orphans.py | 416 ++++++++++++++++++++++++++++++--- tests/grid/test_manage.py | 55 +++++ tests/test_cli.py | 192 +++++++++++++++ 9 files changed, 1100 insertions(+), 72 deletions(-) diff --git a/docs/How-to/config.md b/docs/How-to/config.md index a5b5bfffa..717f90428 100644 --- a/docs/How-to/config.md +++ b/docs/How-to/config.md @@ -45,6 +45,24 @@ example, the `mors` stellar evolution module (`star.module = 'mors'`) requires loader reports an error at startup if a required parameter is missing for the chosen modules. +**Keys outside the schema are refused.** Loading a configuration fails if the +file contains a key that the schema does not define, and the error names every +such key. Because each parameter has a default, a misspelled or outdated option +name would otherwise take no effect at all: the run would proceed on the +default while the file appears to say otherwise. The check covers nested keys, +so `planet.elements.H_budgets` is caught as readily as a stray top-level +section. It runs when a run configuration is loaded, which includes `proteus +start`, the data-download commands, and the base configuration of a parameter +grid. If a key is rejected that you expect to exist, check its spelling against +the configuration reference pages above or against `all_options.toml`. + +**Sections must be written as single tables.** Writing `[[planet]]` rather than +`[planet]` declares an array of tables, which the parser cannot map onto the +schema: the section is discarded whole and every parameter inside it reverts to +its default. The name itself is spelled correctly in that case, so nothing +about it looks wrong in the file. Loading refuses such a section and names it, +and the fix is to remove the extra brackets. + See [`all_options.toml`](https://github.com/FormingWorlds/PROTEUS/blob/main/input/all_options.toml) for a comprehensive example. Have a look at the other [input configs](https://github.com/FormingWorlds/PROTEUS/tree/main/input) for ideas of how to set up your config in practice. ## Root parameters diff --git a/src/proteus/cli.py b/src/proteus/cli.py index 757efc8fd..71d3dbf80 100644 --- a/src/proteus/cli.py +++ b/src/proteus/cli.py @@ -65,7 +65,7 @@ def _should_apply_deterministic(argv, environ) -> bool: from proteus import Proteus # noqa: E402 from proteus import __version__ as proteus_version # noqa: E402 -from proteus.config import read_config_object # noqa: E402 +from proteus.config import UnknownConfigKeyError, read_config_object # noqa: E402 from proteus.utils.data import download_sufficient_data # noqa: E402 from proteus.utils.helper import get_proteus_dir, resolve_fwl_data_dir # noqa: E402 from proteus.utils.logs import bootstrap_logger, setup_logger # noqa: E402 @@ -89,7 +89,24 @@ def _should_apply_deterministic(argv, environ) -> bool: ) -@click.group() +class ConfigAwareGroup(click.Group): + """Command group that presents a refused configuration as a CLI error. + + Every command that reads a configuration can refuse it over unrecognised + keys. Catching that here rather than in each command keeps the message in + the same style as the rest of the CLI, and reaches subcommands too, since + they are invoked through this group. Only the configuration-key error is + caught, so an unrelated failure still surfaces with its traceback. + """ + + def invoke(self, ctx): + try: + return super().invoke(ctx) + except UnknownConfigKeyError as exc: + raise click.ClickException(str(exc)) from exc + + +@click.group(cls=ConfigAwareGroup) @click.version_option(version=proteus_version) def cli(): # Ensure the 'fwl' logger has a handler as early as possible, before any diff --git a/src/proteus/config/__init__.py b/src/proteus/config/__init__.py index c9cc0e1e5..dae994526 100644 --- a/src/proteus/config/__init__.py +++ b/src/proteus/config/__init__.py @@ -7,7 +7,12 @@ import cattrs from ._config import Config -from .orphans import check_config_orphan_free +from .orphans import ( + UnknownConfigKeyError, + find_key_problems, + find_orphan_keys, + format_orphan_message, +) log = logging.getLogger('fwl.' + __name__) @@ -21,12 +26,45 @@ def read_config(path: Path | str) -> dict: return config -def read_config_object(path: Path | str) -> Config: - """Read and validate config into Config object.""" +def read_config_object(path: Path | str, *, strict: bool = True) -> Config: + """Read and validate config into Config object. + + Parameters + ---------- + path: + Path to the TOML config file. + strict: + Reject keys the schema does not define, and sections it declares as a + table but the file supplies otherwise. cattrs discards both without + complaint, which turns a misspelling into a silent fallback to the + default, so rejection is the default here. Pass False only when the + caller performs the same check itself and can report the failure + better. + + Returns + ------- + Config + The structured configuration. + + Raises + ------ + UnknownConfigKeyError + If the file contains keys the schema cannot accept, when *strict*. + ValueError + If a value fails validation. + """ # Read config from TOML file in path as a raw dict. cfg = read_config(path) + # Reject unrecognised keys before structuring, so that a typo is reported + # as a typo rather than as whatever the resulting default happens to break + # further downstream. + if strict: + orphans, mistyped = find_key_problems(cfg) + if orphans or mistyped: + raise UnknownConfigKeyError(format_orphan_message(orphans, path, mistyped)) + # Attempt to structure config with cattrs. try: # Structure the config @@ -61,4 +99,12 @@ def read_config_object(path: Path | str) -> Config: ) from None -__all__ = ['Config', 'read_config_object', 'read_config', 'check_config_orphan_free'] +__all__ = [ + 'Config', + 'UnknownConfigKeyError', + 'read_config_object', + 'read_config', + 'find_key_problems', + 'find_orphan_keys', + 'format_orphan_message', +] diff --git a/src/proteus/config/orphans.py b/src/proteus/config/orphans.py index 9aa334889..b6cbb8bd6 100644 --- a/src/proteus/config/orphans.py +++ b/src/proteus/config/orphans.py @@ -10,7 +10,9 @@ from __future__ import annotations import logging +import types import typing +from pathlib import Path import attrs @@ -19,6 +21,16 @@ log = logging.getLogger('fwl.' + __name__) +class UnknownConfigKeyError(ValueError): + """Raised when a config file carries keys outside the schema. + + A distinct type so that callers can present this to the user as a + configuration problem without also catching unrelated failures. It derives + from ValueError, which is what every other config rejection raises, so a + caller that does not care about the distinction needs no change. + """ + + def _extract_attrs_class(hint: type) -> type | None: """Return the attrs class from a type hint, unwrapping union types. @@ -45,6 +57,33 @@ def _extract_attrs_class(hint: type) -> type | None: return None +def _expects_single_table(hint: type) -> bool: + """Whether *hint* puts exactly one table at a field, rather than several. + + A field holding one nested class is written as ``[name]`` and nothing else + is valid there. A field holding a container of them would be written as + repeated ``[[name]]`` tables, where a list is the intended shape rather + than a mistake. The schema declares no such field today, so this only + keeps a later one from being refused. + + Parameters + ---------- + hint: + Type hint to inspect. + + Returns + ------- + bool + True when a single table is the only shape the field accepts. + """ + origin = typing.get_origin(hint) + if origin is None: + return isinstance(hint, type) and attrs.has(hint) + if origin in (types.UnionType, typing.Union): + return any(isinstance(arg, type) and attrs.has(arg) for arg in typing.get_args(hint)) + return False + + def _collect_orphan_keys(data: dict, cls: type, path: str = '') -> list[str]: """Recursively collect TOML keys that have no matching field in *cls*. @@ -64,9 +103,32 @@ def _collect_orphan_keys(data: dict, cls: type, path: str = '') -> list[str]: *data* but are not declared fields of *cls* or any nested attrs class. """ + return _collect_key_problems(data, cls, path)[0] + + +def _collect_key_problems(data: dict, cls: type, path: str = '') -> tuple[list[str], list[str]]: + """Recursively collect the keys in *data* that *cls* cannot accept. + + Parameters + ---------- + data: + Raw TOML sub-dict to inspect. + cls: + attrs-decorated class to compare against. + path: + Dotted key prefix for building human-readable paths in error messages. + + Returns + ------- + tuple[list[str], list[str]] + Two lists of dotted key paths, in file order. The first holds names the + schema does not declare. The second holds sections the schema declares + as a table but which the file supplies as something else. + """ + # If the class is not an attrs class, then we cannot inspect it. if not attrs.has(cls): - return [] + return [], [] # Get the field names of the attrs class field_names = {f.name for f in attrs.fields(cls)} @@ -80,6 +142,7 @@ def _collect_orphan_keys(data: dict, cls: type, path: str = '') -> list[str]: hints = {} orphans: list[str] = [] + mistyped: list[str] = [] # Loop through keys in the raw dict. for key, value in data.items(): @@ -89,20 +152,31 @@ def _collect_orphan_keys(data: dict, cls: type, path: str = '') -> list[str]: # Check if the key is in the attrs class. If not, add to orphans. if key not in field_names: orphans.append(full_path) + continue - # If this is a dict, then we need to go deeper. - elif isinstance(value, dict): - nested_cls = _extract_attrs_class(hints.get(key)) + # Nothing further to check unless the schema puts a nested class here. + nested_cls = _extract_attrs_class(hints.get(key)) + if nested_cls is None: + continue - # Recursion on this function - if nested_cls is not None: - orphans.extend(_collect_orphan_keys(value, nested_cls, full_path)) + # If this is a dict, then we need to go deeper. + if isinstance(value, dict): + sub_orphans, sub_mistyped = _collect_key_problems(value, nested_cls, full_path) + orphans.extend(sub_orphans) + mistyped.extend(sub_mistyped) + elif _expects_single_table(hints.get(key)): + # The schema puts one table here and the file supplies something + # else, most often because the section was written as [[name]] + # rather than [name]. The name still matches a field, so nothing + # above notices, while structuring discards the section whole and + # every parameter inside it falls back to its default. + mistyped.append(full_path) - return orphans + return orphans, mistyped -def check_config_orphan_free(raw_dict: dict) -> bool: - """Detect if *raw_dict* contains keys that Config schema doesn't define. +def find_orphan_keys(raw_dict: dict) -> list[str]: + """List the keys in *raw_dict* that the Config schema does not define. Parameters ---------- @@ -110,27 +184,91 @@ def check_config_orphan_free(raw_dict: dict) -> bool: Raw TOML dict as returned by `tomllib.load`. Returns - ------ - bool - True if looks good. False if unrecognised keys are found. + ------- + list[str] + Dotted key paths (e.g. ``"planet.orphan_field"``) in file order. + Empty when every key maps onto a Config field. """ + return _collect_orphan_keys(raw_dict, Config) + + +def find_key_problems(raw_dict: dict) -> tuple[list[str], list[str]]: + """List the keys in *raw_dict* that the Config schema cannot accept. + + Both kinds leave the file saying one thing and the run doing another, so + both are reported together. + + Parameters + ---------- + raw_dict: + Raw TOML dict as returned by `tomllib.load`. - # Identify orphaned keys in the raw_dict - orphans = _collect_orphan_keys(raw_dict, Config) + Returns + ------- + tuple[list[str], list[str]] + Dotted key paths in file order: names the schema does not define, and + sections the schema declares as a table but which the file supplies as + something else. Both are empty for a conforming file. + """ + return _collect_key_problems(raw_dict, Config) - # No orphans -> looks good - if not orphans: - return True - # If didn't return, then we have some orphans to deal with - # Construct a message for the user. - log.error('Configuration contains "orphan" keys that are not recognised.') - log.error('\tPerhaps you have a typo or are using an outdated option name.') - log.error('\tCheck input/all_options.toml for parameter reference.') +def format_orphan_message( + orphans: list[str], path: Path | str, mistyped: list[str] | None = None +) -> str: + """Build the user-facing message naming the keys that were refused. - # List the keys - msg = '\tUnrecognised keys: ' + ', '.join(f'"{key}"' for key in orphans) - log.error(msg) + Parameters + ---------- + orphans: + Dotted key paths the schema does not define. + path: + Config file the keys came from, quoted back to the user. + mistyped: + Dotted paths of sections the schema declares as a table but which the + file supplies as something else. Omit when there are none. - # Return false if orphans - return False + Returns + ------- + str + Multi-line message listing the keys and how to resolve them. + """ + blocks: list[str] = [] + + if orphans: + keys = ', '.join(f'"{key}"' for key in orphans) + single = len(orphans) == 1 + heading = ( + 'Unrecognised configuration key' if single else 'Unrecognised configuration keys' + ) + subject = 'This key is' if single else 'These keys are' + setting = 'Setting it' if single else 'Setting them' + blocks.append( + f'{heading} in {path}:\n' + f' {keys}\n' + f' {subject} not part of the configuration schema. {setting} ' + f'has no effect, so the file is refused rather than run on defaults ' + f'that were not asked for. Check for a typo or an outdated option ' + f'name.' + ) + + if mistyped: + sections = ', '.join(f'"{key}"' for key in mistyped) + single = len(mistyped) == 1 + heading = ( + 'Misdeclared configuration section' + if single + else 'Misdeclared configuration sections' + ) + subject = 'This section is' if single else 'These sections are' + blocks.append( + f'{heading} in {path}:\n' + f' {sections}\n' + f' {subject} declared as a table by the schema, but the file gives ' + f'another kind of value. Writing a section as [[name]] rather than ' + f'[name] is the usual cause. As written the whole section is ' + f'discarded and every parameter inside it falls back to its default.' + ) + + blocks.append('See input/all_options.toml for the full parameter reference.') + return '\n'.join(blocks) diff --git a/src/proteus/proteus.py b/src/proteus/proteus.py index ef4ab3ad6..e71fcbad6 100644 --- a/src/proteus/proteus.py +++ b/src/proteus/proteus.py @@ -15,7 +15,13 @@ from juliacall import Main # noqa: F401 import proteus.utils.archive as archive -from proteus.config import check_config_orphan_free, read_config, read_config_object +from proteus.config import ( + UnknownConfigKeyError, + find_key_problems, + format_orphan_message, + read_config, + read_config_object, +) from proteus.utils.constants import noble_gases, vap_list, vol_list from proteus.utils.helper import ( CleanDir, @@ -41,19 +47,64 @@ class Proteus: def __init__(self, *, config_path: Path | str) -> None: - # Read and parse configuration file + # Read and parse configuration file. Keys the schema cannot accept are + # collected here but reported further down: resolving the output + # directory needs a structured config, and the refusal is recorded in a + # status file under that directory. self.config_path = config_path - self.config = read_config_object(config_path) + + # The keys are collected before the config is structured, because a + # misspelling is usually what makes a value fail validation and is the + # more useful of the two to report. The check re-reads the raw TOML, so + # it applies only when the path resolves to a file: a caller that + # substitutes the loader supplies the parsed config by other means and + # leaves nothing here to re-read. + orphans: list[str] = [] + mistyped: list[str] = [] + if os.path.isfile(config_path): + orphans, mistyped = find_key_problems(read_config(config_path)) + orphan_error = ( + format_orphan_message(orphans, config_path, mistyped) + if orphans or mistyped + else None + ) + + try: + self.config = read_config_object(config_path, strict=False) + except ValueError as exc: + # An unrecognised key is reported first because it is often what + # made the rest of the file fail, but the other complaint is kept + # alongside it: it may name a missing package or an unreadable + # path, which the key on its own does not explain. There is no + # output directory to record this in, since resolving one needs the + # config that just failed to structure. + if orphan_error: + raise UnknownConfigKeyError( + f'{orphan_error}\nLoading the file also reported:\n{exc}' + ) from None + raise # Setup directories dictionary self.directories: dict = None # Directories dictionary - self.init_directories() - - # Check for orphan keys in the config - if self.directories and os.path.isfile(config_path): - if not check_config_orphan_free(read_config(config_path)): - UpdateStatusfile(self.directories, 20) - raise RuntimeError(f'Unknown configuration keys found in {config_path}.') + try: + self.init_directories() + except Exception as exc: + # Resolving the directories needs a configured environment, so it + # can fail for reasons of its own. A key already found unrecognised + # is reported alongside that failure rather than dropped: someone + # setting up for the first time can easily have both, and being + # told only about the environment hides the typo until the next + # attempt. + if orphan_error: + raise UnknownConfigKeyError( + f'{orphan_error}\nResolving the output directory also failed:\n{exc}' + ) from None + raise + + # Reject unrecognised keys now that the failure can be recorded. + if orphan_error: + UpdateStatusfile(self.directories, 20) + raise UnknownConfigKeyError(orphan_error) # Helpfile variables for the current iteration self.hf_row = None diff --git a/tests/config/test_config.py b/tests/config/test_config.py index d26006e84..99296e685 100644 --- a/tests/config/test_config.py +++ b/tests/config/test_config.py @@ -12,6 +12,7 @@ from itertools import chain from types import SimpleNamespace +import attrs import pytest from helpers import PROTEUS_ROOT @@ -65,6 +66,156 @@ def test_read_config_returns_dict(): assert 'params' in raw or 'star' in raw or 'orbit' in raw +def _write_variant(tmp_path, name, mutate): + """Copy input/dummy.toml, apply *mutate* to the raw dict, write it back. + + Returns the path of the written file. Used by the strict-loading tests + below to build near-identical configs that differ only in the key under + test. + """ + import tomllib + + import tomlkit + + with open(PROTEUS_ROOT / 'input' / 'dummy.toml', 'rb') as f: + raw = tomllib.load(f) + mutate(raw) + path = tmp_path / name + with open(path, 'w') as f: + tomlkit.dump(raw, f) + return path + + +@pytest.mark.unit +def test_read_config_object_rejects_a_misspelled_field(tmp_path): + """A misspelled field is refused instead of falling back to its default. + + The two files here differ only in the spelling of one key. Spelled wrong, + the load is refused and the offending key is named; spelled right, the same + number reaches the Config. Without the refusal both files would load and + the user would have no way to tell which value took effect. + """ + + def _typo(raw): + del raw['planet']['mass_tot'] + raw['planet']['mass_total'] = 2.5 # deliberate misspelling + + def _correct(raw): + raw['planet']['mass_tot'] = 2.5 + + with pytest.raises(ValueError, match='planet.mass_total'): + read_config_object(_write_variant(tmp_path, 'typo.toml', _typo)) + + # The correctly spelled file carries the value through, so the refusal + # above is about the spelling and not about the value 2.5 itself. + cfg = read_config_object(_write_variant(tmp_path, 'correct.toml', _correct)) + assert cfg.planet.mass_tot == pytest.approx(2.5) + + # 2.5 differs from what dummy.toml sets, so a loader that ignored the file + # and returned the untouched config would fail the check above. + assert read_config(PROTEUS_ROOT / 'input' / 'dummy.toml')['planet'][ + 'mass_tot' + ] != pytest.approx(2.5) + + +@pytest.mark.unit +def test_read_config_object_rejects_a_section_written_as_an_array_of_tables(tmp_path): + """``[[planet]]`` is refused instead of dropping the whole section. + + An array of tables carries a name the schema declares, so nothing about the + name is wrong and a check that only compares names passes it. Structuring + then discards the section entirely and every parameter inside it reverts to + its default, which is the silent fallback this loader exists to stop, and a + worse case than one misspelled key because it takes the whole section with + it. + """ + source = (PROTEUS_ROOT / 'input' / 'dummy.toml').read_text() + + # 3.7 differs from the schema default, so whether the section was read or + # dropped is visible in the loaded value rather than having to be inferred. + source = source.replace('mass_tot = 1.0', 'mass_tot = 3.7') + assert 'mass_tot = 3.7' in source, 'fixture no longer matches dummy.toml' + assert attrs.fields(Planet).mass_tot.default == pytest.approx(1.0) + + single = tmp_path / 'single_table.toml' + single.write_text(source) + # Control: as a plain table the value reaches the Config, so the refusal + # below is caused by the brackets and not by the value or this fixture. + assert read_config_object(single).planet.mass_tot == pytest.approx(3.7) + + array = tmp_path / 'array_of_tables.toml' + array.write_text(source.replace('\n[planet]\n', '\n[[planet]]\n')) + with pytest.raises(ValueError, match='planet') as excinfo: + read_config_object(array) + assert 'Misdeclared' in str(excinfo.value) + + # Without the refusal the run would proceed on 1.0 while the file asks for + # 3.7. Pinning that here shows the check is what stands between the two. + dropped = read_config_object(array, strict=False) + assert dropped.planet.mass_tot == pytest.approx(1.0) + assert dropped.planet.mass_tot != pytest.approx(3.7) + + +@pytest.mark.unit +def test_read_config_object_without_strict_applies_the_default(tmp_path): + """With strict off the unknown key is dropped and the default takes over. + + This is exactly the outcome strict loading exists to prevent, and it is the + path the runner takes so it can record the failure in a status file before + refusing. Pinning it keeps the escape hatch honest: it tolerates the key, it + does not honour it. + """ + + # Three distinct masses so the surviving value identifies its own source: + # 2.5 would mean the misspelling was honoured, 1.0 (the Planet.mass_tot + # schema default) would mean the whole section was dropped, 1.75 means the + # correctly spelled sibling key was read and only the typo discarded. + def _typo(raw): + raw['planet']['mass_tot'] = 1.75 + raw['planet']['mass_total'] = 2.5 + + path = _write_variant(tmp_path, 'typo.toml', _typo) + + cfg = read_config_object(path, strict=False) + assert cfg.planet.mass_tot == pytest.approx(1.75) + assert cfg.planet.mass_tot != pytest.approx(2.5) + + # Not the schema default either, so a loader that discarded the section + # wholesale rather than just the unknown key would fail here. + assert attrs.fields(Planet).mass_tot.default == pytest.approx(1.0) + + # Same file, same content: only the parameter decides whether it loads. + with pytest.raises(ValueError, match='planet.mass_total'): + read_config_object(path) + + +@pytest.mark.unit +def test_read_config_object_names_the_unknown_key_before_a_bad_value(tmp_path): + """An unknown key is reported ahead of a value that fails validation. + + A misspelling is usually what caused the downstream value to be wrong, so + reporting the value error first would send the user after a symptom while + the cause stays invisible. + """ + + def _both(raw): + raw['planet']['mass_tot'] = -5.0 # invalid: mass must be positive + raw['planet']['mass_total'] = 2.5 # unknown key + + def _value_only(raw): + raw['planet']['mass_tot'] = -5.0 + + with pytest.raises(ValueError, match='planet.mass_total') as excinfo: + read_config_object(_write_variant(tmp_path, 'both.toml', _both)) + # The value complaint is held back until the schema mismatch is resolved. + assert 'must be > 0' not in str(excinfo.value) + + # Edge case: with the unknown key removed the negative mass is still caught, + # so the ordering above suppresses the message rather than the check. + with pytest.raises(ValueError, match='must be > 0'): + read_config_object(_write_variant(tmp_path, 'value_only.toml', _value_only)) + + @pytest.mark.unit def test_valid_config_version_rejects_old(): """config_version validator rejects old versions with a clear upgrade message.""" diff --git a/tests/config/test_orphans.py b/tests/config/test_orphans.py index dd6abf0d0..5ae8a8966 100644 --- a/tests/config/test_orphans.py +++ b/tests/config/test_orphans.py @@ -6,12 +6,15 @@ from __future__ import annotations +import attrs import pytest from proteus.config.orphans import ( _collect_orphan_keys, _extract_attrs_class, - check_config_orphan_free, + find_key_problems, + find_orphan_keys, + format_orphan_message, ) pytestmark = [pytest.mark.unit, pytest.mark.timeout(30)] @@ -21,6 +24,28 @@ # Helpers # --------------------------------------------------------------------------- + +@attrs.define +class _Leaf: + """Innermost class of the synthetic schema used below.""" + + value: int = 0 + + +@attrs.define +class _Holder: + """Synthetic schema pairing a container of tables with a single table. + + The real Config declares no container of nested classes, so the two shapes + can only be compared against a schema written for the purpose. Defined at + module scope because the walk resolves annotations through + `typing.get_type_hints`, which cannot see names local to a function. + """ + + many: list[_Leaf] = attrs.field(factory=list) + one: _Leaf = attrs.field(factory=_Leaf) + + _MINIMAL_VALID = { 'config_version': '3.0', 'star': {'module': 'dummy', 'dummy': {'radius': 1.0}}, @@ -250,25 +275,201 @@ def test_orphans_collect_orphan_keys_non_attrs_type_skips_recursion(): # --------------------------------------------------------------------------- -# check_config_orphan_free +# find_orphan_keys # --------------------------------------------------------------------------- -def test_orphans_check_config_orphan_free_clean_config_passes(): - """A config with no orphans returns True without raising. +def test_find_orphan_keys_reports_nested_paths_and_none_for_a_clean_config(): + """Unknown keys come back as dotted paths; a schema-conforming config yields none. - The paired negative injects one unknown key into that config and expects - False rather than a raise. + The nested case is the one that matters: a misspelling two levels down in + ``planet.elements`` costs the user the same silently-applied default as one + at the top level, but only recursion into the nested class can see it. """ - result = check_config_orphan_free(_MINIMAL_VALID) - assert result is True + from copy import deepcopy + + dirty = deepcopy(_MINIMAL_VALID) + # Plural typo two levels deep, and an unknown top-level section. + dirty['planet']['elements']['H_budgets'] = 1.0 + dirty['atmosphere'] = {'module': 'agni'} + + orphans = find_orphan_keys(dirty) + assert set(orphans) == {'planet.elements.H_budgets', 'atmosphere'} + + # The correctly spelled sibling sits right next to the typo and must not be + # swept up with it, otherwise every config with a typo would look wholly + # unrecognised. + assert 'planet.elements.H_budget' not in orphans - # Error contract: the same config carrying one unknown key returns False - # rather than raising, because the caller reports the keys and decides. The - # pair also shows True is driven by the config content, not returned - # unconditionally. - dirty = {**_MINIMAL_VALID, 'planet': {**_MINIMAL_VALID['planet'], 'typo_field': 99}} - assert check_config_orphan_free(dirty) is False + # An unknown section is reported once, at the section, rather than once per + # key inside it: there is no schema below an unrecognised branch to compare + # against. + assert 'atmosphere.module' not in orphans + + # Edge case: the same config without the two injected keys is clean, so the + # result above is driven by the injection rather than by the fixture. + assert find_orphan_keys(_MINIMAL_VALID) == [] + + +# --------------------------------------------------------------------------- +# find_key_problems +# --------------------------------------------------------------------------- + + +def test_find_key_problems_catches_a_section_that_is_not_a_table(): + """A section given as anything but a table is reported, at any depth. + + Writing ``[[planet]]`` instead of ``[planet]`` declares an array of tables. + The name still matches a schema field, so a check that only looks at names + sees nothing wrong, while structuring discards the section whole and every + parameter inside it silently reverts to its default. That is the same + outcome as a misspelling and is reported the same way. + """ + from copy import deepcopy + + # A discriminating value: it differs from the schema default, so a section + # that is dropped rather than read is visible in the result. + good = deepcopy(_MINIMAL_VALID) + good['planet']['mass_tot'] = 3.7 + assert find_key_problems(good) == ([], []) + + # Array of tables at the top level. + aot = deepcopy(good) + aot['planet'] = [good['planet']] + assert find_key_problems(aot) == ([], ['planet']) + + # Array of tables one level down, where only recursion can see it. + nested = deepcopy(good) + nested['planet']['elements'] = [good['planet']['elements']] + assert find_key_problems(nested) == ([], ['planet.elements']) + + # Edge cases: a scalar and a string in place of a table are the same fault. + for value in (5, 'earth'): + scalar = deepcopy(good) + scalar['planet'] = value + assert find_key_problems(scalar) == ([], ['planet']) + + # The two kinds are reported separately rather than one masking the other. + both = deepcopy(good) + both['planet'] = [good['planet']] + both['star']['typo_field'] = 1 + assert find_key_problems(both) == (['star.typo_field'], ['planet']) + + +def test_collect_key_problems_allows_repeated_tables_for_a_container_field(): + """A field holding several nested classes accepts a list rather than a table. + + The schema declares no such field today, so this is checked against a + synthetic one. It matters because the rule that refuses a non-table is + otherwise applied to every nested class: adding a field typed as a list of + them later would start refusing configurations that are correct, which is a + worse failure than the one the refusal exists to prevent. + """ + from proteus.config.orphans import _collect_key_problems + + # A list against the container field is the intended shape, so neither list + # reports it. + assert _collect_key_problems({'many': [{'value': 1}]}, _Holder) == ([], []) + + # Discriminator: the same list against the single-table field beside it is + # still refused, so the carve-out is driven by the field's type and is not + # a blanket exemption for lists. + assert _collect_key_problems({'one': [{'value': 1}]}, _Holder) == ([], ['one']) + + # A table remains valid for the single-table field, and an unknown name + # inside it is still found, so recursion is unaffected. + assert _collect_key_problems({'one': {'value': 1}}, _Holder) == ([], []) + assert _collect_key_problems({'one': {'nope': 1}}, _Holder) == (['one.nope'], []) + + +def test_find_key_problems_leaves_a_mistyped_section_out_of_the_orphan_list(): + """A misdeclared section is not reported as an unrecognised key. + + ``planet`` is a name the schema declares, so calling it unrecognised would + send the user looking for a spelling mistake that is not there. The two + faults need different advice and are kept apart. + """ + from copy import deepcopy + + aot = deepcopy(_MINIMAL_VALID) + aot['planet'] = [_MINIMAL_VALID['planet']] + + orphans, mistyped = find_key_problems(aot) + assert mistyped == ['planet'] + assert orphans == [] + # The name-only helper agrees, so the split is a property of the walk and + # not of one caller's interpretation. + assert find_orphan_keys(aot) == [] + + +# --------------------------------------------------------------------------- +# format_orphan_message +# --------------------------------------------------------------------------- + + +def test_format_orphan_message_quotes_every_key_and_names_the_file(): + """The rejection message lists all unrecognised keys, the file, and the reference. + + This message is the only guidance the user gets when a load is refused, so + it has to name every key rather than just the first one, and say where the + valid names are written down. + """ + msg = format_orphan_message(['planet.mass_total', 'atmosphere'], '/runs/case.toml') + + # Both keys, not just the head of the list. + assert '"planet.mass_total"' in msg + assert '"atmosphere"' in msg + assert '/runs/case.toml' in msg + assert 'all_options.toml' in msg + + # Limit input: one key yields exactly one quoted name. A regression that + # padded the list with a stray empty entry would quote four times here. + single = format_orphan_message(['params.dt.maxium'], 'case.toml') + assert single.count('"') == 2 + assert '"params.dt.maxium"' in single + + # The wording agrees with the count. Telling someone who mistyped one key + # that "these keys are not part of the schema" reads as though the file has + # more wrong with it than it does. + assert 'Unrecognised configuration key in' in single + assert 'This key is not part' in single + assert 'these keys' not in single.lower() + + # The many-key message keeps the plural, so the singular above is chosen + # from the count rather than applied to every message. + assert 'Unrecognised configuration keys in' in msg + assert 'These keys are not part' in msg + + +def test_format_orphan_message_reports_mistyped_sections_in_their_own_block(): + """A misdeclared section gets its own advice, not the spelling advice. + + The remedy differs: an unrecognised key wants a spelling check, a section + written as an array of tables wants a bracket removed. Folding them into + one block would give whichever user is in the minority the wrong + instruction. + """ + both = format_orphan_message(['planet.mass_total'], '/runs/case.toml', ['star']) + + assert '"planet.mass_total"' in both + assert '"star"' in both + # The unrecognised key comes first: it is the more common mistake, and the + # section advice is useless to someone who has neither. + assert both.index('Unrecognised') < both.index('Misdeclared') + assert '[[name]]' in both + # The reference line is printed once, not once per block. + assert both.count('all_options.toml') == 1 + + # Only sections: the spelling block is absent rather than empty, so nobody + # is told to check a spelling when no name was misspelled. + sections_only = format_orphan_message([], '/runs/case.toml', ['star']) + assert 'Misdeclared configuration section in' in sections_only + assert 'Unrecognised' not in sections_only + + # Only keys: symmetrically, no bracket advice appears. + keys_only = format_orphan_message(['planet.mass_total'], '/runs/case.toml') + assert 'Misdeclared' not in keys_only + assert '[[name]]' not in keys_only # --------------------------------------------------------------------------- @@ -277,7 +478,7 @@ def test_orphans_check_config_orphan_free_clean_config_passes(): def test_orphans_detected_in_raw_dict_from_toml_file(tmp_path): - """check_config_orphan_free detects orphan keys injected into a TOML file.""" + """An orphan key injected into a TOML file is found by name.""" import tomllib from helpers import PROTEUS_ROOT @@ -295,17 +496,16 @@ def test_orphans_detected_in_raw_dict_from_toml_file(tmp_path): with open(cfg_path, 'rb') as f: raw = tomllib.load(f) - result = check_config_orphan_free(raw) - assert result is False + assert find_orphan_keys(raw) == ['planet.TYPO_FIELD'] # Discrimination: the original dummy.toml must be orphan-free. A regression - # that always returned False would fail this second check. + # that always reported a key would fail this second check. with open(dummy_path, 'rb') as f: clean_raw = tomllib.load(f) - assert check_config_orphan_free(clean_raw) is True + assert find_orphan_keys(clean_raw) == [] def test_orphans_detected_at_top_level_in_toml_file(tmp_path): - """check_config_orphan_free detects an unknown top-level key in a TOML file.""" + """An unknown top-level key in a TOML file is found by name.""" import tomllib from helpers import PROTEUS_ROOT @@ -323,14 +523,14 @@ def test_orphans_detected_at_top_level_in_toml_file(tmp_path): raw = tomllib.load(f) # The orphan key is detected at the TOP level. - result = check_config_orphan_free(raw) - assert result is False + orphans = find_orphan_keys(raw) + assert orphans == ['EXTRA_SECTION'] - # Discrimination: confirm the orphan is specifically EXTRA_SECTION + # Discrimination: the same walk over the schema root reports the identical + # key, so the public helper is not silently widening the search. from proteus.config._config import Config - orphans = _collect_orphan_keys(raw, Config) - assert 'EXTRA_SECTION' in orphans + assert _collect_orphan_keys(raw, Config) == orphans # --------------------------------------------------------------------------- @@ -339,11 +539,14 @@ def test_orphans_detected_at_top_level_in_toml_file(tmp_path): def test_orphans_proteus_init_raises_on_dirty_config(tmp_path): - """Proteus.__init__ raises RuntimeError and writes status 20 for a dirty config. + """The runner names the unknown key and writes status 20 before refusing. The dummy.toml is used as the valid base; one orphan key is injected into the [planet] section. Proteus resolves the output directory from the config, writes the status file, and then raises before completing initialisation. + The key has to appear in the exception itself: at this point in startup no + log handler is attached, so anything reported only through the logger is + lost and the user is left with a run that stopped for no stated reason. """ import tomllib @@ -367,9 +570,12 @@ def test_orphans_proteus_init_raises_on_dirty_config(tmp_path): with open(dirty_path, 'w') as f: tomlkit.dump(raw, f) - with pytest.raises(RuntimeError, match='Unknown configuration keys'): + with pytest.raises(ValueError) as excinfo: Proteus(config_path=dirty_path) + # The offending key is carried by the exception, not only by a log line. + assert 'planet.TYPO_FIELD' in str(excinfo.value) + # Status file must exist (this checks for regression where file isn't written) status_file = tmp_path / 'run_output' / 'status' assert status_file.exists(), 'Status file must be written before the raise' @@ -379,6 +585,160 @@ def test_orphans_proteus_init_raises_on_dirty_config(tmp_path): assert content.startswith('20'), f'Expected status 20, got: {content!r}' +def test_orphans_proteus_init_names_the_unknown_key_before_a_bad_value(tmp_path): + """The runner leads with an unknown key but keeps the other complaint too. + + A misspelling is usually what leaves a value wrong, so reporting the value + first sends the user after a symptom. Dropping the value complaint is no + better: it can name a missing package or an unreadable path that the key + alone does not explain, so both belong in the message with the key first. + + A file that fails to structure has no status file written for it, unlike a + file whose only fault is the key. The output directory is named inside the + configuration, so a file that cannot be structured has nowhere to record + anything; the message is the whole of the report. That limit is pinned here + so it stays a deliberate one. + """ + import tomllib + + import tomlkit + from helpers import PROTEUS_ROOT + + from proteus import Proteus + + with open(PROTEUS_ROOT / 'input' / 'dummy.toml', 'rb') as f: + raw = tomllib.load(f) + raw['params']['out']['path'] = str(tmp_path / 'run_output') + raw['planet']['mass_tot'] = -5.0 # rejected: the planet mass must be positive + raw['planet']['mass_total'] = 2.5 # unknown key + + both = tmp_path / 'both.toml' + with open(both, 'w') as f: + tomlkit.dump(raw, f) + + with pytest.raises(ValueError) as excinfo: + Proteus(config_path=both) + message = str(excinfo.value) + assert 'planet.mass_total' in message + # The value complaint survives alongside it, and comes second. + assert 'must be > 0' in message + assert message.index('planet.mass_total') < message.index('must be > 0') + + # Nothing was recorded on disk: structuring failed before the output + # directory could be resolved, so the refusal lives only in the message. + # The sibling test above, where the key is the only fault, does get a + # status file, and that contrast is the point. + assert not (tmp_path / 'run_output' / 'status').exists() + + # Edge case: with the unknown key removed the negative mass is still the + # only thing reported, so the key does not crowd out an unrelated failure + # and does not attach itself to a file that has none. + del raw['planet']['mass_total'] + value_only = tmp_path / 'value_only.toml' + with open(value_only, 'w') as f: + tomlkit.dump(raw, f) + with pytest.raises(ValueError) as value_info: + Proteus(config_path=value_only) + assert 'must be > 0' in str(value_info.value) + # Singular substring, so neither the one-key nor the many-key heading slips + # past this check. + assert 'Unrecognised configuration key' not in str(value_info.value) + + +def test_orphans_proteus_init_refuses_a_section_written_as_an_array_of_tables(tmp_path): + """The runner refuses ``[[planet]]`` and records it, like any other refusal. + + The runner checks the raw file itself rather than letting the strict loader + do it, so the loader being correct says nothing about this path, which is + the one an actual run takes. Left unrefused the section is discarded whole + and every parameter inside it reverts to its default. + """ + from helpers import PROTEUS_ROOT + + from proteus import Proteus + + source = (PROTEUS_ROOT / 'input' / 'dummy.toml').read_text() + # A value distinct from the schema default, so a dropped section would be + # visible rather than having to be inferred. + source = source.replace('mass_tot = 1.0', 'mass_tot = 3.7') + assert 'mass_tot = 3.7' in source, 'fixture no longer matches dummy.toml' + assert source.count('path = "auto"') == 1, 'fixture no longer matches dummy.toml' + source = source.replace('path = "auto"', f'path = "{tmp_path / "run_output"}"') + + array = tmp_path / 'array_of_tables.toml' + array.write_text(source.replace('\n[planet]\n', '\n[[planet]]\n')) + + with pytest.raises(ValueError) as excinfo: + Proteus(config_path=array) + assert 'planet' in str(excinfo.value) + assert 'Misdeclared' in str(excinfo.value) + + # Recorded on disk as well, so a run stopped this way is distinguishable + # from one that died without reaching the check. + status_file = tmp_path / 'run_output' / 'status' + assert status_file.exists(), 'Status file must be written before the raise' + assert status_file.read_text().strip().startswith('20') + + # Discriminator: the same file as a plain table is accepted and carries the + # value, so the refusal is caused by the brackets rather than by this + # fixture being unloadable. + single = tmp_path / 'single_table.toml' + single.write_text(source) + assert Proteus(config_path=single).config.planet.mass_tot == pytest.approx(3.7) + + +def test_orphans_proteus_init_keeps_the_key_when_the_directories_fail(tmp_path, monkeypatch): + """A pending unknown key survives a failure to resolve the output directory. + + Resolving the directories needs a configured environment and can fail on + its own, most often because FWL_DATA is unset. Someone installing for the + first time can easily have both that and a typo, and reporting only the + environment leaves the typo to be discovered on the next attempt. + """ + import tomllib + + import tomlkit + from helpers import PROTEUS_ROOT + + import proteus.utils.coupler as coupler + from proteus import Proteus + + def _no_directories(_config): + raise OSError('The FWL_DATA environment variable has not been set.') + + monkeypatch.setattr(coupler, 'set_directories', _no_directories) + + with open(PROTEUS_ROOT / 'input' / 'dummy.toml', 'rb') as f: + raw = tomllib.load(f) + raw['params']['out']['path'] = str(tmp_path / 'run_output') + raw['planet']['mass_total'] = 2.5 # unknown key, the only fault in the file + + typo = tmp_path / 'typo.toml' + with open(typo, 'w') as f: + tomlkit.dump(raw, f) + + with pytest.raises(ValueError) as excinfo: + Proteus(config_path=typo) + message = str(excinfo.value) + # Both complaints reach the user, with the key first: the environment is + # the easier of the two to notice without being told. + assert 'planet.mass_total' in message + assert 'FWL_DATA' in message + assert message.index('planet.mass_total') < message.index('FWL_DATA') + + # Edge case and discriminator: the same directory failure on a file with no + # unknown key propagates untouched, so the wrapping is driven by the pending + # key rather than applied to every startup failure. + del raw['planet']['mass_total'] + clean = tmp_path / 'clean.toml' + with open(clean, 'w') as f: + tomlkit.dump(raw, f) + with pytest.raises(OSError) as os_info: + Proteus(config_path=clean) + assert 'FWL_DATA' in str(os_info.value) + assert 'Unrecognised configuration key' not in str(os_info.value) + + def test_orphans_proteus_init_succeeds_on_clean_config(tmp_path): """Proteus.__init__ completes without error when the config has no orphan keys. @@ -445,7 +805,7 @@ def test_all_options_phase_boundary_margin_declared_and_resolves(): # The whole reference file stays orphan-free, so the new key has a schema # home and is not one of the silently-discarded orphans. - assert check_config_orphan_free(raw) is True + assert find_orphan_keys(raw) == [] # End-to-end resolution through the parser yields the same 200.0. Pinning # the exact band is itself the discriminator: it rejects a 0.0 step-cap- diff --git a/tests/grid/test_manage.py b/tests/grid/test_manage.py index c86cc0899..a8b7992c1 100644 --- a/tests/grid/test_manage.py +++ b/tests/grid/test_manage.py @@ -577,6 +577,61 @@ def write(self, path): values = {v for _attr, v in recursive_calls} assert values == {0.5, 1.0, 200.0} + def test_unknown_key_in_the_base_config_stops_the_grid( + self, fake_proteus_dir, tmp_path, monkeypatch + ): + """A misspelled key in the base config halts the grid before any case is written. + + Per-case configs are serialised out of the structured object, so an + unrecognised key in the base file never survives into them. Nothing + downstream can notice it: every case config reads as clean while the + whole grid runs on a default the user did not choose. Loading the base + file is the only place the mistake is still visible. + """ + import tomllib + + import tomlkit + from helpers import PROTEUS_ROOT + + with open(PROTEUS_ROOT / 'tests' / 'grid' / 'base.toml', 'rb') as f: + raw = tomllib.load(f) + # Misspell the maximum time-step. Left unchecked this silently reverts + # to the schema default for every case in the grid. + raw['params']['dt']['maxium'] = raw['params']['dt'].pop('maximum') + dirty = tmp_path / 'dirty_base.toml' + with open(dirty, 'w') as f: + tomlkit.dump(raw, f) + + monkeypatch.setattr(gm.os, 'sync', lambda: None) + g = Grid(name='typo_base_grid', base_config_path=str(dirty)) + g.add_dimension('m', 'planet.mass_tot') + g.set_dimension_direct('m', [0.5, 1.0]) + g.generate() + + with pytest.raises(ValueError, match='params.dt.maxium'): + g.write_config_files() + + # No case config was written, so the grid cannot proceed on the + # silently defaulted value. + assert not [p for p in os.listdir(g.cfgdir) if p.startswith('case_')] + + # Edge case and discriminator: the unmodified base file writes both + # cases, so the refusal above is caused by the misspelling and not by + # this base config being unloadable in the first place. + clean = tmp_path / 'clean_base.toml' + with open(PROTEUS_ROOT / 'tests' / 'grid' / 'base.toml', 'rb') as f: + with open(clean, 'w') as out: + tomlkit.dump(tomllib.load(f), out) + g2 = Grid(name='clean_base_grid', base_config_path=str(clean)) + g2.add_dimension('m', 'planet.mass_tot') + g2.set_dimension_direct('m', [0.5, 1.0]) + g2.generate() + g2.write_config_files() + assert sorted(p for p in os.listdir(g2.cfgdir) if p.startswith('case_')) == [ + 'case_000000.toml', + 'case_000001.toml', + ] + # --------------------------------------------------------------------------- # Grid.slurm_config diff --git a/tests/test_cli.py b/tests/test_cli.py index dcb298965..4e9e80bd0 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -10,6 +10,7 @@ from proteus import __version__ as proteus_version from proteus import cli +from proteus.config import UnknownConfigKeyError pytestmark = [pytest.mark.unit, pytest.mark.timeout(30)] @@ -560,6 +561,59 @@ def fake_download_melting_curves(configuration, clean: bool = False): assert not any(c[0] == 'zalmoxis_eos' for c in calls) +@pytest.mark.unit +def test_get_interiordata_reports_an_unknown_config_key_cleanly(monkeypatch, tmp_path): + """A misspelled key stops the download and is reported as a CLI error. + + The download commands act on the configuration, so acting on one whose keys + were silently discarded would fetch data for a setup the user did not ask + for. The failure has to arrive in the CLI's own error style with the key + named, not as a traceback. + """ + import tomllib + + import tomlkit + from helpers import PROTEUS_ROOT + + runner = CliRunner() + downloads = [] + monkeypatch.setattr( + 'proteus.utils.data.download_interior_lookuptables', + lambda clean=False: downloads.append('interior'), + ) + monkeypatch.setattr( + 'proteus.utils.data.download_melting_curves', + lambda configuration, clean=False: downloads.append('melt'), + ) + + with open(PROTEUS_ROOT / 'input' / 'dummy.toml', 'rb') as f: + raw = tomllib.load(f) + raw['planet']['mass_total'] = 2.5 # deliberate misspelling of mass_tot + cfg = tmp_path / 'typo.toml' + with open(cfg, 'w') as f: + tomlkit.dump(raw, f) + + res = runner.invoke(cli.cli, ['get', 'interiordata', '--config-path', str(cfg)]) + assert res.exit_code != 0 + assert 'planet.mass_total' in res.output + # A ClickException prints "Error: ..." and does not surface a traceback. + assert 'Traceback' not in res.output + # The config-dependent download is not reached; the config-independent one + # ahead of it may already have run, which is why only the former is pinned. + assert 'melt' not in downloads + + # Discrimination: with the key spelled correctly the same command completes + # and the melting-curve download does run, so the refusal is caused by the + # misspelling rather than by this config being unusable. + raw['planet']['mass_tot'] = raw['planet'].pop('mass_total') + good = tmp_path / 'good.toml' + with open(good, 'w') as f: + tomlkit.dump(raw, f) + res_ok = runner.invoke(cli.cli, ['get', 'interiordata', '--config-path', str(good)]) + assert res_ok.exit_code == 0 + assert 'melt' in downloads + + @pytest.mark.unit def test_get_interiordata_fetches_zalmoxis_eos(monkeypatch, tmp_path): """``proteus get interiordata`` downloads the structure EOS tables when the @@ -1141,6 +1195,144 @@ def fake_grid_from_config(path, test_run=False): assert received[0][1] is False +def test_start_reports_an_unknown_config_key_cleanly(tmp_path): + """``proteus start`` refuses a misspelled key in the CLI's own error style. + + This is the command most runs go through, so a refusal that arrives as a + bare traceback leaves the name of the offending key buried in it. + """ + import tomllib + + import tomlkit + from helpers import PROTEUS_ROOT + + with open(PROTEUS_ROOT / 'input' / 'dummy.toml', 'rb') as f: + raw = tomllib.load(f) + raw['params']['out']['path'] = str(tmp_path / 'run_output') + raw['planet']['mass_total'] = 2.5 # deliberate misspelling of mass_tot + cfg = tmp_path / 'typo.toml' + with open(cfg, 'w') as f: + tomlkit.dump(raw, f) + + res = runner.invoke(cli.cli, ['start', '-c', str(cfg), '--offline']) + assert res.exit_code != 0 + assert 'planet.mass_total' in res.output + assert 'Traceback' not in res.output + + +def test_cli_does_not_convert_unrelated_value_errors(tmp_path, monkeypatch): + """A failure that is not about configuration keys keeps its traceback. + + Presenting every ValueError as a tidy CLI message would hide real bugs, so + the group converts only the configuration-key error. + """ + + def boom(*_args, **_kwargs): + raise ValueError('something else went wrong entirely') + + monkeypatch.setattr(cli, 'Proteus', boom) + + cfg = tmp_path / 'cfg.toml' + cfg.write_text('config_version = "3.0"\n') + + res = runner.invoke(cli.cli, ['start', '-c', str(cfg), '--offline']) + assert res.exit_code != 0 + # Not laundered into "Error: ...": the exception escapes for the traceback. + assert isinstance(res.exception, ValueError) + assert 'something else went wrong entirely' in str(res.exception) + + +def test_grid_reports_an_unknown_key_in_the_base_config_cleanly(tmp_path, monkeypatch): + """``proteus grid`` refuses a base config with a misspelled key, in CLI style. + + Case config files are written out from the parsed base config, so an + unrecognised key in the base never reaches them and the grid would + otherwise run every case on a default nobody chose. The refusal has to name + the key and arrive as a CLI error, since the whole ensemble depends on it. + """ + import tomllib + + import tomlkit + from helpers import PROTEUS_ROOT + + import proteus.grid.manage as gmanage + + monkeypatch.setattr(gmanage, 'PROTEUS_DIR', str(tmp_path)) + monkeypatch.setattr(gmanage.time, 'sleep', lambda *_a, **_k: None) + + with open(PROTEUS_ROOT / 'tests' / 'grid' / 'base.toml', 'rb') as f: + base = tomllib.load(f) + base['params']['dt']['maxium'] = base['params']['dt'].pop('maximum') + base_path = tmp_path / 'base.toml' + with open(base_path, 'w') as f: + tomlkit.dump(base, f) + + grid_toml = tmp_path / 'run.grid.toml' + grid_toml.write_text( + 'config_version = "3.0"\n' + 'output = "cli_typo_grid"\n' + 'symlink = ""\n' + f'ref_config = "{base_path}"\n' + 'use_slurm = false\n' + 'max_jobs = 1\n' + 'max_days = 1\n' + 'max_mem = 1\n' + '["planet.mass_tot"]\n' + ' method = "direct"\n' + ' values = [0.7]\n' + ) + + res = runner.invoke(cli.cli, ['grid', '-c', str(grid_toml), '--dry-run']) + assert res.exit_code != 0 + assert 'params.dt.maxium' in res.output + # A ClickException prints "Error: ..."; an unwrapped raise prints a traceback. + assert 'Traceback' not in res.output + + +def test_update_input_data_refuses_an_unknown_config_key_before_downloading( + tmp_path, monkeypatch +): + """A misspelled key stops the data refresh before anything is fetched. + + This helper runs at the tail of installing and of updating, after every + other step has reported success, so refusing here has to happen before the + download rather than after it. The helper is called directly because the + commands that reach it, ``install-all`` and ``update-all``, perform a full + installation; that the error it raises is presented in the CLI's own style + rather than as a traceback is pinned by the group-level tests above. + """ + import tomllib + + import tomlkit + from helpers import PROTEUS_ROOT + + downloads = [] + monkeypatch.setattr( + cli, 'download_sufficient_data', lambda configuration, clean: downloads.append(clean) + ) + + with open(PROTEUS_ROOT / 'input' / 'dummy.toml', 'rb') as f: + raw = tomllib.load(f) + raw['planet']['mass_total'] = 2.5 # deliberate misspelling of mass_tot + cfg = tmp_path / 'typo.toml' + with open(cfg, 'w') as f: + tomlkit.dump(raw, f) + + with pytest.raises(UnknownConfigKeyError) as excinfo: + cli._update_input_data(cfg) + assert 'planet.mass_total' in str(excinfo.value) + assert not downloads + + # Discrimination: spelled correctly the same config completes the refresh, + # so the refusal is caused by the misspelling and not by this config. + raw['planet']['mass_tot'] = raw['planet'].pop('mass_total') + good = tmp_path / 'good.toml' + with open(good, 'w') as f: + tomlkit.dump(raw, f) + assert cli._update_input_data(good) is True + assert downloads == [True] + + def test_grid_dry_run_passes_test_run_flag(monkeypatch, tmp_path): """``proteus grid -c cfg --dry-run`` flips test_run to True so the grid is generated without launching PROTEUS. Discrimination: the only difference From 28952694b6e7dabc4a8711e9e226a88fc8e0283a Mon Sep 17 00:00:00 2001 From: timlichtenberg Date: Thu, 30 Jul 2026 20:41:11 +0200 Subject: [PATCH 2/3] Simplify the config key check to one walk and one entry point 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. --- src/proteus/config/__init__.py | 84 +++++++++++-------- src/proteus/config/orphans.py | 83 ++++--------------- src/proteus/proteus.py | 23 ++++-- tests/config/test_config.py | 25 +++--- tests/config/test_orphans.py | 142 +++++++++++++++++++++++---------- 5 files changed, 194 insertions(+), 163 deletions(-) diff --git a/src/proteus/config/__init__.py b/src/proteus/config/__init__.py index dae994526..1fae15b3e 100644 --- a/src/proteus/config/__init__.py +++ b/src/proteus/config/__init__.py @@ -7,12 +7,7 @@ import cattrs from ._config import Config -from .orphans import ( - UnknownConfigKeyError, - find_key_problems, - find_orphan_keys, - format_orphan_message, -) +from .orphans import UnknownConfigKeyError, find_key_problems, format_orphan_message log = logging.getLogger('fwl.' + __name__) @@ -26,20 +21,22 @@ def read_config(path: Path | str) -> dict: return config -def read_config_object(path: Path | str, *, strict: bool = True) -> Config: - """Read and validate config into Config object. +def structure_config(raw: dict, path: Path | str) -> Config: + """Structure a raw config dict into a Config object. + + This performs no key checking: cattrs discards anything the schema does not + map, so a caller that uses this directly is responsible for having checked + the keys itself. `read_config_object` is the checked entry point and is + what almost every caller wants. The step is separate so the runner can + obtain a configuration, resolve the output directory named inside it, and + record a refusal there before raising. Parameters ---------- + raw: + Raw TOML dict as returned by `read_config`. path: - Path to the TOML config file. - strict: - Reject keys the schema does not define, and sections it declares as a - table but the file supplies otherwise. cattrs discards both without - complaint, which turns a misspelling into a silent fallback to the - default, so rejection is the default here. Pass False only when the - caller performs the same check itself and can report the failure - better. + Config file the dict came from, quoted back in any error. Returns ------- @@ -48,27 +45,12 @@ def read_config_object(path: Path | str, *, strict: bool = True) -> Config: Raises ------ - UnknownConfigKeyError - If the file contains keys the schema cannot accept, when *strict*. ValueError If a value fails validation. """ - # Read config from TOML file in path as a raw dict. - cfg = read_config(path) - - # Reject unrecognised keys before structuring, so that a typo is reported - # as a typo rather than as whatever the resulting default happens to break - # further downstream. - if strict: - orphans, mistyped = find_key_problems(cfg) - if orphans or mistyped: - raise UnknownConfigKeyError(format_orphan_message(orphans, path, mistyped)) - - # Attempt to structure config with cattrs. try: - # Structure the config - obj = cattrs.structure(cfg, Config) + obj = cattrs.structure(raw, Config) log.debug( 'Config structured: star.module=%s, interior_energetics.module=%s, ' 'outgas.module=%s, atmos_clim.module=%s, escape.module=%s', @@ -78,8 +60,6 @@ def read_config_object(path: Path | str, *, strict: bool = True) -> Config: obj.atmos_clim.module, obj.escape.module, ) - - # Looks good! Return the structured config object. return obj # Catch validation exceptions @@ -99,12 +79,46 @@ def read_config_object(path: Path | str, *, strict: bool = True) -> Config: ) from None +def read_config_object(path: Path | str) -> Config: + """Read and validate config into Config object. + + Parameters + ---------- + path: + Path to the TOML config file. + + Returns + ------- + Config + The structured configuration. + + Raises + ------ + UnknownConfigKeyError + If the file carries keys the schema cannot accept. + ValueError + If a value fails validation. + """ + + # Read config from TOML file in path as a raw dict. + cfg = read_config(path) + + # Reject unusable keys before structuring, so that a typo is reported as a + # typo rather than as whatever the resulting default happens to break + # further downstream. + orphans, mistyped = find_key_problems(cfg) + if orphans or mistyped: + raise UnknownConfigKeyError(format_orphan_message(orphans, path, mistyped)) + + return structure_config(cfg, path) + + __all__ = [ 'Config', 'UnknownConfigKeyError', 'read_config_object', 'read_config', + 'structure_config', 'find_key_problems', - 'find_orphan_keys', 'format_orphan_message', ] diff --git a/src/proteus/config/orphans.py b/src/proteus/config/orphans.py index b6cbb8bd6..9f1e62c69 100644 --- a/src/proteus/config/orphans.py +++ b/src/proteus/config/orphans.py @@ -35,7 +35,7 @@ def _extract_attrs_class(hint: type) -> type | None: """Return the attrs class from a type hint, unwrapping union types. This is required for handling recursion (nested classes) in the config - schema. It is called by `_collect_orphan_keys` below. + schema. It is called by `find_key_problems` below. Parameters ---------- @@ -84,46 +84,33 @@ def _expects_single_table(hint: type) -> bool: return False -def _collect_orphan_keys(data: dict, cls: type, path: str = '') -> list[str]: - """Recursively collect TOML keys that have no matching field in *cls*. +def find_key_problems( + data: dict, cls: type = Config, path: str = '' +) -> tuple[list[str], list[str]]: + """Recursively collect the keys in *data* that the schema cannot accept. - Parameters - ---------- - data: - Raw TOML sub-dict to inspect. - cls: - attrs-decorated class to compare against. - path: - Dotted key prefix for building human-readable paths in error messages. - - Returns - ------- - list[str] - Dotted key paths (e.g. ``"planet.orphan_field"``) that appear in - *data* but are not declared fields of *cls* or any nested attrs class. - """ - - return _collect_key_problems(data, cls, path)[0] - - -def _collect_key_problems(data: dict, cls: type, path: str = '') -> tuple[list[str], list[str]]: - """Recursively collect the keys in *data* that *cls* cannot accept. + Both kinds of fault leave the file saying one thing and the run doing + another, so both are collected in one walk and reported together. Parameters ---------- data: - Raw TOML sub-dict to inspect. + Raw TOML dict as returned by `tomllib.load`, or a sub-dict of one. cls: - attrs-decorated class to compare against. + attrs-decorated class to compare against. Defaults to the whole Config + schema; the parameter exists so the walk can descend into nested + classes and is not normally passed by callers. path: - Dotted key prefix for building human-readable paths in error messages. + Dotted key prefix for the paths in the returned lists. Carried by the + recursion; callers start at the root and leave it empty. Returns ------- tuple[list[str], list[str]] Two lists of dotted key paths, in file order. The first holds names the schema does not declare. The second holds sections the schema declares - as a table but which the file supplies as something else. + as a table but which the file supplies as something else. Both are + empty for a conforming file. """ # If the class is not an attrs class, then we cannot inspect it. @@ -161,7 +148,7 @@ def _collect_key_problems(data: dict, cls: type, path: str = '') -> tuple[list[s # If this is a dict, then we need to go deeper. if isinstance(value, dict): - sub_orphans, sub_mistyped = _collect_key_problems(value, nested_cls, full_path) + sub_orphans, sub_mistyped = find_key_problems(value, nested_cls, full_path) orphans.extend(sub_orphans) mistyped.extend(sub_mistyped) elif _expects_single_table(hints.get(key)): @@ -175,44 +162,6 @@ def _collect_key_problems(data: dict, cls: type, path: str = '') -> tuple[list[s return orphans, mistyped -def find_orphan_keys(raw_dict: dict) -> list[str]: - """List the keys in *raw_dict* that the Config schema does not define. - - Parameters - ---------- - raw_dict: - Raw TOML dict as returned by `tomllib.load`. - - Returns - ------- - list[str] - Dotted key paths (e.g. ``"planet.orphan_field"``) in file order. - Empty when every key maps onto a Config field. - """ - return _collect_orphan_keys(raw_dict, Config) - - -def find_key_problems(raw_dict: dict) -> tuple[list[str], list[str]]: - """List the keys in *raw_dict* that the Config schema cannot accept. - - Both kinds leave the file saying one thing and the run doing another, so - both are reported together. - - Parameters - ---------- - raw_dict: - Raw TOML dict as returned by `tomllib.load`. - - Returns - ------- - tuple[list[str], list[str]] - Dotted key paths in file order: names the schema does not define, and - sections the schema declares as a table but which the file supplies as - something else. Both are empty for a conforming file. - """ - return _collect_key_problems(raw_dict, Config) - - def format_orphan_message( orphans: list[str], path: Path | str, mistyped: list[str] | None = None ) -> str: diff --git a/src/proteus/proteus.py b/src/proteus/proteus.py index e71fcbad6..fb39fe2ff 100644 --- a/src/proteus/proteus.py +++ b/src/proteus/proteus.py @@ -21,6 +21,7 @@ format_orphan_message, read_config, read_config_object, + structure_config, ) from proteus.utils.constants import noble_gases, vap_list, vol_list from proteus.utils.helper import ( @@ -53,16 +54,20 @@ def __init__(self, *, config_path: Path | str) -> None: # status file under that directory. self.config_path = config_path - # The keys are collected before the config is structured, because a + # The keys are checked before the config is structured, because a # misspelling is usually what makes a value fail validation and is the - # more useful of the two to report. The check re-reads the raw TOML, so - # it applies only when the path resolves to a file: a caller that - # substitutes the loader supplies the parsed config by other means and - # leaves nothing here to re-read. + # more useful of the two to report. This reads the raw TOML itself and + # structures it separately, rather than going through the checked + # loader, so that a refusal can still be recorded under the output + # directory named inside the file. It applies only when the path + # resolves to a file: a caller that substitutes the loader supplies the + # parsed config by other means and leaves nothing here to read. orphans: list[str] = [] mistyped: list[str] = [] + raw: dict | None = None if os.path.isfile(config_path): - orphans, mistyped = find_key_problems(read_config(config_path)) + raw = read_config(config_path) + orphans, mistyped = find_key_problems(raw) orphan_error = ( format_orphan_message(orphans, config_path, mistyped) if orphans or mistyped @@ -70,7 +75,11 @@ def __init__(self, *, config_path: Path | str) -> None: ) try: - self.config = read_config_object(config_path, strict=False) + self.config = ( + structure_config(raw, config_path) + if raw is not None + else read_config_object(config_path) + ) except ValueError as exc: # An unrecognised key is reported first because it is often what # made the rest of the file fail, but the other complaint is kept diff --git a/tests/config/test_config.py b/tests/config/test_config.py index 99296e685..5b70dc8a0 100644 --- a/tests/config/test_config.py +++ b/tests/config/test_config.py @@ -17,7 +17,7 @@ from helpers import PROTEUS_ROOT from proteus import Proteus -from proteus.config import Config, read_config, read_config_object +from proteus.config import Config, read_config, read_config_object, structure_config from proteus.config._config import ( instmethod_dummy, instmethod_evolve, @@ -149,21 +149,22 @@ def test_read_config_object_rejects_a_section_written_as_an_array_of_tables(tmp_ read_config_object(array) assert 'Misdeclared' in str(excinfo.value) - # Without the refusal the run would proceed on 1.0 while the file asks for - # 3.7. Pinning that here shows the check is what stands between the two. - dropped = read_config_object(array, strict=False) + # Structuring without the key check is what the runner does so it can + # resolve the output directory before refusing. Left unchecked it would run + # on 1.0 while the file asks for 3.7, so this pins what the check prevents. + dropped = structure_config(read_config(array), array) assert dropped.planet.mass_tot == pytest.approx(1.0) assert dropped.planet.mass_tot != pytest.approx(3.7) @pytest.mark.unit -def test_read_config_object_without_strict_applies_the_default(tmp_path): - """With strict off the unknown key is dropped and the default takes over. +def test_structure_config_drops_an_unknown_key_and_applies_the_default(tmp_path): + """Structuring without the key check lets the default take over. - This is exactly the outcome strict loading exists to prevent, and it is the - path the runner takes so it can record the failure in a status file before - refusing. Pinning it keeps the escape hatch honest: it tolerates the key, it - does not honour it. + This is exactly the outcome the checked loader exists to prevent, and it is + the path the runner takes so it can resolve the output directory and record + the failure before refusing. Pinning it keeps that step honest: it tolerates + the key, it does not honour it. """ # Three distinct masses so the surviving value identifies its own source: @@ -176,7 +177,7 @@ def _typo(raw): path = _write_variant(tmp_path, 'typo.toml', _typo) - cfg = read_config_object(path, strict=False) + cfg = structure_config(read_config(path), path) assert cfg.planet.mass_tot == pytest.approx(1.75) assert cfg.planet.mass_tot != pytest.approx(2.5) @@ -184,7 +185,7 @@ def _typo(raw): # wholesale rather than just the unknown key would fail here. assert attrs.fields(Planet).mass_tot.default == pytest.approx(1.0) - # Same file, same content: only the parameter decides whether it loads. + # Same file, same content: only the entry point decides whether it loads. with pytest.raises(ValueError, match='planet.mass_total'): read_config_object(path) diff --git a/tests/config/test_orphans.py b/tests/config/test_orphans.py index 5ae8a8966..9f6775426 100644 --- a/tests/config/test_orphans.py +++ b/tests/config/test_orphans.py @@ -10,10 +10,8 @@ import pytest from proteus.config.orphans import ( - _collect_orphan_keys, _extract_attrs_class, find_key_problems, - find_orphan_keys, format_orphan_message, ) @@ -32,6 +30,18 @@ class _Leaf: value: int = 0 +@attrs.define +class _Unresolvable: + """Synthetic schema whose annotation names a class that does not exist. + + Used to drive the branch where `typing.get_type_hints` raises. A schema + that cannot be introspected must still let the walk report the names it can + compare, rather than aborting and letting every key through unchecked. + """ + + field: 'NoSuchClassAnywhere' = None # noqa: F821 + + @attrs.define class _Holder: """Synthetic schema pairing a container of tables with a single table. @@ -108,7 +118,7 @@ def test_orphans_extract_attrs_class_returns_none_for_non_attrs_hints(): Two branches reach the same answer by different routes: a bare scalar has no ``__args__`` and exits early, while a parameterised container does enter the ``__args__`` loop but holds no attrs member. Both must fall through to None, - or ``_collect_orphan_keys`` would recurse into a value the schema never + or ``find_key_problems`` would recurse into a value the schema never declares as a sub-config and report its keys as orphans. """ from proteus.config._planet import Planet @@ -127,23 +137,23 @@ def test_orphans_extract_attrs_class_returns_none_for_non_attrs_hints(): # --------------------------------------------------------------------------- -# _collect_orphan_keys +# find_key_problems, unknown names # --------------------------------------------------------------------------- -def test_orphans_collect_orphan_keys_empty_dict_is_clean(): +def test_orphans_find_key_problems_empty_dict_is_clean(): """An empty raw dict has no orphans; the result must be an empty list.""" from proteus.config._config import Config - result = _collect_orphan_keys({}, Config) + result = find_key_problems({}, Config)[0] assert result == [] # Discrimination: a non-empty invalid dict must NOT produce an empty list; # this verifies the clean path is really empty and not a degenerate always-pass. dirty = {'GHOST': 1} - assert _collect_orphan_keys(dirty, Config) != [] + assert find_key_problems(dirty, Config)[0] != [] -def test_orphans_collect_orphan_keys_valid_top_level_keys(): +def test_orphans_find_key_problems_valid_top_level_keys(): """Known top-level Config fields do not appear in the orphan list. The paired negative adds the same unknown key to that very dict and expects @@ -152,7 +162,7 @@ def test_orphans_collect_orphan_keys_valid_top_level_keys(): from proteus.config._config import Config data = {'star': {}, 'planet': {}, 'orbit': {}, 'config_version': '3.0'} - result = _collect_orphan_keys(data, Config) + result = find_key_problems(data, Config)[0] assert result == [] # Discrimination: an unknown key alongside the very same valid keys is @@ -162,42 +172,42 @@ def test_orphans_collect_orphan_keys_valid_top_level_keys(): # membership; this pins exact-list equality against a mixed dict, so a scan # that reported a known sibling alongside the orphan would fail here. mixed = dict(data, NOT_A_FIELD=1) - assert _collect_orphan_keys(mixed, Config) == ['NOT_A_FIELD'] + assert find_key_problems(mixed, Config)[0] == ['NOT_A_FIELD'] -def test_orphans_collect_orphan_keys_single_top_level_orphan(): +def test_orphans_find_key_problems_single_top_level_orphan(): """A single unknown top-level key is returned with its bare name.""" from proteus.config._config import Config data = {'UNKNOWN_TOP': 'bad'} - result = _collect_orphan_keys(data, Config) + result = find_key_problems(data, Config)[0] assert result == ['UNKNOWN_TOP'] # Discrimination: the known key 'star' must not appear in orphans. data_mixed = {'star': {}, 'UNKNOWN_TOP': 'bad'} - mixed_result = _collect_orphan_keys(data_mixed, Config) + mixed_result = find_key_problems(data_mixed, Config)[0] assert 'star' not in mixed_result assert 'UNKNOWN_TOP' in mixed_result -def test_orphans_collect_orphan_keys_nested_orphan_in_planet(): +def test_orphans_find_key_problems_nested_orphan_in_planet(): """A typo inside the planet section is reported with its dotted path.""" from proteus.config._config import Config data = {'planet': {'mass_tot': 1.0, 'typo_field': 99}} - result = _collect_orphan_keys(data, Config) + result = find_key_problems(data, Config)[0] assert result == ['planet.typo_field'] # Discrimination: the valid key 'mass_tot' must not appear in orphans. assert 'planet.mass_tot' not in result -def test_orphans_collect_orphan_keys_deeply_nested_orphan(): +def test_orphans_find_key_problems_deeply_nested_orphan(): """Orphan keys inside doubly-nested sub-configs are reported with full path.""" from proteus.config._config import Config data = {'star': {'mors': {'age_now': 4.5, 'bad_star_key': 'oops'}}} - result = _collect_orphan_keys(data, Config) + result = find_key_problems(data, Config)[0] assert result == ['star.mors.bad_star_key'] # Orphans at two different depths in a single walk: each dotted path must @@ -211,13 +221,13 @@ def test_orphans_collect_orphan_keys_deeply_nested_orphan(): 'mors': {'age_now': 4.5, 'bad_star_key': 'oops'}, } } - assert sorted(_collect_orphan_keys(two_depths, Config)) == [ + assert sorted(find_key_problems(two_depths, Config)[0]) == [ 'star.NOT_A_STAR_FIELD', 'star.mors.bad_star_key', ] -def test_orphans_collect_orphan_keys_multiple_orphans_all_reported(): +def test_orphans_find_key_problems_multiple_orphans_all_reported(): """All orphan keys are collected; none is silently dropped.""" from proteus.config._config import Config @@ -226,13 +236,13 @@ def test_orphans_collect_orphan_keys_multiple_orphans_all_reported(): 'planet': {'mass_tot': 1.0, 'typo_b': 2}, 'star': {'extra_c': 3}, } - result = _collect_orphan_keys(data, Config) + result = find_key_problems(data, Config)[0] assert set(result) == {'GHOST_A', 'planet.typo_b', 'star.extra_c'} # Discrimination: exactly three orphans; valid keys must not inflate the count. assert len(result) == 3 -def test_orphans_collect_orphan_keys_valid_nested_dict_does_not_raise(): +def test_orphans_find_key_problems_valid_nested_dict_does_not_raise(): """A known nested dict with all valid keys produces an empty orphan list. The paired negative misspells one of those same nested keys and expects the @@ -244,7 +254,7 @@ def test_orphans_collect_orphan_keys_valid_nested_dict_does_not_raise(): 'star': {'module': 'dummy'}, 'planet': {'mass_tot': 1.0}, } - result = _collect_orphan_keys(data, Config) + result = find_key_problems(data, Config)[0] assert result == [] # Discrimination: the walk does descend into these same nested dicts, so a @@ -254,17 +264,17 @@ def test_orphans_collect_orphan_keys_valid_nested_dict_does_not_raise(): # start; this one mutates the dict just asserted clean, so the two results # come from the same input and the empty list cannot be a shape artefact. data['planet']['mass_totl'] = 1.0 - assert _collect_orphan_keys(data, Config) == ['planet.mass_totl'] + assert find_key_problems(data, Config)[0] == ['planet.mass_totl'] -def test_orphans_collect_orphan_keys_non_attrs_type_skips_recursion(): +def test_orphans_find_key_problems_non_attrs_type_skips_recursion(): """A known field with a plain scalar type does not trigger dict recursion.""" from proteus.config._config import Config # config_version is a str field; a dict value for it is unusual but the # validator must not recurse into it (it has no attrs type). data = {'config_version': {'nested': 'should_not_recurse'}} - result = _collect_orphan_keys(data, Config) + result = find_key_problems(data, Config)[0] # 'config_version' itself is a known field, so it should not appear. assert 'config_version' not in result @@ -275,11 +285,11 @@ def test_orphans_collect_orphan_keys_non_attrs_type_skips_recursion(): # --------------------------------------------------------------------------- -# find_orphan_keys +# find_key_problems, nested paths # --------------------------------------------------------------------------- -def test_find_orphan_keys_reports_nested_paths_and_none_for_a_clean_config(): +def test_find_key_problems_names_reports_nested_paths_and_none_for_a_clean_config(): """Unknown keys come back as dotted paths; a schema-conforming config yields none. The nested case is the one that matters: a misspelling two levels down in @@ -293,7 +303,7 @@ def test_find_orphan_keys_reports_nested_paths_and_none_for_a_clean_config(): dirty['planet']['elements']['H_budgets'] = 1.0 dirty['atmosphere'] = {'module': 'agni'} - orphans = find_orphan_keys(dirty) + orphans = find_key_problems(dirty)[0] assert set(orphans) == {'planet.elements.H_budgets', 'atmosphere'} # The correctly spelled sibling sits right next to the typo and must not be @@ -308,7 +318,7 @@ def test_find_orphan_keys_reports_nested_paths_and_none_for_a_clean_config(): # Edge case: the same config without the two injected keys is clean, so the # result above is driven by the injection rather than by the fixture. - assert find_orphan_keys(_MINIMAL_VALID) == [] + assert find_key_problems(_MINIMAL_VALID)[0] == [] # --------------------------------------------------------------------------- @@ -356,7 +366,7 @@ def test_find_key_problems_catches_a_section_that_is_not_a_table(): assert find_key_problems(both) == (['star.typo_field'], ['planet']) -def test_collect_key_problems_allows_repeated_tables_for_a_container_field(): +def test_find_key_problems_allows_repeated_tables_for_a_container_field(): """A field holding several nested classes accepts a list rather than a table. The schema declares no such field today, so this is checked against a @@ -365,21 +375,69 @@ def test_collect_key_problems_allows_repeated_tables_for_a_container_field(): them later would start refusing configurations that are correct, which is a worse failure than the one the refusal exists to prevent. """ - from proteus.config.orphans import _collect_key_problems - # A list against the container field is the intended shape, so neither list # reports it. - assert _collect_key_problems({'many': [{'value': 1}]}, _Holder) == ([], []) + assert find_key_problems({'many': [{'value': 1}]}, _Holder) == ([], []) # Discriminator: the same list against the single-table field beside it is # still refused, so the carve-out is driven by the field's type and is not # a blanket exemption for lists. - assert _collect_key_problems({'one': [{'value': 1}]}, _Holder) == ([], ['one']) + assert find_key_problems({'one': [{'value': 1}]}, _Holder) == ([], ['one']) # A table remains valid for the single-table field, and an unknown name # inside it is still found, so recursion is unaffected. - assert _collect_key_problems({'one': {'value': 1}}, _Holder) == ([], []) - assert _collect_key_problems({'one': {'nope': 1}}, _Holder) == (['one.nope'], []) + assert find_key_problems({'one': {'value': 1}}, _Holder) == ([], []) + assert find_key_problems({'one': {'nope': 1}}, _Holder) == (['one.nope'], []) + + +def test_find_key_problems_catches_a_mistyped_optional_section(): + """An optional section typed as a union is held to the same shape rule. + + Module sections such as ``star.mors`` are declared ``Mors | None``, so the + class is reached through the union branch rather than directly. Without it + the most common optional sections in the schema would accept ``[[mors]]`` + and silently default the whole block. + """ + from copy import deepcopy + + from proteus.config._star import Mors + from proteus.config.orphans import _expects_single_table + + assert _expects_single_table(Mors | None) is True + # Discrimination: a union carrying no attrs member must not demand a table, + # or ordinary optional scalars would be refused. + assert _expects_single_table(float | None) is False + + aot = deepcopy(_MINIMAL_VALID) + aot['star']['mors'] = [{'age_now': 4.5}] + assert find_key_problems(aot) == ([], ['star.mors']) + + # Edge case: the same section as a table is accepted, and an unknown name + # inside it is still reported, so recursion through the union is intact. + good = deepcopy(_MINIMAL_VALID) + good['star']['mors'] = {'age_now': 4.5} + assert find_key_problems(good) == ([], []) + good['star']['mors']['bad_key'] = 1 + assert find_key_problems(good) == (['star.mors.bad_key'], []) + + +def test_find_key_problems_handles_a_schema_it_cannot_introspect(): + """A class that is not attrs, or whose hints fail to resolve, is survivable. + + Neither case should raise. The unintrospectable class contributes nothing, + and the class whose annotations cannot be resolved still has its own field + names compared, so an unknown key beside the broken one is reported rather + than the whole section being waved through. + """ + # Not an attrs class at all: nothing to compare against, nothing reported. + assert find_key_problems({'anything': 1}, str) == ([], []) + + # Annotations that cannot be resolved: the walk logs and carries on using + # the field names, so the known field is accepted and the unknown one is + # still named. + orphans, mistyped = find_key_problems({'field': {}, 'not_a_field': 1}, _Unresolvable) + assert orphans == ['not_a_field'] + assert mistyped == [] def test_find_key_problems_leaves_a_mistyped_section_out_of_the_orphan_list(): @@ -399,7 +457,7 @@ def test_find_key_problems_leaves_a_mistyped_section_out_of_the_orphan_list(): assert orphans == [] # The name-only helper agrees, so the split is a property of the walk and # not of one caller's interpretation. - assert find_orphan_keys(aot) == [] + assert find_key_problems(aot)[0] == [] # --------------------------------------------------------------------------- @@ -496,12 +554,12 @@ def test_orphans_detected_in_raw_dict_from_toml_file(tmp_path): with open(cfg_path, 'rb') as f: raw = tomllib.load(f) - assert find_orphan_keys(raw) == ['planet.TYPO_FIELD'] + assert find_key_problems(raw)[0] == ['planet.TYPO_FIELD'] # Discrimination: the original dummy.toml must be orphan-free. A regression # that always reported a key would fail this second check. with open(dummy_path, 'rb') as f: clean_raw = tomllib.load(f) - assert find_orphan_keys(clean_raw) == [] + assert find_key_problems(clean_raw)[0] == [] def test_orphans_detected_at_top_level_in_toml_file(tmp_path): @@ -523,14 +581,14 @@ def test_orphans_detected_at_top_level_in_toml_file(tmp_path): raw = tomllib.load(f) # The orphan key is detected at the TOP level. - orphans = find_orphan_keys(raw) + orphans = find_key_problems(raw)[0] assert orphans == ['EXTRA_SECTION'] # Discrimination: the same walk over the schema root reports the identical # key, so the public helper is not silently widening the search. from proteus.config._config import Config - assert _collect_orphan_keys(raw, Config) == orphans + assert find_key_problems(raw, Config)[0] == orphans # --------------------------------------------------------------------------- @@ -805,7 +863,7 @@ def test_all_options_phase_boundary_margin_declared_and_resolves(): # The whole reference file stays orphan-free, so the new key has a schema # home and is not one of the silently-discarded orphans. - assert find_orphan_keys(raw) == [] + assert find_key_problems(raw)[0] == [] # End-to-end resolution through the parser yields the same 200.0. Pinning # the exact band is itself the discriminator: it rejects a 0.0 step-cap- From e92317967d999f1823e2700c884f117d94e3459c Mon Sep 17 00:00:00 2001 From: timlichtenberg Date: Fri, 31 Jul 2026 11:36:50 +0200 Subject: [PATCH 3/3] Reword the configuration refusal message and link the reference docs 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. --- src/proteus/config/orphans.py | 15 ++++++--------- tests/config/test_orphans.py | 10 ++++++---- 2 files changed, 12 insertions(+), 13 deletions(-) diff --git a/src/proteus/config/orphans.py b/src/proteus/config/orphans.py index 9f1e62c69..c6556574a 100644 --- a/src/proteus/config/orphans.py +++ b/src/proteus/config/orphans.py @@ -191,14 +191,12 @@ def format_orphan_message( 'Unrecognised configuration key' if single else 'Unrecognised configuration keys' ) subject = 'This key is' if single else 'These keys are' - setting = 'Setting it' if single else 'Setting them' + setting = 'setting it' if single else 'setting them' blocks.append( f'{heading} in {path}:\n' f' {keys}\n' - f' {subject} not part of the configuration schema. {setting} ' - f'has no effect, so the file is refused rather than run on defaults ' - f'that were not asked for. Check for a typo or an outdated option ' - f'name.' + f' {subject} unrecognised, so {setting} will have no effect. ' + f'Check for typos or outdated option names.' ) if mistyped: @@ -214,10 +212,9 @@ def format_orphan_message( f'{heading} in {path}:\n' f' {sections}\n' f' {subject} declared as a table by the schema, but the file gives ' - f'another kind of value. Writing a section as [[name]] rather than ' - f'[name] is the usual cause. As written the whole section is ' - f'discarded and every parameter inside it falls back to its default.' + f'another kind of value. Check for typos or double-brackets.' ) - blocks.append('See input/all_options.toml for the full parameter reference.') + blocks.append('See input/all_options.toml for reference, or read the docs:') + blocks.append('https://proteus-framework.org/PROTEUS/Reference/config/params.html') return '\n'.join(blocks) diff --git a/tests/config/test_orphans.py b/tests/config/test_orphans.py index 9f6775426..bb842a590 100644 --- a/tests/config/test_orphans.py +++ b/tests/config/test_orphans.py @@ -490,13 +490,15 @@ def test_format_orphan_message_quotes_every_key_and_names_the_file(): # that "these keys are not part of the schema" reads as though the file has # more wrong with it than it does. assert 'Unrecognised configuration key in' in single - assert 'This key is not part' in single + assert 'This key is unrecognised' in single + assert 'setting it will have no effect' in single assert 'these keys' not in single.lower() # The many-key message keeps the plural, so the singular above is chosen # from the count rather than applied to every message. assert 'Unrecognised configuration keys in' in msg - assert 'These keys are not part' in msg + assert 'These keys are unrecognised' in msg + assert 'setting them will have no effect' in msg def test_format_orphan_message_reports_mistyped_sections_in_their_own_block(): @@ -514,7 +516,7 @@ def test_format_orphan_message_reports_mistyped_sections_in_their_own_block(): # The unrecognised key comes first: it is the more common mistake, and the # section advice is useless to someone who has neither. assert both.index('Unrecognised') < both.index('Misdeclared') - assert '[[name]]' in both + assert 'double-brackets' in both # The reference line is printed once, not once per block. assert both.count('all_options.toml') == 1 @@ -527,7 +529,7 @@ def test_format_orphan_message_reports_mistyped_sections_in_their_own_block(): # Only keys: symmetrically, no bracket advice appears. keys_only = format_orphan_message(['planet.mass_total'], '/runs/case.toml') assert 'Misdeclared' not in keys_only - assert '[[name]]' not in keys_only + assert 'double-brackets' not in keys_only # ---------------------------------------------------------------------------