Fix dot-notation read on $@ref-backed ConfigParser proxies#8994
Open
VenkateswarluNagineni wants to merge 1 commit into
Open
Fix dot-notation read on $@ref-backed ConfigParser proxies#8994VenkateswarluNagineni wants to merge 1 commit into
VenkateswarluNagineni wants to merge 1 commit into
Conversation
_ConfigProxy.__getattr__ fell back to getattr(self._value, key) when the chained config id was absent from the resolver. For a proxy backed by a $@ref the children have no ids of their own, so parser.alias.x looked up "x" as a dict *attribute* and raised AttributeError, even though parser.alias["x"] resolved it and returned the value. Resolve a key present in the underlying container the same way __getitem__ already does, so dot- and bracket-notation agree on ref-backed proxies and config keys keep precedence over dict methods, as documented. Keys absent from the container still fall back to the container's own attributes, so .keys()/.items() are unaffected. Signed-off-by: VenkateswarluNagineni <venkates2002@tamu.edu>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughUpdated Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Description
_ConfigProxy(added in #8858) resolves a dotted key by chaining toget_parsed_content, and falls back to the underlying container when the chained id is not in the resolver:A proxy backed by a
$@refwraps the parsed value of the referenced node, but that node's children have no ids of their own, soalias::xis absent from the resolver. The fallback then looksxup as a dict attribute rather than a key, and dot-notation fails on a value that bracket-notation returns happily:This also affects chained refs (
"alias": "$@mid","mid": "$@target").Ref-backed proxies are already treated as first-class elsewhere:
_backing_id()resolves the full$@refchain for writes, andtest_ref_backed_proxy_write_throughcoversparser.alias["x"]reads and writes. The dot-notation read is the one path that was not covered, and it diverges. It also contradicts the documented precedence rule on the class ("Config keys take precedence overdict/listattributes and methods") — herexis a key of the aliased node, but the dict attribute lookup wins and raises.Proposed changes
Make
__getattr__'s fallback mirror the one__getitem__already uses: if the chained id is absent but the key exists in the underlying container, returnself._value[key]. Keys that are not in the container still fall through togetattr, so container methods (.keys(),.items(), …) are unaffected, as is the existing "config key shadows a same-named dict method" behaviour.The change is confined to the
except KeyErrorfallback, so any id that resolves today keeps resolving through_chainexactly as before — no behaviour change for non-ref proxies.How did you test it?
test_ref_backed_proxy_attribute_readandtest_chained_ref_backed_proxy_attribute_readnext to the existing ref write-through tests. Both fail without the source change (AttributeError) and pass with it; they assertparser.alias.x == parser.alias["x"], and that.keys()still resolves.tests/bundle/test_config_parser.py: 34 passed before, 36 passed after (2 new), no regressions.tests/bundle/suite: identical results before and after the change (the only failures are pre-existing network-dependenttest_bundle_downloadcases, unchanged by this PR).ruff check/ruff format --check(repo-pinned 0.15.20),black,isortclean on both files;mypy monai/bundle/config_parser.pyreports the same single pre-existingyaml.safe_dumperror asdev, no new ones.Notes for the reviewer
The fallback returns the raw value, matching
__getitem__'s fallback rather than wrapping it in a new proxy — this keeps the two notations exactly consistent and the change minimal. Happy to wrap the result in_wrap_parsedinstead if you'd prefer deeper dot-chaining through refs, though that would make dot- and bracket-notation diverge again in the other direction.Types of changes
./runtests.sh -f -u --net --coverage../runtests.sh --quick --unittests --disttests. (Ran the affected suites directly withpyteston Windows, plusruff/black/isort/mypy, rather thanruntests.sh; details above.)make htmlcommand in thedocs/folder.