Fix _ensure_container dropping flags when target is already a config (#580)#1307
Open
jbbqqf wants to merge 1 commit into
Open
Fix _ensure_container dropping flags when target is already a config (#580)#1307jbbqqf wants to merge 1 commit into
jbbqqf wants to merge 1 commit into
Conversation
…mry#580) When `_ensure_container` was invoked with a pre-built `DictConfig` or `ListConfig`, both the primitive-container and structured-config branches were skipped and the function returned the input unchanged — silently discarding any `flags` argument the caller had passed. This contradicted the signature, which advertises `flags` as a general post-condition. Apply the flags explicitly via `_set_flag` in that path. Adds a parametrized regression test covering dict / list / DictConfig / ListConfig inputs so the contract is exercised uniformly.
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
omegaconf._utils._ensure_containersilently dropped itsflagsargument when the input was already aDictConfigorListConfig. The first two branches construct fresh containers viaOmegaConf.create(..., flags=flags)/OmegaConf.structured(..., flags=flags), but the third branch (already-a-config) returned the target unchanged. This PR applies the flags in that path so the contract is uniform.Fixes #580.
Context
Issue #580 (filed by @Jasha10) shows the divergence:
_ensure_container({...}, flags={"my_flag": True})→ flag set_ensure_container(DataclassUser, flags={"my_flag": True})→ flag set_ensure_container(DictConfig({...}), flags={"my_flag": True})→ flag silently droppedThe function is internal but its
flags: Optional[Dict[str, bool]]parameter advertises a single post-condition; this fix makes the four input shapes behave consistently.Changes
omegaconf/_utils.py— split thenot OmegaConf.is_config(target)branch intoelif OmegaConf.is_config(target): apply flags+else: raise ValueError. Inline comment points at the issue and explains why the explicit_set_flagloop is needed (the primitive/structured branches never run for already-built configs).tests/test_utils.py— new parametrized regressiontest_ensure_container_applies_flagscoveringdict,list,DictConfig,ListConfiginputs.news/580.bugfix— towncrier entry.Reproduce BEFORE/AFTER yourself (copy-paste)
What I ran locally
pytest tests/test_utils.py::test_ensure_container_applies_flags -v→ 4 passed on the branch, 2 failed onorigin/main(thedictconfig/listconfigparametrizations).pytest tests/test_utils.py tests/test_create.py tests/test_basic_ops_dict.py tests/test_basic_ops_list.py -q→ 1607 passed, 1 skipped on the branch.Edge cases tested
{"name": "bond", "age": 7}OmegaConf.create(..., flags=...)(unchanged path)test_ensure_container_applies_flags[dict][1, 2, 3]OmegaConf.create(..., flags=...)(unchanged path)test_ensure_container_applies_flags[list]DictConfig(content={"name": "bond", "age": 7})_set_flaglooptest_ensure_container_applies_flags[dictconfig](was failing onmain)ListConfig(content=[1, 2, 3])_set_flaglooptest_ensure_container_applies_flags[listconfig](was failing onmain)"abc"ValueErrortest_ensure_container_raises_ValueErrorstill passesRisk / blast radius
Narrow. All current internal callers of
_ensure_container(omegaconf/omegaconf.pylines 430, 472, 1112, 1165) invoke it withoutflags=, so the new branch is only entered when an external user (or future caller) explicitly passes flags with a pre-built config — exactly the scenario that was silently broken. Mutation is in-place, matching how_set_flagworks elsewhere; no new copy semantics.Release note
PR drafted with assistance from Claude Code. The change was reviewed manually against omry/omegaconf's source and the issue body. The reproducer block above was used during development; it is the same one a reviewer can paste verbatim.