Skip to content

Fix dot-notation read on $@ref-backed ConfigParser proxies#8994

Open
VenkateswarluNagineni wants to merge 1 commit into
Project-MONAI:devfrom
VenkateswarluNagineni:fix/config-proxy-ref-attr-read
Open

Fix dot-notation read on $@ref-backed ConfigParser proxies#8994
VenkateswarluNagineni wants to merge 1 commit into
Project-MONAI:devfrom
VenkateswarluNagineni:fix/config-proxy-ref-attr-read

Conversation

@VenkateswarluNagineni

Copy link
Copy Markdown
Contributor

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:

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:

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

  • 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).
  • 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.

_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>
@coderabbitai

coderabbitai Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 278fd715-8a0c-44bd-92e9-3b1fbacdd350

📥 Commits

Reviewing files that changed from the base of the PR and between 3a458fe and 9298678.

📒 Files selected for processing (2)
  • monai/bundle/config_parser.py
  • tests/bundle/test_config_parser.py

📝 Walkthrough

Walkthrough

Updated _ConfigProxy.__getattr__ to return literal dictionary entries when chained configuration lookup cannot resolve a key, otherwise preserving attribute fallback behavior. Added tests confirming direct and chained $@ref proxies provide matching dot- and bracket-notation reads, including container method fallback.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and concisely summarizes the main change: fixing dot-notation reads on $@ref-backed ConfigParser proxies.
Description check ✅ Passed The description is detailed and follows the template sections, including changes, tests, and change-type checkboxes; only the issue link is left blank.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant