refactor: split config resolution from validation (#13) - #19
Merged
rgutzen merged 13 commits intoSep 9, 2026
Conversation
…missing system deps)
Fix the test-time dtype mismatch (Half input vs float32 conv) by making TrainerParams.precision the single source of truth for dtype resolution. - Add dynvision/utils/dtype_policy.py: canonical PRECISION_TO_DTYPE map (stored-parameter semantics), resolve_dtype (warn + float32 default), DtypePolicy dataclass, and shared coordinate_component_dtypes helper. - Route get_effective_dtype_from_precision and DtypeDeviceCoordinator through the shared resolver; replace silent float16 fallbacks with warn + float32. - Delete DataParams.precision and dead ModelParams.target_dtype. - Apply the same coordinate_component_dtypes validator to TestingParams (removing the 'precision' alias that swallowed unscoped precision and caused data to derive float16). - Default dataloaders to float32. Regression tests in tests/params/test_dtype_policy.py.
…-free module Prerequisite for a pydantic-free resolution layer (Lindsay-Lab#13). Both names are re-exported from base_params so existing import paths keep working.
…ce (Lindsay-Lab#13) Introduces ConfigSchema / ConfigSource / ResolvedConfig and a resolve() function that turns labelled parameter sources into per-component dicts without any pydantic or torch dependency.
…indsay-Lab#13) from_cli_and_config now runs three explicit stages: gather (I/O), resolve (pure), validate. build_config_schema() is the adapter that projects the pydantic class graph onto a plain ConfigSchema, and _instantiate_resolved() is the isolated validation half. All private phase helpers are retained as thin delegations, so the existing test call sites and the _handle_unscoped_param / preprocessor override points in Init/Testing/TrainingParams keep working unchanged. composite_params.py: 1107 -> 743 lines.
Updates the parameter-processing developer guide to describe the three stages (gather / resolve / validate), the resolution-layer types, and the alias-shadowing caveat that caused the c10::Half crash. Marks the planning doc implemented and records deviations.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
dynvision/base/coordination.py currently mis-normalizes "bfloat16"/"torch.bfloat16" in map_dtype(), causing explicit bfloat16 targets to fall back to float32.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR implements issue #13 by splitting config resolution (precedence/alias/mode routing over plain dicts) from pydantic validation, introducing a pure resolve(sources, schema) -> ResolvedConfig seam. It also includes the stacked dtype-policy work that centralizes Lightning precision → stored-parameter torch.dtype mapping and aligns data/trainer dtype behavior in both training and testing contexts.
Changes:
- Added a dependency-free resolution layer (
dynvision/params/resolution.py) plus shared provenance primitives (dynvision/params/provenance.py), and rewiredCompositeParamsto run gather → resolve → validate. - Centralized precision→dtype logic in
dynvision/utils/dtype_policy.py, updated params/components to use it, and removed legacy dtype/precision fields/aliases that caused thec10::Halfmismatch. - Added extensive unit tests for resolution/dtype behavior and updated developer docs to reflect the new architecture.
File summaries
| File | Description |
|---|---|
| tests/params/test_dtype_policy.py | Adds regression + unit tests for centralized precision→dtype mapping and coordination behavior. |
| tests/params/test_config_resolution.py | Adds pure-dict resolution tests and enforces “no validation deps” invariant for the resolver. |
| pyproject.toml | Updates scikit-image requirement and comments out ffcv dependency. |
| dynvision/utils/torch_utils.py | Makes get_effective_dtype_from_precision delegate to the central dtype policy and return torch.dtype. |
| dynvision/utils/dtype_policy.py | Introduces the canonical precision→dtype map + policy helpers + shared dtype coordination routine. |
| dynvision/utils/init.py | Re-exports dtype policy symbols from dynvision.utils. |
| dynvision/params/training_params.py | Uses shared dtype-coordination helper during validation. |
| dynvision/params/trainer_params.py | Stores/returns effective dtype as torch.dtype derived via the shared resolver. |
| dynvision/params/testing_params.py | Removes the problematic precision alias; adds dtype coordination validator and coordinated-dtype accessor. |
| dynvision/params/resolution.py | New pure resolution implementation (alias/scope/source precedence, provenance propagation, preprocessors). |
| dynvision/params/provenance.py | New dependency-free home for ProvenanceRecord and ParamsDict. |
| dynvision/params/model_params.py | Removes target_dtype field. |
| dynvision/params/data_params.py | Removes precision field/validator and legacy dtype derivation; adjusts dtype defaults/fallbacks. |
| dynvision/params/composite_params.py | Introduces schema adapter + integrates resolution layer; keeps legacy helpers as delegating wrappers. |
| dynvision/params/base_params.py | Re-exports provenance primitives from the new dependency-free module. |
| dynvision/data/ffcv_dataloader.py | Changes default dtype from float16 to float32. |
| dynvision/data/dataloader.py | Changes default dtype from float16 to float32. |
| dynvision/configs/config_defaults.yaml | Removes target_dtype default entry. |
| dynvision/base/coordination.py | Routes dtype mapping through resolve_dtype and updates float16 fallbacks to float32. |
| docs/development/planning/dtype-handling-refactor.md | Adds planning/design doc for the dtype-policy refactor. |
| docs/development/planning/config-resolution-refactor.md | Adds planning/design doc for the resolution/validation split and recorded deviations. |
| docs/development/guides/parameter-processing.md | Updates developer guide to describe gather → resolve → validate and the new resolver interfaces. |
Review details
- Files reviewed: 22/22 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ional ffcv import - map_dtype() in coordination.py stripped the substring "float" from all spellings, turning "bfloat16"/"torch.bfloat16" into "b16", which resolve_dtype() couldn't match and silently fell back to float32. Now bfloat16 spellings normalize to "bf16" explicitly before the "float" strip is applied to the other spellings. - DataParams.effective_dtype did a string-keyed dict lookup on self.dtype, but validate_dtype already converts self.dtype to torch.dtype, so the lookup always missed and silently returned torch.float32 regardless of the configured dtype. Now returns self.dtype directly. - data_params.py and transforms.py had unconditional top-level `from ffcv.loader import OrderOption` / `import ffcv.transforms`, but ffcv is commented out of pyproject.toml (no wheel / missing system deps). This broke every test module that imports dynvision.params or dynvision.data.transforms in environments without ffcv installed (8 CI import errors). Both imports are now optional: data_params.py falls back to a local OrderOption-mirroring IntEnum, and transforms.py raises a clear ImportError only if backend="ffcv" is actually requested without ffcv installed. Adds regression tests for both dtype bugs and verifies the ffcv-optional import path by blocking ffcv at runtime.
ffcv is an optional dependency (commented out of pyproject.toml — no wheel / missing system deps), but tests/data/test_transforms.py had no guard for the tests exercising backend="ffcv". After making the ffcv imports in dynvision/data/transforms.py and dynvision/params/data_params.py optional, these tests started failing loudly in CI with the new, correct ImportError instead of the previous collection-time ModuleNotFoundError that masked the whole test module. Add a requires_ffcv skip marker (based on actual ffcv importability) to the three tests that genuinely need a working ffcv install: test_parse_bare_module_name_ffcv, test_get_ffcv_train_transforms, test_ffcv_mnist_train_workflow. Verified clean pass+skip both with ffcv present and with it blocked at import time.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
merge_mode_sections() currently contradicts its “input is not modified” contract by using a shallow copy, which can mutate callers’ nested config dicts during mode merges.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
dynvision/params/resolution.py:303
- merge_mode_sections() says the input config is not modified, but
merged = dict(config)is a shallow copy. Ifconfigcontains nested dicts outside the mode block (e.g.,trainer: {…}), the subsequent deep_merge() will mutate those shared nested dict objects in the caller’s input mapping.
- Files reviewed: 26/26 changed files
- Comments generated: 2
- Review effort level: Lite
Comment on lines
220
to
+224
| def effective_dtype(self) -> Optional[torch.dtype]: | ||
| """Get effective dtype without optimization (to avoid circular dependency).""" | ||
| # Convert string dtype to torch dtype if specified | ||
| if self.dtype is not None: | ||
| dtype_map = { | ||
| "float16": torch.float16, | ||
| "float32": torch.float32, | ||
| "float64": torch.float64, | ||
| "int8": torch.int8, | ||
| "int16": torch.int16, | ||
| "int32": torch.int32, | ||
| "int64": torch.int64, | ||
| } | ||
| return dtype_map.get(self.dtype, torch.float16) | ||
|
|
||
| return None | ||
| # validate_dtype already normalizes self.dtype to a torch.dtype (or | ||
| # None), so no further string-keyed lookup is needed here. | ||
| return self.dtype |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Collaborator
Author
|
@copilot resolve the merge conflicts in this pull request |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new resolver currently (1) can leak composite provenance entries for keys not present in the composite base when unscoped_handler routes keys only into components, and (2) merge_mode_sections advertises non-mutation but can mutate nested dicts due to shallow copying.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Lite
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It refactors core configuration/validation behavior across multiple layers (including precedence and provenance plumbing), so despite strong tests it warrants final human review given its system-wide impact.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
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
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.
Summary
Introduces a pure, testable config-resolution layer that operates on plain dicts
before pydantic validation. Fixes issue #13.
Changes
New module:
dynvision/params/resolution.py(670 lines) with a singleinterface:
resolve(sources, schema) -> ResolvedConfig. Pure: no pydantic, notorch, no external dynvision imports except the equally dependency-free
provenance.py.Adapter:
CompositeParams.build_config_schema()projects the pydantic classgraph into plain-data
ConfigSchema— component fields, aliases, mode name,preprocessors, unscoped handler. The only place that reads
model_fields.Three explicit stages in
from_cli_and_config: gather (I/O) → resolve (pure)→ validate.
_instantiate_resolved()is the isolated validation half.Extracted
ProvenanceRecord/ParamsDictinto a dependency-freeprovenance.pyso the resolution layer can use them. Re-exported frombase_paramsto keep every import path working.Line count:
composite_params.pyshrank from 1,107 → 743 lines.Resolution logic is now in one module instead of scattered across four.
Regression: the c10::Half crash
The alias
precision → trainer.precisionwas deleted before other components sawit, so
DataParams.precisionsilently stayedNoneand mismatched its dtype vs.the model's. No test could reach that intermediate state because the only way to
inspect the dict was to construct a fully valid params object.
Now
test_alias_diverts_unscoped_key_away_from_sibling_componentsdirectly assertsthis shape on a plain dict, without torch or a dataset.
Tests
ConfigSchema,ConfigSource,resolve(), and the helperfunctions. All use plain dicts; all pass without pydantic or torch.
test_resolution_layer_has_no_validation_dependencies)fails the build if anyone reintroduces a pydantic/torch import.
ffcvfailures.require zero changes.
Stacked on #18
This branch was rebased onto
refactor/dtype-policy(PR #18) because issue #13explicitly generalizes the pattern that branch established, and the motivating
bug's fix lives there. Should merge after #18.
Outstanding: mkdocs build
mkdocs is not installed in the
dynvisionconda env. Code structure was verifiedmanually (code-fence parity, referenced files exist, planning docs covered by
not_in_navrule), and the doc's worked example was executed to verify the output.Actual build should be confirmed in CI.
Related issues
refactor: centralize dtype handling via DtypePolicy)docs/development/planning/config-resolution-refactor.mdfor the design anddeviations.