Use expanded pip install tooling#21
Conversation
Remove workarounds that were only needed for Python 3.9: - _installACVLUtils (pinning acvl-utils==0.2 for upstream bugs) - _downgradeDynamicNetworkArchitecture (pinning ==0.2.0 for TotalSegmentator compatibility) - acvl-utils entry in skip list - sys.version_info < (3, 12) guards The extension now targets Slicer 5.10+ (Python 3.12+). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace custom dependency management infrastructure with Slicer's new pip utilities (pip_check, pip_install with skip_packages): - Replace pipInstallSelective + METADATA scrubbing with slicer.util.pip_install(skip_packages=[...]) - Replace 6 version-checking methods with slicer.util.pip_check - Replace custom confirmation dialog with confirmOkCancelDisplay - Remove pip_install/pip_uninstall logging wrappers - Remove progressInfo Signal from InstallLogic (Slicer handles install progress UI via show_progress parameter) This reduces InstallLogic from 345 to 180 lines. The remaining code is PyTorch extension handling which is genuinely extension-specific. Note: SlicerTotalSegmentator uses InstallLogic.isPackageInstalled, isPackageInstalledAndCompatible, packageMetaFilePath, and setupPythonRequirements. It will need to be updated to use slicer.util.pip_check directly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Thibault-Pelletier
left a comment
There was a problem hiding this comment.
Thx for the PR!
Having this in Slicer core would indeed remove a lot of code!
Just a thought
A lot of the complexity comes from trying to cope with bad package dependencies in the nnUNet pip package.
This is still ongoing as seen in this issue
I think that, as module maintainers, we will probably still need to freeze versions depending on combinations of SlicerVersion & OS.
This could be done using requirements files with specific naming conventions and given to the slicer.util.pip method, or other.
Looking forward to your thoughts on this!
| self.assertFalse(InstallLogic.isPackageInstalledAndCompatible("not_package")) | ||
| def test_pip_check_nnunetv2_does_not_raise(self): | ||
| # Should not raise, regardless of whether nnunetv2 is installed | ||
| slicer.util.pip_check(Requirement("nnunetv2")) |
There was a problem hiding this comment.
Test file should be removed altogether (no need to test a Slicer core function in an extension)
|
Another thought regarding the breaking changes, the methods could be kept and delegate to the slicer.util methods internally. This would avoid breaking other's code for what is only utility functions. The same can be said for the install reporting. Having the install pushed elsewhere from a progress bar / python console is useful for end users. Having a signal for this information stream and being able to display it in the module panel is useful for modules installing a lot of dependencies (or dependencies which take a while to install like the nnUNet. |
Yes, agreed that this would be the approach: requirements files with specific naming conventions.
Right; if we move forward I can also make a PR on TotalSegmentator but I agree with your suggestion if it doesn't get integrated quickly. |
SlicerNNUNet is used in more than the TotalSegmentator and we don't have versionning system nor check in the module dependencies. If it were a normal Python package we would bump the major to indicate breaking changes in the API. |
If Slicer/Slicer#9010 is merged, this PR is demonstrating how the python dependency handling in SlicerNNUnet could be simplified.
This is a demonstration -- feel free to close out the PR or to edit it or request changes. It does work though!
Summary
slicer.utilpip utilities:pip_check, andpip_installwithskip_packagesWhat changed
The extension previously implemented its own recursive selective pip install (
pipInstallSelective) with METADATA file scrubbing, version checking, and requirement string normalization — about 180 lines of infrastructure. This duplicated functionality now available in Slicer viaslicer.util.pip_checkandslicer.util.pip_install(skip_packages=[...]).Deleted:
pipInstallSelective,_removeSkippedPackagesFromMetaDataFile,packageMetaFilePath,cleanPyPiRequirement,isPackageInstalledAndCompatible,isPackageInstalled,isInstalledPackageCompatible,needsToInstallRequirement,getInstalledPackageVersion,asRequirement,_requestPermissionToInstallOrRaise,pip_installwrapper,pip_uninstallwrapper,_installACVLUtils,_downgradeDynamicNetworkArchitecture,progressInfoSignal.Kept:
setupPythonRequirements(rewritten),_installNNUnet(rewritten),_installPyTorch(rewritten),getInstalledNNUnetVersion, PyTorch extension handling (installPyTorchExtensionAndRestartIfNeeded,installTorchUtils,_getTorchLogic).Breaking changes
SlicerTotalSegmentator imports from
SlicerNNUNetLib.InstallLogicand uses:InstallLogic.isPackageInstalled(Requirement("nnunetv2"))→ replace withslicer.util.pip_check(Requirement("nnunetv2"))InstallLogic.isPackageInstalledAndCompatible(...)→ replace withslicer.util.pip_check(...)InstallLogic.packageMetaFilePath(...)→ useimportlib.metadata.files(...)directlyInstallLogic.setupPythonRequirements(...)→ still available, signature unchangedTotalSegmentator will need to be updated to use
slicer.util.pip_checkdirectly for the removed methods.Why
pip_installinstead ofpip_ensureThis extension uses the lower-level
pip_check+pip_installrather thanpip_ensurebecause the install flow has extension-specific steps thatpip_ensurecan't accommodate: the PyTorch Slicer extension must be installed (and possibly require a restart) before the pip install, torch is installed separately viaSlicerPyTorchrather than pip, and nnunetv2 must be explicitly uninstalled before reinstalling.Requires
Slicer 5.10+ with
slicer.util.pip_installskip_packagessupport (Slicer PR #9010).