One configuration class: delete CLIConfig (item 4, part 2 of 2) - #161
Merged
Conversation
INCOMPLETE -- tests/test_cli_config_schema.py still has failures. Committed to
make the work durable. Do not merge.
Item 4, part 2. `CLIConfig` is gone (188 lines) and `build_cli_config`,
`load_yaml_config` and `merge_configs` return `Auto3DOptions`. The parity suite
that existed to keep two classes in step is retired: 3 classes from
test_config_parity.py and 7 definitions from test_cli_config_schema.py whose
whole subject was "the two agree". The behavioural tests beside them are
repointed, not deleted -- gpu_idx parsing, engine rejection, merge substitution
and the shipped-YAML checks all still mean something with one class.
Four capabilities had to move rather than vanish:
- `parse_gpu_idx` and the `Path` -> `str` coercion are field validators on
`Auto3DOptions`; both are pure parsing with no dependency on the model layer.
- Engine-name canonical case (`ani2x` -> `ANI2x`) is a table in config.py,
applied in the model validator. It used to be applied on the way across.
- Engine-name *resolution* deliberately did NOT move onto the model, and this
is the design decision of this PR. `resolve_engine_name` lives in
`Auto3D.models`, so a validator would point the foundation layer at the
engine layer -- the edge #159 removed -- and would run on every
construction, including the pickled reconstruction inside each spawned
worker, where resolving a registry name can reach the registry. It runs at
the CLI boundary in `build_cli_config` instead, which is where its value
was: a typo refused while the user is still looking at their terminal.
- `to_auto3d_options`'s refusal of a settings-only config is now
`require_input_path()`, called by the three entry points that need a
runnable config.
The `Literal["rdkit", "oechem"]` annotations died with CLIConfig, so mypy loses
that visibility; `ENGINE_CHOICES` plus the model validator is the surviving
guard, and `test_engine_choices_table_matches_cliconfig_literals` is deleted
because it asserted the annotations rather than the behaviour.
Two tests were over-deleted in the process and restored:
`test_shipped_parameters_yaml_is_complete` and
`test_config_init_tables_only_name_real_options` merely *used* the parity
helper; they check the shipped YAML and the `config init` tables name only real
options, which is worth keeping. `_auto3d_option_fields` survives for them,
reading `model_fields`.
Remaining: the engine case/rejection tests in test_cli_config_schema.py, a full
suite run, and CHANGELOG.
BREAKING CHANGE: `Auto3D.cli.config_schema.CLIConfig` is removed, along with `to_auto3d_options`. `build_cli_config`, `load_yaml_config` and `merge_configs` return `Auto3DOptions`. Completes item 4. `CLIConfig` declared 27 of `Auto3DOptions`' 28 fields a second time, and a 708-line parity suite existed to keep the two in step -- its existence was the symptom, so it goes with the cause. What moved onto the surviving class, because it was capability rather than duplication: `Path` -> `str` coercion for `path`, `gpu_idx` parsing from `"0"` / `"0,1"` / `[0, 1]`, and canonical case for the three built-in engine names (`ani2x` -> `ANI2x`), applied at construction now rather than on the way across. Engine-name *resolution* deliberately stayed off the model, and that is the design decision here rather than an omission. `resolve_engine_name` lives in `Auto3D.models`, so a validator on `Auto3DOptions` would point the foundation layer at the engine layer -- the edge #159 removed, and one tests/test_layer_boundaries.py would refuse -- and it would run a registry lookup on every construction, including the pickled reconstruction inside each spawned worker. It runs in `build_cli_config`, which is where its value always was: refusing a typo while the user is still looking at their terminal. `check_valid_configuration` checks it again on the run path. The config validates values, not resolvability. `to_auto3d_options`'s other job -- refusing a settings-only config where a runnable one is needed -- is now `require_input_path()`, called by name at the three entry points that need one, rather than riding along inside a conversion. Tests: the parity classes are retired, the behavioural ones repointed. Two were deleted by mistake in the process and restored (`test_shipped_parameters_yaml_ is_complete` and `test_config_init_tables_only_name_real_options` merely *used* the parity helper; they check that the shipped YAML and the `config init` tables name only real options, which is worth keeping without a second class). One loss, stated rather than discovered later: `CLIConfig`'s `Literal["rdkit", "oechem"]` annotations are gone, so mypy no longer sees those as types. `ENGINE_CHOICES` plus the model validator is the surviving guard -- weaker statically, stronger at runtime, since it now applies on every entry point instead of only where a type checker ran. Suite: 1747 passed, 1 skipped, 70 deselected.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Breaking. Completes item 4.
Auto3D.cli.config_schema.CLIConfigis removed,along with
to_auto3d_options.CLIConfigdeclared 27 ofAuto3DOptions' 28 fields a second time, and a708-line parity suite existed to keep the two in step. Its existence was the
symptom, so it goes with the cause.
CLIConfig(path=Path("in.smi"), k=1)build_cli_config(path="in.smi", k=1)config.to_auto3d_options()Auto3DOptionsrequire_input_path(config)What moved, because it was capability rather than duplication
Auto3DOptionsgains thePath→strcoercion forpath,gpu_idxparsingfrom
"0"/"0,1"/[0, 1], and canonical case for the three built-in enginenames (
ani2x→ANI2x) — applied at construction now instead of on the wayacross.
The design decision
Engine-name resolution deliberately stayed off the model. Two independent
reasons, either sufficient:
resolve_engine_namelives inAuto3D.models, so a validator onAuto3DOptionswould point the foundation layer at the engine layer — the edge#159 removed, and one
tests/test_layer_boundaries.pywould refuse outright.It would also run a registry lookup on every construction, including the
pickled reconstruction inside each spawned worker.
So it runs in
build_cli_config, which is where its value always was: refusinga typo while the user is still looking at their terminal.
check_valid_configurationchecks it again on the run path. In one line: theconfig validates values, not resolvability.
One loss, stated rather than discovered later
CLIConfig'sLiteral["rdkit", "oechem"]annotations are gone, so mypy nolonger sees those as types.
ENGINE_CHOICESplus the model validator is thesurviving guard — weaker statically, stronger at runtime, since it now applies
on every entry point instead of only where a type checker was run.
On the tests
Parity classes retired; behavioural ones repointed, not deleted —
gpu_idxparsing, engine rejection, merge substitution and the shipped-YAML checks all
still mean something with one class.
Two were deleted by mistake and restored:
test_shipped_parameters_yaml_is_completeandtest_config_init_tables_only_name_real_optionsmerely used the parity helper.They check that the shipped YAML and the
config inittables name only realoptions — worth keeping with or without a second class.
Suite: 1747 passed, 1 skipped, 70 deselected.