Skip to content

Conversation

@eclipse0922
Copy link
Contributor

@eclipse0922 eclipse0922 commented Dec 1, 2025

Fixes #8396 .

Description

This PR fixes a bug where Invertd would fail with a RuntimeError when postprocessing pipelines contain invertible transforms (such as Lambdad) before Invertd is called. Root Cause: Invertd copied the entire applied_operations list (including both preprocessing AND postprocessing transforms) before inversion. When the preprocessing pipeline's inverse was called, it would pop transforms from the end and encounter postprocessing transforms first, causing an ID mismatch error.

Solution: Implements automatic group tracking infrastructure:
Each Compose instance automatically assigns its unique ID as a group identifier to all child transforms (recursively, including deeply nested wrapped transforms)
Transform metadata in applied_operations now includes an optional group field
Invertd automatically filters applied_operations to only include transforms from the target Compose instance
Zero API changes - the solution is fully automatic and transparent to users
Backward compatible - gracefully handles transforms without groups

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.
  • In-line docstrings updated.
  • Documentation updated, tested make html command in the docs/ folder.

Addresses an issue where `Invertd` fails when postprocessing
contains invertible transforms before `Invertd` is called.
The solution uses automatic group tracking: `Compose` assigns
its ID to child transforms, allowing `Invertd` to filter and
select only the relevant transforms for inversion.

This ensures correct inversion when multiple transform pipelines
are used or when post-processing steps include invertible transforms.
`TraceableTransform` now stores group information. `Invertd` now
filters transforms by group, falling back to the original
behavior if no group information is present (for backward
compatibility). Adds tests to verify the fix and group isolation.
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 1, 2025

Walkthrough

This change implements automatic grouping of nested transforms within Compose instances to enable fine-grained inversion control. When Compose initializes, it assigns a _group identifier (based on instance ID) to nested TraceableTransform objects. This group ID is tracked in the transform info dictionary via a new TraceKeys.GROUP enum member. Invertd now filters applied operations by group ID to invert only transforms from its associated Compose pipeline, preventing cross-interference when multiple pipelines are present. The implementation adds three new private methods (_set_transform_groups in Compose, _filter_transforms_by_group in Invertd) and includes seven new test methods validating multi-pipeline isolation and group tracking behavior.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • monai/transforms/compose.py: Review recursive grouping logic and attribute traversal for nested transforms; verify infinite recursion prevention and Compose re-wrapping avoidance.
  • monai/transforms/post/dictionary.py: Examine _filter_transforms_by_group filtering logic and backward-compatibility fallback when no matching group is found.
  • tests/transforms/inverse/test_invertd.py: Validate that all seven new test methods correctly exercise the grouping/isolation scenarios and don't introduce false positives; ensure tests cover edge cases like mixed pipelines.
  • Cross-file coherence: Confirm TraceKeys.GROUP enum usage is consistent across compose.py, inverse.py, and dictionary.py.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 76.47% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Title check ❓ Inconclusive Title is vague and generic, failing to convey the specific changes—adds automatic transform grouping for Invertd to handle multiple pipelines. Use a more descriptive title like 'Add automatic transform grouping to Invertd for multiple pipeline support' to clarify the main change.
✅ Passed checks (1 passed)
Check name Status Explanation
Description check ✅ Passed PR description provides clear context: fixes issue #8396, explains the bug, root cause, and solution with automatic group tracking. Includes appropriate checklist items marked.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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 and usage tips.

DCO Remediation Commit for sewon.jeon <[email protected]>

I, sewon.jeon <[email protected]>, hereby add my Signed-off-by to this commit: d97bdd5
I, sewon.jeon <[email protected]>, hereby add my Signed-off-by to this commit: c523e94

Signed-off-by: sewon.jeon <[email protected]>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
monai/transforms/inverse.py (1)

122-133: Group metadata wiring is correct; consider guarding the zip to catch mismatches

The added GROUP handling looks consistent with the new _group field and Invertd’s filtering logic. To make this a bit safer (and address the Ruff B905 hint without depending on zip(strict=...) support), you could assert that the number of keys matches vals before zipping:

-        vals = (
-            self.__class__.__name__,
-            id(self),
-            self.tracing,
-            self._do_transform if hasattr(self, "_do_transform") else True,
-        )
-        info = dict(zip(self.transform_info_keys(), vals))
+        vals = (
+            self.__class__.__name__,
+            id(self),
+            self.tracing,
+            self._do_transform if hasattr(self, "_do_transform") else True,
+        )
+        keys = self.transform_info_keys()
+        if len(keys) != len(vals):
+            raise ValueError(f"transform_info_keys {keys} and vals {vals} must have the same length.")
+        info = dict(zip(keys, vals))

This keeps behavior identical today but will fail fast if either side is changed inconsistently in the future.

monai/transforms/post/dictionary.py (1)

862-884: Group-based filtering in Invertd looks correct; minor cleanup possible

The _filter_transforms_by_group helper plus the switch to reading applied_operations from MetaTensor cleanly achieves the goal: Invertd now inverts only transforms whose GROUP matches id(self.transform), and the “no matches → return all” fallback keeps old behavior when grouping isn’t available.

