Add nested dot-notation access to ConfigParser (#6837)#8858
Merged
ericspod merged 9 commits intoMay 19, 2026
Conversation
Wrap dict/list results from ConfigParser.__getattr__ in _ConfigProxy
so that parser.training.trainer.max_epochs chains through
get_parsed_content using the '::' separator, equivalent to
parser.get_parsed_content('training::trainer::max_epochs').
Signed-off-by: Soumya Snigdha Kundu <soumya_snigdha.kundu@kcl.ac.uk>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR extends ConfigParser to support nested dot-notation and bracket-notation access across hierarchical config structures. A new _ConfigProxy wrapper class intercepts attribute and index access on parsed dict/list results, translating each access step into chained get_parsed_content() lookups. When a key is absent from the config hierarchy, the proxy falls back to native container semantics. The getattr method is updated to wrap returned dicts and lists in this proxy. A comprehensive test validates nested access patterns, container delegation, fallback behavior, and error handling. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
monai/bundle/config_parser.py (1)
38-110: ⚡ Quick winDocument the new proxy surface more fully.
_wrap_parsedand the new_ConfigProxymethods add subtle behavior (._raw, config-key precedence, fallback to native exceptions), but the changed definitions mostly do not document args/returns/raised exceptions. Please add Google-style docstrings to the new definitions so this behavior is explicit.As per coding guidelines, "Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@monai/bundle/config_parser.py` around lines 38 - 110, Add Google-style docstrings for the new API surface: document the function _wrap_parsed (args: parser, id, value; returns wrapped _ConfigProxy or raw value; no exceptions), the class _ConfigProxy (describe purpose, precedence rules for config keys vs underlying dict/list, and that underlying value is available via ._raw), and key methods __init__ (args and stored attributes), _chain (args, returns wrapped value, may raise KeyError), __getattr__ and __getitem__ (describe fallback behavior to underlying value and which exceptions may propagate), and the _raw property and __eq__ (returns and behavior). Ensure each docstring follows Google style with Args, Returns, and Raises sections where applicable and references the ID_SEP_KEY/key-chaining behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@monai/bundle/config_parser.py`:
- Around line 80-86: The proxied container currently only implements __getitem__
in _ConfigProxy (using _chain and falling back to _value) so assignment/deletion
like parser.foo["bar"] = ... or parser.foo[0] = ... break; implement and
delegate __setitem__ and __delitem__ on _ConfigProxy to call
self._chain(str(key)) behavior or fall back to self._value[key] for
writes/deletes, mirroring the __getitem__ semantics (use _chain for key
resolution, then perform the set/delete on the underlying container), and add a
regression test that exercises assignment and deletion on proxied dicts and
lists to prevent future regressions.
---
Nitpick comments:
In `@monai/bundle/config_parser.py`:
- Around line 38-110: Add Google-style docstrings for the new API surface:
document the function _wrap_parsed (args: parser, id, value; returns wrapped
_ConfigProxy or raw value; no exceptions), the class _ConfigProxy (describe
purpose, precedence rules for config keys vs underlying dict/list, and that
underlying value is available via ._raw), and key methods __init__ (args and
stored attributes), _chain (args, returns wrapped value, may raise KeyError),
__getattr__ and __getitem__ (describe fallback behavior to underlying value and
which exceptions may propagate), and the _raw property and __eq__ (returns and
behavior). Ensure each docstring follows Google style with Args, Returns, and
Raises sections where applicable and references the ID_SEP_KEY/key-chaining
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 65e1e366-7f13-4c3f-8bba-30a789a618f0
📒 Files selected for processing (2)
monai/bundle/config_parser.pytests/bundle/test_config_parser.py
Preserve the pre-proxy behaviour where parser.x returned the raw dict/list, so parser.x[k] = v and del parser.x[k] still work. Add a regression test. Signed-off-by: Soumya Snigdha Kundu <soumya_snigdha.kundu@kcl.ac.uk>
Contributor
|
Tip For best results, initiate chat on the files or code changes.
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
monai/bundle/config_parser.py (1)
209-224: 💤 Low valueAdd
Returns:section to docstring.Per coding guidelines, docstrings should describe return values. The updated docstring explains the wrapping behavior but lacks a formal
Returns:section.Suggested docstring addition
See also: :py:meth:`get_parsed_content` + + Returns: + The parsed content, wrapped in ``_ConfigProxy`` if it is a dict or list. """🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@monai/bundle/config_parser.py` around lines 209 - 224, Add a formal "Returns:" section to the __getattr__ docstring describing what is returned: the parsed value fetched via get_parsed_content(id) (including possible types: primitive, dict, list, object), and that dicts/lists are wrapped in a _ConfigProxy to allow nested attribute/index access; mention the default parsing behavior (lazy=True, instantiate=True, eval_expr=True) and that the returned value is equivalent to parser.get_parsed_content("training::trainer::max_epochs") for nested access. Reference __getattr__, get_parsed_content, and _wrap_parsed in the description so readers can locate the implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@monai/bundle/config_parser.py`:
- Around line 209-224: Add a formal "Returns:" section to the __getattr__
docstring describing what is returned: the parsed value fetched via
get_parsed_content(id) (including possible types: primitive, dict, list,
object), and that dicts/lists are wrapped in a _ConfigProxy to allow nested
attribute/index access; mention the default parsing behavior (lazy=True,
instantiate=True, eval_expr=True) and that the returned value is equivalent to
parser.get_parsed_content("training::trainer::max_epochs") for nested access.
Reference __getattr__, get_parsed_content, and _wrap_parsed in the description
so readers can locate the implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7e12037f-c52a-4e8a-9a91-fecb19ec1ede
📒 Files selected for processing (2)
monai/bundle/config_parser.pytests/bundle/test_config_parser.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/bundle/test_config_parser.py
ericspod
requested changes
May 18, 2026
Member
ericspod
left a comment
ericspod
left a comment
There was a problem hiding this comment.
Hi @aymuos15 I think this solution generally works. It has the side-effect of changing the type of extracted values from a ConfigParser instance, eg. type(parser.A) is _ConfigProxy rather than dict. This would possibly change some behaviour for those checking the member type. The docstring for __getattr__ mentions this behaviour but it should be explained in the docstring for the class along with an added example code of the notation. I had another question about behaviour in a comment below as well.
Signed-off-by: Soumya Snigdha Kundu <soumya_snigdha.kundu@kcl.ac.uk>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/bundle/test_config_parser.py (1)
396-475: ⚡ Quick winAdd method docstrings for new test definitions.
The new
setUp/test_*methods should include Google-style docstrings to satisfy repo policy.As per coding guidelines, "Docstrings should be present for all definition which describe each variable, return value, and raised exception in the appropriate section of the Google-style of docstrings."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/bundle/test_config_parser.py` around lines 396 - 475, The tests lack Google-style docstrings: add concise Google-style docstrings to setUp and each test method (setUp, test_nested_attribute_access, test_nested_index_access, test_raw_and_container_protocol, test_native_index_fallback, test_attribute_write_through, test_item_write_through, test_delete_write_through, test_copy_and_pickle_yield_raw_container, test_config_key_shadows_container_method, test_missing_raises) describing purpose, key parameters/state (e.g., self.config), expected behavior, return value (None), and any exceptions raised; place the docstring as the first statement in each method, following the repo’s Google-style sections (Args, Returns, Raises) and keeping them brief and relevant to the test logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@monai/bundle/config_parser.py`:
- Around line 158-166: The bug is that __setitem__ and __delitem__ operate on
the raw node at self._id (which can be a "`@ref`" string) instead of the resolved
backing container; change both methods to resolve the backing id/container
before mutating: call the parser's ref-resolution API to get the resolved
id/node for self._id (e.g. via self._parser.ref_resolver.resolve(...) or the
parser method that returns the backing id/node), compute the child id with
self._child_id(...) against that resolved id, perform the set/delete on the
resolved container (use int(key) when the resolved node is a list), and then
call self._parser.ref_resolver.reset() after the mutation; add a regression test
exercising mutations through an `@ref-backed` dict and list proxy to ensure
parser.<id>.x and get_parsed_content reflect the change.
- Around line 132-148: The proxy's __getattr__ falls back to the underlying
container (self._value) exposing raw mutator methods (e.g., append, extend,
insert, remove, update, pop, popitem, clear, setdefault) without resetting the
reference resolver, causing stale parsed trees after mutations; modify
__getattr__ so when getattr(self._value, key) is returned and key is one of
those mutator names (or the attribute is callable and belongs to list/dict
mutators), wrap that callable in a small wrapper that calls ref_resolver.reset()
(or self.ref_resolver.reset()) before/after invoking the original mutator and
then returns the wrapper; keep the existing behaviour for non-mutators and
continue to use self._chain(key) for config keys.
In `@tests/bundle/test_config_parser.py`:
- Line 458: Add an S301 suppression and brief clarifying comment to the trusted
in-process pickle roundtrip assertion that uses pickle.loads(pickle.dumps(a));
specifically, append a trailing comment like "# noqa: S301 - trusted in-process
roundtrip for test only" to the line containing pickle.loads(pickle.dumps(a))
(the assertEqual that compares the roundtripped value) so Ruff will ignore the
unsafe-deserialization warning while documenting why it's safe.
---
Nitpick comments:
In `@tests/bundle/test_config_parser.py`:
- Around line 396-475: The tests lack Google-style docstrings: add concise
Google-style docstrings to setUp and each test method (setUp,
test_nested_attribute_access, test_nested_index_access,
test_raw_and_container_protocol, test_native_index_fallback,
test_attribute_write_through, test_item_write_through,
test_delete_write_through, test_copy_and_pickle_yield_raw_container,
test_config_key_shadows_container_method, test_missing_raises) describing
purpose, key parameters/state (e.g., self.config), expected behavior, return
value (None), and any exceptions raised; place the docstring as the first
statement in each method, following the repo’s Google-style sections (Args,
Returns, Raises) and keeping them brief and relevant to the test logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 94ccc805-ac18-4f9c-bd03-1509a082f310
📒 Files selected for processing (2)
monai/bundle/config_parser.pytests/bundle/test_config_parser.py
Signed-off-by: Soumya Snigdha Kundu <soumya_snigdha.kundu@kcl.ac.uk>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
tests/bundle/test_config_parser.py (1)
458-458:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSuppress Ruff S301 on this trusted roundtrip.
The comment does not silence the lint error. Add
# noqa: S301here, or split out the payload and tag theloadscall, so CI stays green.Minimal patch
- self.assertEqual(pickle.loads(pickle.dumps(a)), {"B": {"C": 1, "D": [10, 20]}}) # trusted in-process roundtrip + payload = pickle.dumps(a) + self.assertEqual(pickle.loads(payload), {"B": {"C": 1, "D": [10, 20]}}) # noqa: S301 - trusted in-process roundtrip🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/bundle/test_config_parser.py` at line 458, The test triggers Ruff S301 for untrusted pickle use; suppress it by silencing the linter on this known-safe in-process roundtrip: update the assertion in tests/bundle/test_config_parser.py (the line with self.assertEqual(pickle.loads(pickle.dumps(a)), {"B": {"C": 1, "D": [10, 20]}})) to include a suppression, e.g. add "# noqa: S301" to the expression or alternatively split the payload into a variable and append "# noqa: S301" to the pickle.loads(...) call so the S301 warning is suppressed while keeping the test behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@monai/bundle/config_parser.py`:
- Around line 178-185: The proxy allows setting/deleting the internal attribute
"_raw" via __setattr__ and __delattr__, which creates/removes a config key
instead of preserving it as read-only; update __setattr__ and __delattr__ in
ConfigProxy so that if key == "_raw" they raise AttributeError (preventing
mutation), otherwise keep the existing behavior that checks
_ConfigProxy._INTERNAL and delegates to object.__setattr__ or self[key]/del
self[key]; reference the methods __setattr__, __delattr__ and the internal
sentinel _ConfigProxy._INTERNAL to locate the changes.
- Around line 116-123: _backing_id currently only peels a single $@ref hop so
chained aliases (alias -> $@mid -> $@target) still resolve to the intermediate
id; update _backing_id to iteratively resolve references: repeatedly read raw =
self._parser[current_id], call ReferenceResolver.match_refs_pattern(raw), and if
a single ref is found set current_id to that ref and continue until raw is not a
string or no refs found, then return current_id. Apply the identical
transitive-resolution fix to the other reference-resolving helper around the
167-175 region (the same logic used by ConfigParser.__setitem__ will then
operate on the real target), and add/adjust the
test_ref_backed_proxy_write_through test to cover chained aliases.
- Around line 44-59: Rename the parameter named id to config_id in helper
functions to avoid shadowing the builtin and clarify intent: update the function
signature and all internal references in _wrap_parsed(parser: ConfigParser, id:
str, value: Any) -> Any to use config_id, and make the same rename in the other
helper function(s) around lines 102-110 (update their signatures and all uses of
id to config_id), ensuring calls to these helpers and any places constructing
the ::-separated identifier use the new name consistently.
---
Duplicate comments:
In `@tests/bundle/test_config_parser.py`:
- Line 458: The test triggers Ruff S301 for untrusted pickle use; suppress it by
silencing the linter on this known-safe in-process roundtrip: update the
assertion in tests/bundle/test_config_parser.py (the line with
self.assertEqual(pickle.loads(pickle.dumps(a)), {"B": {"C": 1, "D": [10, 20]}}))
to include a suppression, e.g. add "# noqa: S301" to the expression or
alternatively split the payload into a variable and append "# noqa: S301" to the
pickle.loads(...) call so the S301 warning is suppressed while keeping the test
behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6492d5b7-9b19-4f2c-a157-d894ce9a0f94
📒 Files selected for processing (2)
monai/bundle/config_parser.pytests/bundle/test_config_parser.py
ericspod
reviewed
May 19, 2026
Member
|
My comments were addressed here and it looks better, but you should look at the Coderabbit remarks and check what should be addressed. The one about raw mutator methods maybe is fine to leave however, but either way resolve comments once they're addressed then I can review again. |
- _backing_id() now loops until no more $@ref hops remain, fixing chained-alias write/delete (alias -> mid -> target). - __setattr__ and __delattr__ raise AttributeError on _raw to prevent silently creating a config key named _raw. - Add regression tests for both cases. Signed-off-by: Soumya Snigdha Kundu <soumya_snigdha.kundu@kcl.ac.uk>
aymuos15
force-pushed
the
fix-6837-configparser-dot-notation
branch
from
41dec58 to
c73b66c
Compare
…tents Signed-off-by: Soumya Snigdha Kundu <soumya_snigdha.kundu@kcl.ac.uk>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/bundle/test_config_parser.py (1)
458-458:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd explicit S301 suppression on trusted pickle roundtrip.
Line 458 still triggers Ruff S301. Add
# noqa: S301with the trust-context note so CI lint passes.Suggested patch
- self.assertEqual(pickle.loads(pickle.dumps(a)), {"B": {"C": 1, "D": [10, 20]}}) # trusted in-process roundtrip + self.assertEqual( + pickle.loads(pickle.dumps(a)), {"B": {"C": 1, "D": [10, 20]}} # noqa: S301 - trusted in-process roundtrip for test only + )#!/bin/bash set -euo pipefail rg -n 'pickle\.loads\s*\(' tests/bundle/test_config_parser.py rg -n 'noqa:\s*S301' tests/bundle/test_config_parser.py🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/bundle/test_config_parser.py` at line 458, The test uses an in-process trusted pickle roundtrip via pickle.dumps/pickle.loads in the assertion inside tests/bundle/test_config_parser.py (the failing expression is the assertEqual comparing pickle.loads(pickle.dumps(a)) to the expected dict); suppress the Ruff S301 warning by appending a inline noqa comment to that expression — add "# noqa: S301 # trusted in-process roundtrip" to the line containing pickle.loads(pickle.dumps(a)) so the linter recognizes this trusted usage without changing test behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@monai/bundle/config_parser.py`:
- Around line 175-178: The __setitem__ implementation always encodes the
provided key as a new id segment which breaks symmetry with
__getitem__/__delitem__ for literal keys that contain ID_SEP_KEY; change
__setitem__ (involving _backing_id, ID_SEP_KEY) to first check whether the
literal combined key (candidate = f"{backing}{ID_SEP_KEY}{key}") already exists
in self._parser and, if it does, write to that exact candidate (preserving the
literal key), otherwise fall back to the existing behavior of treating the key
as a nested id segment; this preserves literal "a::b" assignments while keeping
nested-path behavior when no literal key exists.
---
Duplicate comments:
In `@tests/bundle/test_config_parser.py`:
- Line 458: The test uses an in-process trusted pickle roundtrip via
pickle.dumps/pickle.loads in the assertion inside
tests/bundle/test_config_parser.py (the failing expression is the assertEqual
comparing pickle.loads(pickle.dumps(a)) to the expected dict); suppress the Ruff
S301 warning by appending a inline noqa comment to that expression — add "#
noqa: S301 # trusted in-process roundtrip" to the line containing
pickle.loads(pickle.dumps(a)) so the linter recognizes this trusted usage
without changing test behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c8ba4132-26a1-43e0-84c4-234bd7fddfb8
📒 Files selected for processing (2)
monai/bundle/config_parser.pytests/bundle/test_config_parser.py
Preserves literal dict keys (e.g. "a::b") and matches the semantics of __delitem__ and __getitem__. Signed-off-by: Soumya Snigdha Kundu <soumya_snigdha.kundu@kcl.ac.uk>
Contributor
Author
|
Thank you so much for the guidance and detailed review! |
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
2 participants
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.
Fixes #6837.
Description
Follow-up to #5813: extends dot/bracket notation to nested config. Dict/list results from
ConfigParser.__getattr__are wrapped in a_ConfigProxythat chains viaget_parsed_content, so nested$@ref/_target_still resolve:._rawexposes the raw container;len/iter/in/booland missing-key access delegate to it. Config keys take precedence over dict/list methods on attribute access (consistent with #5813).Types of changes
parser.xdict/list is now_ConfigProxy;isinstance(parser.x, dict)no longer matches).