Auto3DOptions becomes pydantic, with one unset sentinel (item 4, part 1 of 2) - #160
Merged
Conversation
INCOMPLETE -- 20 tests fail, all of them asserting the behavior this commit
deliberately changes. Committed to make the work durable, not because it is
done. Do not merge.
Item 4, first two of four steps.
**Pydantic.** `Auto3DOptions` is a `BaseModel` with `extra="forbid"`, so an
unrecognized key is an error on the Python API as it already was on the CLI.
`__post_init__` became a `model_validator(mode="after")` running the same two
shared checkers over the same two tables, so no rule moved when the class did.
Two things needed care, and both would have failed silently:
- `dataclasses.replace(config, ...)` re-runs validation; the obvious pydantic
translation, `model_copy(update=...)`, does not. A straight swap would have
made every copied config unchecked, and the copy is only taken on the way
into the optimizer, where a bad value shows up as a bad run rather than an
error. `Auto3DOptions.replace()` goes back through the constructor instead,
and `test_replace_revalidates_rather_than_copying_blindly` pins it --
including asserting that `model_copy` really is the variant that would have
let it through.
- Pydantic type-checks *before* the model validator, so a wrong type raised
`pydantic.ValidationError` while a wrong value raised `ConfigurationError`:
two exception types from one constructor for one kind of mistake. The CLI's
`handle_error` maps the first to exit 1 "unexpected error" and the second to
exit 2 with a hint. `__init__` now translates, so every construction failure
is a `ConfigurationError` carrying pydantic's own field-and-value message.
**One sentinel.** `k` and `window` are `int | None` / `float | None`, defaulting
to `None`. `False` meant "unset" on these two only; `memory` and `max_confs`
already used `None`, and the CLI schema used `None` for all four -- so a
translation function existed purely to convert between the two conventions on
the way across. It is deleted. `k=False` is now a `ConfigurationError` (`False`
coerces to 0, below the declared floor of 1) rather than a silent "not
specified".
Still to do: delete `CLIConfig` and repoint `build_cli_config` /
`load_yaml_config` / `merge_configs`; update the 20 tests that assert the old
sentinel; retire the parity suite; CHANGELOG.
…d ones STILL INCOMPLETE -- 14 tests fail, all of them asserting the two-sentinel contract this change replaces, and 11 of the 14 live in the two files the `CLIConfig` deletion will rewrite. Do not merge. Continues 506ccb2. Suite 1766 -> 1772 passing, 20 -> 14 failing. `bool` is an `int` subclass, so pydantic silently turned `k=False` into `0` and `k=True` into `1`. Left alone that replaced two deliberate guards with confusing outcomes: `k=False` -- which meant "not specified" until this branch -- reported `k must be >= 1, got 0`, complaining about a value the caller never wrote, and `k=True` quietly became "one conformer", a meaning nobody assigned it. A `mode="before"` validator on the four sentinel fields now refuses bools and says what to write instead. `mode="before"` is what makes it reachable at all: the `mode="after"` validator only ever sees the coerced integer. `CLIConfig._false_means_unset` is deleted for the same reason. It existed to map `False` to `None` so the two classes agreed while they disagreed about the sentinel; both spell it `None` now, so the translation had become a divergence rather than a bridge -- `CLIConfig(k=False)` was still being accepted while `Auto3DOptions(k=False)` refused it. `docs/legacy-v2/parameters.yaml` shipped `window: False` and is updated. Worth noting because nothing about the source change points at it: it is an example file, and the test that would have caught a broken example (`test_shipped_parameters_yaml_is_complete`) checks the *other* yaml. Numeric strings are now accepted where they were previously refused -- `threshold="0.3"` parses to 0.3 rather than raising. That is pydantic coercion and it is wanted: YAML hands every scalar over as text, so refusing it would refuse valid config files. `threshold="not-a-number"` still raises ConfigurationError, and the test now asserts both halves. Remaining: delete `CLIConfig` (needs a decision -- its engine-name validator calls `resolve_engine_name` from `Auto3D.models`, so moving it onto `Auto3DOptions` would recreate the L0 -> L2 upward edge item 3 just removed; validating at the CLI boundary instead is the likely answer), retire the parity suite, CHANGELOG.
Completes the first half of item 4. Suite: 1786 passed, 0 failed. The fourteen remaining failures all asserted the two-sentinel world. They are rewritten to the new contract rather than deleted, because `CLIConfig` still exists until the second half lands and drift between the two classes is a live bug class until then -- this branch already produced one, when `_false_means_unset` quietly turned from a bridge into a divergence and left `CLIConfig(k=False)` accepted while `Auto3DOptions(k=False)` refused it. So `test_sentinel_scope_agrees_across_entry_points` now asserts the *new* agreement -- both classes take None, both refuse False -- rather than dropping the False half, and the round-trip forwarding test asserts the unselected selector arrives as None on both sides instead of being translated. `_auto3d_option_fields` reads `model_fields` instead of `dataclasses.fields`; same authoritative schema, and `.default` means the same thing on the FieldInfo it returns. Checked the shipped YAML by hand rather than trusting the unit tests, because the bool guard has to be scoped to exactly the four sentinel fields and both example files carry ordinary bools. `auto3d config validate` exits 0 on `parameters.yaml` (which sets `enumerate_tautomer: False`) and on `docs/legacy-v2/parameters.yaml` (whose `window: False` was updated to None with the source change). CHANGELOG records the three breaks: keyword-only construction, False refused as a sentinel, and every construction failure arriving as ConfigurationError.
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. First half of item 4.
CLIConfigstill exists; collapsing the twoclasses into one is part 2, described at the bottom.
Three breaks, all deliberate
Auto3DOptions("in.smi", k=5)Auto3DOptions(path="in.smi", k=5)Auto3DOptions(k=False)— "not specified"Auto3DOptions(k=None), or omitdataclasses.replace(config, k=1)config.replace(k=1)Falseis refused onk/window/memory/max_confsrather than coerced.boolis anintsubclass, so accepting it reportsk must be >= 1, got 0— abound the caller never went near. The error names
Noneas the replacement.memoryandmax_confsalready meantNone, so this makes all four agree,which is what let the CLI schema's
False→Nonetranslation be deleted.Every construction failure is now a
ConfigurationError— including a wrongtype and an unknown key, which previously raised
pydantic.ValidationErrorand
TypeError. The CLI mapsConfigurationErrorto exit 2 with a hint andeverything else to exit 1 as an unexpected error, so one kind of mistake now
produces one exit code from every entry point.
The two things that would have failed silently
replacehad to keep validating.dataclasses.replacere-runs validators;the obvious pydantic translation,
model_copy(update=...), does not. A straightswap would have made every copied config unchecked — and the copy is taken on
the way into the optimizer, where a bad value surfaces as a bad run rather than
an error.
Auto3DOptions.replace()goes back through the constructor;test_replace_revalidates_rather_than_copying_blindlypins it, and also assertsthat
model_copyreally is the variant that would have let it through.The bool guard needed
mode="before". Pydantic coerces before themode="after"validator runs, so by the time the bounds check seesk=Falseitis already
0.On the tests
The 14 failing tests are rewritten to the new contract, not deleted. While
both classes exist, drift between them is a live bug class — this branch
produced one, when
_false_means_unsetquietly became a divergence and leftCLIConfig(k=False)accepted whileAuto3DOptions(k=False)refused it. Sotest_sentinel_scope_agrees_across_entry_pointsnow asserts the new agreement(both take
None, both refuseFalse) rather than dropping theFalsehalf.Verified by hand, not only by unit test
The bool guard must be scoped to exactly the four sentinel fields, and both
shipped example configs carry ordinary bools.
auto3d config validateexits 0on
parameters.yaml(which setsenumerate_tautomer: False) and ondocs/legacy-v2/parameters.yaml— whosewindow: Falseis updated toNonehere, since the source change would otherwise have broken an in-repo example
with nothing pointing at it.
Suite: 1786 passed, 1 skipped, 70 deselected.
Part 2, not in this PR
Delete
CLIConfigand retire the 708-line parity suite. It needs one designdecision rather than a transplant:
CLIConfig._validate_enginecallsresolve_engine_name, which lives inAuto3D.models. Moving it ontoAuto3DOptionswould recreate the L0 → L2 upward edge #159 removed, and wouldput a possibly-network-touching call on every construction — including the
pickled reconstruction inside every spawned worker. Engine-name resolution
stays at the CLI boundary; the config validates values, not resolvability.