Two small nits you might consider:

  • Import TraceKeys at module level for consistency with other files instead of inside the helper:
-from monai.utils import PostFix, convert_to_tensor, ensure_tuple, ensure_tuple_rep
+from monai.utils import PostFix, TraceKeys, convert_to_tensor, ensure_tuple, ensure_tuple_rep
-        from monai.utils import TraceKeys
-
         # Get the group ID of the transform (Compose instance)
         target_group = str(id(self.transform))
  • Add a brief note in the docstring (or comment above the fallback) that returning all_transforms is explicitly for backward compatibility when grouping metadata is missing, just so future readers don’t “simplify” it away.

Functionally this looks solid.

Also applies to: 919-925

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between e267705 and 8961b77.

📒 Files selected for processing (5)
  • monai/transforms/compose.py (1 hunks)
  • monai/transforms/inverse.py (1 hunks)
  • monai/transforms/post/dictionary.py (2 hunks)
  • monai/utils/enums.py (1 hunks)
  • tests/transforms/inverse/test_invertd.py (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py

⚙️ CodeRabbit configuration file

Review the Python code for quality and correctness. Ensure variable names adhere to PEP8 style guides, are sensible and informative in regards to their function, though permitting simple names for loop and comprehension variables. Ensure routine names are meaningful in regards to their function and use verbs, adjectives, and nouns in a semantically appropriate way. 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. Examine code for logical error or inconsistencies, and suggest what may be changed to addressed these. Suggest any enhancements for code improving efficiency, maintainability, comprehensibility, and correctness. Ensure new or modified definitions will be covered by existing or new unit tests.

Files:

  • monai/transforms/inverse.py
  • monai/transforms/post/dictionary.py
  • monai/transforms/compose.py
  • monai/utils/enums.py
  • tests/transforms/inverse/test_invertd.py
🧬 Code graph analysis (4)
monai/transforms/inverse.py (1)
monai/utils/enums.py (1)
  • TraceKeys (324-337)
monai/transforms/post/dictionary.py (2)
monai/utils/enums.py (2)
  • TraceKeys (324-337)
  • meta (385-386)
monai/data/meta_obj.py (4)
  • applied_operations (190-194)
  • applied_operations (197-203)
  • meta (177-179)
  • meta (182-187)
monai/transforms/compose.py (1)
monai/transforms/inverse.py (2)
  • inverse (440-448)
  • TraceableTransform (42-384)
tests/transforms/inverse/test_invertd.py (2)
monai/transforms/compose.py (5)
  • Compose (123-435)
  • inverse (403-420)
  • inverse (557-578)
  • inverse (652-677)
  • inverse (819-845)
monai/transforms/post/dictionary.py (1)
  • Invertd (772-966)
🪛 Ruff (0.14.6)
monai/transforms/inverse.py

128-128: zip() without an explicit strict= parameter

Add explicit value for parameter strict=

(B905)

monai/transforms/compose.py

301-302: try-except-pass detected, consider logging the exception

(S110)


301-301: Do not catch blind exception: Exception

(BLE001)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
  • GitHub Check: min-dep-pytorch (2.7.1)
  • GitHub Check: min-dep-pytorch (2.8.0)
  • GitHub Check: min-dep-pytorch (2.5.1)
  • GitHub Check: min-dep-py3 (3.9)
  • GitHub Check: min-dep-pytorch (2.6.0)
  • GitHub Check: min-dep-py3 (3.10)
  • GitHub Check: min-dep-py3 (3.12)
  • GitHub Check: min-dep-py3 (3.11)
  • GitHub Check: min-dep-os (windows-latest)
  • GitHub Check: min-dep-os (ubuntu-latest)
  • GitHub Check: min-dep-os (macOS-latest)
  • GitHub Check: build-docs
  • GitHub Check: quick-py3 (ubuntu-latest)
  • GitHub Check: packaging
  • GitHub Check: flake8-py3 (codeformat)
  • GitHub Check: flake8-py3 (mypy)
  • GitHub Check: quick-py3 (macOS-latest)
  • GitHub Check: quick-py3 (windows-latest)
  • GitHub Check: flake8-py3 (pytype)
🔇 Additional comments (2)
monai/utils/enums.py (1)

324-338: TraceKeys.GROUP addition is consistent

Adding GROUP = "group" to TraceKeys matches how grouping metadata is written in get_transform_info and consumed in Invertd; no issues here.

tests/transforms/inverse/test_invertd.py (1)

140-357: Good coverage for group-aware Invertd and Compose behavior

The new tests exercise exactly the tricky scenarios introduced by group tracking: postprocessing before Invertd, multiple independent pipelines, group isolation on applied_operations, direct Compose.inverse() calls, and mixed Invertd/Compose.inverse usage. This should give solid regression coverage for the new _group / TraceKeys.GROUP logic.

Signed-off-by: sewon.jeon <[email protected]>
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.

Invertd Confused by Invertible Transforms in Postprocessing

1 participant