dwi2mask synthstrip: automate GPU usage, fix two pre-existing bugs - #3443
Merged
Merged
Conversation
Mirror "dwi2mask hdbet"'s behaviour: attempt GPU execution first and silently fall back to CPU if it fails, rather than requiring the user to opt in via a "-gpu" option that SynthStrip itself does not gracefully degrade from (a failed "-g" invocation simply aborts and takes dwi2mask with it). The "-gpu" option is therefore removed, along with its use in the options test and the generated docs entry. On CPU fallback, both the GPU and CPU command errors are combined into a single raised MRtrixCmdError (same "===GPU===" / "===CPU===" combined report format used by hdbet.py), so a genuine failure on both paths is still fully diagnosable. While in this file, also fixed two pre-existing bugs confirmed by @Lestropie on the issue: - "cmd" was built from the SYNTHSTRIP_CMD constant instead of the resolved "synthstrip_cmd" path, so a Singularity-based install (found only via SYNTHSTRIP_SINGULARITY) would still try to invoke "mri_synthstrip" directly and fail. - SYNTHSTRIP_SINGULARITY was misspelled as 'sythstrip-singularity' (missing the 'n'), so it could never actually match the intended executable name. Verified with pylint (testing/pylint.rc), clean apart from a pre-existing R0801 duplicate-code note shared with hdbet.py. Fixes MRtrix3#3436 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Lestropie
approved these changes
Sep 15, 2026
Lestropie
left a comment
Member
There was a problem hiding this comment.
Looks as I would have done it myself. 👍
4 tasks
4 tasks
Member
|
Clang tidy review Action is crashing out, but won't be caused by any change in this PR, the PR does not involve any C++ changes, and that Action is considered non-blocking anyway as it only provides code suggestions, so ignoring. Thanks for the contribution! |
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
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.
Summary
Closes #3436. Automates GPU usage in
dwi2mask synthstrip, mirroring the pattern already used bydwi2mask hdbet: attempt the GPU invocation first, and silently fall back to CPU if it fails, instead of requiring an explicit-gpuopt-in.SynthStrip does not itself degrade gracefully when
-gis requested but the hardware/drivers are unavailable — it just fails, takingdwi2maskdown with it. So the-gflag is now requested speculatively on every run and dropped on failure, same ashd-betinhdbet.py. The-gpuoption, its options test entry, and its generated docs entry are all removed accordingly.If both the GPU and CPU attempts fail, both
MRtrixCmdErrors are combined into a single raised error using the same===GPU===/===CPU===report formathdbet.pyuses, so a genuine double failure is still fully diagnosable.Also fixed (same file, confirmed as pre-existing bugs by @Lestropie on the issue)
cmdwas being built from theSYNTHSTRIP_CMDconstant instead of the resolvedsynthstrip_cmdpath returned byshutil.which(...). This meant a Singularity-only install (found viaSYNTHSTRIP_SINGULARITY) would still try to invokemri_synthstripdirectly and fail.SYNTHSTRIP_SINGULARITYwas misspelled'sythstrip-singularity'(missing then), so it could never match the actual container/executable name.Testing
PYTHONPATH="$(pwd)/python/lib" python -m pylint --rcfile=testing/pylint.rc --disable=unexpected-line-ending-format python/mrtrix3/commands/dwi2mask/synthstrip.py— clean, apart from a pre-existingR0801 duplicate-codenote shared withhdbet.py(also present ondevbefore this change, from the two files' now-similar GPU/CPU fallback blocks).testing/scripts/tests/dwi2mask/synthstrip_optionsto drop the now-removed-gpuflag.docs/reference/commands/dwi2mask.rstby hand to matchdocs/generate_user_docs.sh's output (removed the-gpubullet only; no SynthStrip/mri_synthstrip installed locally to run the full doc generation script).🤖 Generated with Claude Code