Fix just() conversion for container fields of dataclass types (#858)#875
Open
martinez-hub wants to merge 1 commit into
Open
Conversation
`just` failed to convert a dataclass to a config when one of its fields was annotated with a container of a dataclass type (e.g. `dict[str, Inner]` or `list[Inner]`). The field's values were converted to `Builds_Inner` configs while the field's annotation retained the original element type, which OmegaConf's type-checking rejected with a `ValidationError` during `to_yaml`. `_retain_type_info` now broadens such a field's annotation to `Any` when the value is a container holding a structured config. `default_factory` values are resolved so mutable container defaults can be inspected. Closes mit-ll-responsible-ai#858
Author
|
The failing All other checks pass, including |
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
Fixes #858.
justfailed to convert a dataclass to a config when one of its fields is annotated with a container of a dataclass type (e.g.dict[str, Inner]orlist[Inner]).Root cause
justrecursively converts the dataclass values intoBuilds_Innerconfigs, but the field's annotation keeps the original element type (dict[str, Inner]). OmegaConf type-checks container elements before instantiation and rejects theBuilds_Innerconfig since it isn't a subclass ofInner._retain_type_infoalready broadens a field's annotation toAnywhen its value is a structured config — but it only inspected the top-level value, not configs nested inside a container. A second gap: mutable container defaults are stored viadefault_factory, so the value reaching that check wasMISSING.Fix
_value_contains_builds(value)— detects a structured config nested (possibly deeply) inside a list/tuple/mapping._resolve_field_default(field)— resolves a field'sdefault_factoryso the real container value can be inspected._retain_type_infonow broadens the annotation toAnywhen the value is a container holding a structured config.Broadening to
Anyonly loosens OmegaConf validation — it never tightens an annotation, so previously-working configs are unaffected.Tests
Added to
tests/test_dataclass_conversion.py:test_just_on_container_field_of_typed_dataclasses—dict[str, Inner],list[Inner], and nesteddict[str, list[Inner]], each round-tripped throughto_yaml→instantiate.Verification
test_launch/test_with_hydra_submitit/test_third_partyexcluded — they run in CI).pyrightbasic: 0 errors on the changed source.ruff format --check,ruff check, andautoflakeall clean (ruff 0.14.1).0.16.1section.