Skip to content

Commit 56f0bd9

Browse files
Fix dot-notation read on $@ref-backed ConfigParser proxies (#8994)
### Description `_ConfigProxy` (added in #8858) resolves a dotted key by chaining to `get_parsed_content`, and falls back to the underlying container when the chained id is not in the resolver: ```python try: return self._chain(key) except KeyError: return getattr(self._value, key) ``` A proxy backed by a `$@ref` wraps the *parsed* value of the referenced node, but that node's children have no ids of their own, so `alias::x` is absent from the resolver. The fallback then looks `x` up as a **dict attribute** rather than a key, and dot-notation fails on a value that bracket-notation returns happily: ```python parser = ConfigParser(config={"target": {"x": 1, "y": 2}, "alias": "$@target"}, globals={"monai": "monai"}) parser.alias["x"] # -> 1 parser.alias.x # -> AttributeError: 'dict' object has no attribute 'x' ``` This also affects chained refs (`"alias": "$@mid"`, `"mid": "$@target"`). Ref-backed proxies are already treated as first-class elsewhere: `_backing_id()` resolves the full `$@ref` chain for writes, and `test_ref_backed_proxy_write_through` covers `parser.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 over `dict`/`list` attributes and methods") — here `x` *is* 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, return `self._value[key]`. Keys that are *not* in the container still fall through to `getattr`, 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 KeyError` fallback, so any id that resolves today keeps resolving through `_chain` exactly as before — no behaviour change for non-ref proxies. ### How did you test it? - Added `test_ref_backed_proxy_attribute_read` and `test_chained_ref_backed_proxy_attribute_read` next to the existing ref write-through tests. Both **fail without the source change** (`AttributeError`) and pass with it; they assert `parser.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. - Whole `tests/bundle/` suite: identical results before and after the change (the only failures are pre-existing network-dependent `test_bundle_download` cases, unchanged by this PR). - `ruff check` / `ruff format --check` (repo-pinned 0.15.20), `black`, `isort` clean on both files; `mypy monai/bundle/config_parser.py` reports the same single pre-existing `yaml.safe_dump` error as `dev`, 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_parsed` instead 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 <!--- Put an `x` in all the boxes that apply, and remove the not applicable items --> - [x] Non-breaking change (fix or new feature that would not break existing functionality). - [ ] Breaking change (fix or new feature that would cause existing functionality to change). - [x] New tests added to cover the changes. - [ ] Integration tests passed locally by running `./runtests.sh -f -u --net --coverage`. - [ ] Quick tests passed locally by running `./runtests.sh --quick --unittests --disttests`. (Ran the affected suites directly with `pytest` on Windows, plus `ruff`/`black`/`isort`/`mypy`, rather than `runtests.sh`; details above.) - [ ] In-line docstrings updated. - [ ] Documentation updated, tested `make html` command in the `docs/` folder. Signed-off-by: VenkateswarluNagineni <venkates2002@tamu.edu> Co-authored-by: Eric Kerfoot <17726042+ericspod@users.noreply.github.com>
1 parent 7c651e9 commit 56f0bd9

2 files changed

Lines changed: 26 additions & 1 deletion

File tree

monai/bundle/config_parser.py

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -162,7 +162,14 @@ def __getattr__(self, key: str) -> Any:
162162
try:
163163
return self._chain(key)
164164
except KeyError:
165-
return getattr(self._value, key)
165+
pass
166+
if isinstance(self._value, dict) and key in self._value:
167+
# the chained id is absent from the resolver (for example when this proxy is
168+
# backed by a `$@ref`, whose children have no ids of their own), but the key
169+
# does exist in the container: resolve it like `__getitem__` does, so dot- and
170+
# bracket-notation agree and config keys keep precedence over dict methods.
171+
return self._value[key]
172+
return getattr(self._value, key)
166173

167174
def __getitem__(self, key: str | int) -> Any:
168175
try:

tests/bundle/test_config_parser.py

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -487,6 +487,24 @@ def test_chained_ref_backed_proxy_write_through(self):
487487
del parser.alias["y"]
488488
self.assertNotIn("y", parser.get_parsed_content("target"))
489489

490+
def test_ref_backed_proxy_attribute_read(self):
491+
# Dot-notation must agree with bracket-notation on a proxy reached via $@ref:
492+
# "alias::x" has no id in the resolver, but "x" is a key of the aliased node, so
493+
# both notations must resolve it (parser.alias.x raised AttributeError before this
494+
# fix, while parser.alias["x"] returned the value).
495+
parser = ConfigParser(config={"target": {"x": 1, "y": 2}, "alias": "$@target"}, globals={"monai": "monai"})
496+
self.assertEqual(parser.alias.x, parser.alias["x"])
497+
self.assertEqual(parser.alias.x, 1)
498+
# a key absent from the container still falls back to the container's own methods
499+
self.assertEqual(sorted(parser.alias.keys()), ["x", "y"])
500+
501+
def test_chained_ref_backed_proxy_attribute_read(self):
502+
# dot-notation must follow the full ref chain, as _backing_id() does for writes.
503+
parser = ConfigParser(
504+
config={"target": {"x": 1}, "mid": "$@target", "alias": "$@mid"}, globals={"monai": "monai"}
505+
)
506+
self.assertEqual(parser.alias.x, 1)
507+
490508
def test_raw_is_read_only(self):
491509
with self.assertRaises(AttributeError):
492510
self.parser.A._raw = {"something": "else"}

0 commit comments

Comments
 (0)