Skip to content
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

BUG: Fix failing to install optional dependencies during selective install #66

Merged

Conversation

jamesobutler
Copy link
Contributor

image

Traceback:

Traceback (most recent call last):
  File "C:\Users\butlej30383\AppData\Local\slicer.org\Slicer 5.6.2\bin\Python\slicer\util.py", line 3255, in tryWithErrorDisplay
    yield
  File "C:/Users/butlej30383/AppData/Local/slicer.org/Slicer 5.6.2/slicer.org/Extensions-32448/TotalSegmentator/lib/Slicer-5.6/qt-scripted-modules/TotalSegmentator.py", line 319, in onPackageUpgrade
    self.logic.setupPythonRequirements(upgrade=True)
  File "C:/Users/butlej30383/AppData/Local/slicer.org/Slicer 5.6.2/slicer.org/Extensions-32448/TotalSegmentator/lib/Slicer-5.6/qt-scripted-modules/TotalSegmentator.py", line 821, in setupPythonRequirements
    self.pipInstallSelective('nnunetv2', nnunetRequirement, packagesToSkip)
  File "C:/Users/butlej30383/AppData/Local/slicer.org/Slicer 5.6.2/slicer.org/Extensions-32448/TotalSegmentator/lib/Slicer-5.6/qt-scripted-modules/TotalSegmentator.py", line 706, in pipInstallSelective
    slicer.util.pip_install(requirement)
  File "C:\Users\butlej30383\AppData\Local\slicer.org\Slicer 5.6.2\bin\Python\slicer\util.py", line 3887, in pip_install
    _executePythonModule("pip", args)
  File "C:\Users\butlej30383\AppData\Local\slicer.org\Slicer 5.6.2\bin\Python\slicer\util.py", line 3848, in _executePythonModule
    logProcessOutput(proc)
  File "C:\Users\butlej30383\AppData\Local\slicer.org\Slicer 5.6.2\bin\Python\slicer\util.py", line 3814, in logProcessOutput
    raise CalledProcessError(retcode, proc.args, output=proc.stdout, stderr=proc.stderr)
subprocess.CalledProcessError: Command '['C:/Users/butlej30383/AppData/Local/slicer.org/Slicer 5.6.2/bin/../bin\\PythonSlicer.EXE', '-m', 'pip', 'install', 'black;extra', '==', 'dev']' returned non-zero exit status 2.

importlib finds the optional dependencies where nnUNet has optional dependencies (see here) for dev usage for enforcing various linting rules. Although these don't need to be installed to run TotalSegmentator successfully, I've changed the code to continue to install all the optional dependencies when using the selective pip install action.

# Install all dependencies but the ones listed in packagesToSkip
import importlib.metadata
requirements = importlib.metadata.requires(packageToInstall)

cc: @lassoan


This was originally brought up by @Keyn34 in Slicer/ExtensionsIndex#2127 (comment).

I also get an error while installing TotalSegmentator Extension for black;extra==dev. I did not investigate further, though.

Copy link
Owner

@lassoan lassoan left a comment

Choose a reason for hiding this comment

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

Thanks a lot @jamesobutler. I've just added two comments inline.

match = re.match(r"([\S]+)[\s]*; extra == '([^']+)'", requirement)
# Rewrite optional depdendencies:
# ruff; extra == "dev" -> ruff
match = re.match(r"([\S]+)[\s]*; extra == \"([^\"]+)\"", requirement)
Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't we just skip the dependency if extra is dev?

Copy link
Contributor Author

@jamesobutler jamesobutler Dec 6, 2024

Choose a reason for hiding this comment

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

I considered it but optional dependencies appears to have no standard about group names within it. A project could calling it “linting” or “dev” or whatever and I didn’t want to add code to generic method pipInstallSelective that was unique to nnUnet. All optional dependencies will report as package_name; extra == “group_name”.

Alternatively the packagesToSkip could include known linting packages like black, ruff, pre-commit, however that list could arbitrarily grow and I didn’t want to start that precedent here either.

TotalSegmentator/TotalSegmentator.py Outdated Show resolved Hide resolved
…stall

subprocess.CalledProcessError: Command '['C:/Users/butlej30383/AppData/Local/slicer.org/Slicer 5.6.2/bin/../bin\\PythonSlicer.EXE', '-m', 'pip', 'install', 'black;extra', '==', 'dev']' returned non-zero exit status 2.
@jamesobutler jamesobutler force-pushed the fix-traceback-optional-dependencies branch from 407a0e6 to bd7eec2 Compare December 6, 2024 23:59
@lassoan lassoan merged commit 2d764fb into lassoan:main Dec 13, 2024
@lassoan
Copy link
Owner

lassoan commented Dec 13, 2024

Thank you for your contribution!

@jamesobutler jamesobutler deleted the fix-traceback-optional-dependencies branch December 13, 2024 16:02
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.

2 participants