-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Change SupervisedTrainer decollation default. #8542
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Change SupervisedTrainer decollation default. #8542
Conversation
Walkthrough
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNone found. Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
Signed-off-by: Ivan <[email protected]> Signed-off-by: id-b3 <[email protected]>
Signed-off-by: id-b3 <[email protected]>
4afd580
to
acd8ceb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
monai/engines/trainer.py (1)
121-124
: Docstring tweak: tighten rationale and guidance.Small wording improvement; also nudge users when they actually need it.
- default to `False` as training slows due to tensor movement to CPU for decollation when enabled. + default is `False` to avoid the CPU transfer overhead introduced by decollation; enable only when your + postprocessing requires list-based transforms from `monai.transforms`.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (4)
monai/engines/trainer.py
(2 hunks)tests/apps/deepgrow/transforms/test_deepgrow_interaction.py
(1 hunks)tests/integration/test_deepedit_interaction.py
(1 hunks)tests/testing_data/config_fl_train.json
(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:
tests/apps/deepgrow/transforms/test_deepgrow_interaction.py
tests/integration/test_deepedit_interaction.py
monai/engines/trainer.py
🔇 Additional comments (4)
tests/testing_data/config_fl_train.json (1)
122-124
: Config: explicitly enabling decollation is correct for this test bundle.The added "decollate": true pairs with monai.engines.SupervisedTrainer under train.trainer and matches the new default change. No JSON syntax issues.
monai/engines/trainer.py (1)
157-157
: Audit complete – no missingdecollate=
needed
No Python call sites pass apostprocessing=
without also specifyingdecollate=
, and the only config referencingSupervisedTrainer
doesn’t usepostprocessing
. Defaultingdecollate=False
is safe as-is.tests/integration/test_deepedit_interaction.py (1)
106-106
: LGTM: enabling decollation here is necessary.Post transforms (Activationsd, AsDiscreted, SplitPredsLabeld, ToTensord) operate per-item; explicitly setting decollate=True preserves existing semantics under the new default.
tests/apps/deepgrow/transforms/test_deepgrow_interaction.py (1)
81-81
: LGTM: explicit decollate=True keeps interaction transforms correct.Iteration transforms include ToNumpyd/ToTensord and discrepancy ops that expect item-wise dicts; this explicit flag is appropriate with the default flipped.
Fixes #8541 .
Description
By default, the SupervisedTrainer has
decollation
enabled by default. The decollation step moves the tensors to CPU for processing which significantly impacts the speed of each training step as described in issue #8541This pull request proposes to change the default of decollation to
False
and provide more information in the documentation as to the impact of the decollation option with regard to the training speed.Types of changes
./runtests.sh -f -u --net --coverage
../runtests.sh --quick --unittests --disttests
.make html
command in thedocs/
folder.