Fix RecursionError in set_rnd on cyclic object graphs (#8087) - #9056
Fix RecursionError in set_rnd on cyclic object graphs (#8087)#9056ousamabenyounes wants to merge 1 commit into
Conversation
…8087) monai.data.utils.set_rnd recursively walks obj.__dict__ to seed randomizable components. When a dataset holds an OmegaConf/Hydra config (whose child nodes back-reference their parent), or any object graph with a reference cycle, the recursion never terminates and raises RecursionError while building a DataLoader with num_workers=0. Track visited object ids in an internal _seen set and skip already-visited objects, breaking the cycle while still seeding every reachable randomizable component exactly once. Signed-off-by: Ben Younes <2910651+ousamabenyounes@users.noreply.github.com>
📝 WalkthroughWalkthrough
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 |
![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 (2)
monai/data/utils.py (1)
689-699: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the return value.
set_rndreturns the next seed, but its docstring has noReturnssection.As per path instructions, “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/data/utils.py` around lines 689 - 699, Update the set_rnd docstring to add a Google-style Returns section documenting that the function returns the next seed as an integer.Source: Path instructions
tests/data/test_dataloader.py (1)
112-123: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd docstrings to the added definitions.
__init__,__len__,__getitem__, andtest_cyclic_reference_no_recursionlack docstrings.As per path instructions, “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.”
Also applies to: 127-131
🤖 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/data/test_dataloader.py` around lines 112 - 123, Add Google-style docstrings to the added __init__, __len__, __getitem__, and test_cyclic_reference_no_recursion definitions, documenting their relevant attributes, arguments, return values, and any exceptions raised; preserve the existing behavior and test logic.Source: Path instructions
🤖 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/data/utils.py`:
- Around line 702-711: Update set_rnd to check and record the list or tuple
identity in _seen before recursively iterating its items, while preserving the
existing seed propagation and return behavior. Add a regression test covering a
dataset whose cfg contains itself, ensuring set_rnd completes without
RecursionError.
---
Nitpick comments:
In `@monai/data/utils.py`:
- Around line 689-699: Update the set_rnd docstring to add a Google-style
Returns section documenting that the function returns the next seed as an
integer.
In `@tests/data/test_dataloader.py`:
- Around line 112-123: Add Google-style docstrings to the added __init__,
__len__, __getitem__, and test_cyclic_reference_no_recursion definitions,
documenting their relevant attributes, arguments, return values, and any
exceptions raised; preserve the existing behavior and test logic.
🪄 Autofix
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 Plus
Run ID: 0a702579-4303-457b-9a49-7f6a0e95349c
📒 Files selected for processing (2)
monai/data/utils.pytests/data/test_dataloader.py
| if isinstance(obj, (tuple, list)): # ZipDataset.data is a list | ||
| _seed = seed | ||
| for item in obj: | ||
| _seed = set_rnd(item, seed=seed) | ||
| _seed = set_rnd(item, seed=seed, _seen=_seen) | ||
| return seed if _seed == seed else seed + 1 # return a different seed if there are randomizable items | ||
| if not hasattr(obj, "__dict__"): | ||
| return seed # no attribute | ||
| if id(obj) in _seen: | ||
| return seed # already visited: avoid infinite recursion on cyclic references | ||
| _seen.add(id(obj)) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Track list and tuple identities before recursion.
Line 702 processes containers before the _seen check. A self-referential list still raises RecursionError.
Add the identity check before walking a list or tuple. Add a regression case where dataset.cfg contains itself.
Proposed fix
if _seen is None:
_seen = set()
if isinstance(obj, (tuple, list)): # ZipDataset.data is a list
+ if id(obj) in _seen:
+ return seed
+ _seen.add(id(obj))
_seed = seed
for item in obj:
_seed = set_rnd(item, seed=seed, _seen=_seen)
return seed if _seed == seed else seed + 1 # return a different seed if there are randomizable items📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if isinstance(obj, (tuple, list)): # ZipDataset.data is a list | |
| _seed = seed | |
| for item in obj: | |
| _seed = set_rnd(item, seed=seed) | |
| _seed = set_rnd(item, seed=seed, _seen=_seen) | |
| return seed if _seed == seed else seed + 1 # return a different seed if there are randomizable items | |
| if not hasattr(obj, "__dict__"): | |
| return seed # no attribute | |
| if id(obj) in _seen: | |
| return seed # already visited: avoid infinite recursion on cyclic references | |
| _seen.add(id(obj)) | |
| if _seen is None: | |
| _seen = set() | |
| if isinstance(obj, (tuple, list)): # ZipDataset.data is a list | |
| if id(obj) in _seen: | |
| return seed | |
| _seen.add(id(obj)) | |
| _seed = seed | |
| for item in obj: | |
| _seed = set_rnd(item, seed=seed, _seen=_seen) | |
| return seed if _seed == seed else seed + 1 # return a different seed if there are randomizable items | |
| if not hasattr(obj, "__dict__"): | |
| return seed # no attribute | |
| if id(obj) in _seen: | |
| return seed # already visited: avoid infinite recursion on cyclic references | |
| _seen.add(id(obj)) |
🤖 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/data/utils.py` around lines 702 - 711, Update set_rnd to check and
record the list or tuple identity in _seen before recursively iterating its
items, while preserving the existing seed propagation and return behavior. Add a
regression test covering a dataset whose cfg contains itself, ensuring set_rnd
completes without RecursionError.
1 participant
Description
monai.data.utils.set_rndseeds all randomizable properties of an object byrecursively walking
obj.__dict__. When a dataset holds a config object whosegraph contains a reference cycle — e.g. an OmegaConf/Hydra config, whose child
nodes back-reference their parent node — the recursion never terminates and
raises
RecursionErrorwhile constructing aDataLoaderwithnum_workers=0:This reproduces the Hydra
DataLoaderrecursion reported in the issue: theDataLoader.__init__seeding path callsset_rnd(dataset, ...), and thedataset's config attribute forms a parent/child cycle.
Fix
Track the ids of already-visited objects in an internal
_seenset and skip anyobject already seen, breaking cyclic references while still seeding every
reachable randomizable component exactly once. Non-cyclic graphs and existing
seeding semantics are unchanged (shared randomizable objects are now seeded once
per call, which is the correct deterministic behavior).
Types of changes
Fixes #8087
Test verification (RED → GREEN)
New regression test
TestLoaderRecursion.test_cyclic_reference_no_recursionconstructs a
DataLoaderover a dataset whose attribute graph has a referencecycle.
RED — on
devbefore the fix:GREEN — with the fix: