Add nested dot-notation access to ConfigParser (#6837)#8858
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>
📝 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}
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>
|
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! |
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
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.
| result = trans(np.ones(64)) | ||
| self.assertTupleEqual(result.shape, (1, 8, 8)) | ||
|
|
||
| def test_nested_dot_notation(self): |
There was a problem hiding this comment.
I think this should be broken up into multiple tests focused on specific areas. Checking exception raises should be separate, for example. You can put config into a setUp method to avoid copying it.
|
|
||
| # item assignment/deletion writes through to the underlying container, | ||
| # preserving the pre-proxy behaviour where ``parser.x`` was the raw dict/list. | ||
| parser.A.B["C"] = 99 |
There was a problem hiding this comment.
I think _ConfigProxy.__setattr__ may need to be implemented or something else fixed. If I do parser.A.X=[2,3] this should create an item X in the dictionary A as defined in your test here, and I can see that dictionary changed in the return value for parser.get_parsed_content("A"). However, the expression parser.A.X raises AttributeError: 'dict' object has no attribute 'X'. I don't think the assignment is updating the underlying config structure correctly. You should add tests that attempt to add members this way then extract the value again, and also check against get_parsed_content.
2 participants
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).