Lock in QT import dependancy bug + PEP8 Import Compliance #223

SomeoneInParticular · 2025-06-09T00:06:45Z

As the title suggests, this refactor should fix the need to install anything externally if its provided by 3D Slicer itself. This PR also resolves some poor PEP8 coding practices (namely relying on Python import order to ensure things are imported before we call them) to avoid similar bugs cropping up again in the future.

Next step is digging into how to expose everything to the IDE for development purposes; this will be in a follow-up PR

…d first to run (though its dependencies still do...)

…st to run.

…t to run.

…tors

…to run.

… to run.

…` to be loaded first to run.

… loaded first to run.

…licit requirements.py run

…requirements.py

…SampleData went is still a mystery)

…time extremely fast!

… until install is complete.

… utility can be re-written

…all; this aligns more closely with PEP standard practice (and defers the import until its actually needed).

AcastaPaloma · 2025-06-16T18:12:06Z

Haven't gotten any issues yet while testing! Imports all make sense.

maxradx · 2025-06-16T21:51:31Z

Hey @SomeoneInParticular ! When clicking on Load segmentation button and trying to load a previous version, there is an import error (which was not previously present). Can you fix it?

slicercart/SlicerCART/src/scripts/LoadSegmentationWindow.py

Line 109 in 3cba28e

if "nii" in ConfigPath.INPUT_FILE_EXTENSION:

…re respect of 08 columns (there are still remaining paragraph of code to consider, however). Please address the comments so this branch can be ready for merge. A bug has been disclosed in the comments. Thanks

maxradx

Nice work. Interesting to see how much faster SlicerCART is loading!
Left few comments, including some questions.
There is a bug that has been introduced (see comments). Please fix it so it can be ready for PR. Thanks!

maxradx · 2025-06-16T21:29:58Z

SlicerCART/src/scripts/InteractingClasses.py

                edit_button = qt.QPushButton('Edit')
                edit_button.clicked.connect(
-                    lambda state, label=label: self.push_edit_button(label))
+                    lambda _, l=label: self.push_edit_button(l))


Likely a detail, but why did the variable names changed?

_ is shorthand for "variable we are not going to use"; this just makes is trivial for Python programmers to "at a glance" understand that they can safely ignore it for debugging purposes.

Likewise, its common for lambda functions to use single-character variables to keep it short. This very much a preference, though; if we prefer to keep "full" variable names, I have no qualms reverting this.

SlicerCART/src/scripts/InteractingClasses.py

maxradx · 2025-06-16T21:34:57Z

SlicerCART/src/utils/install_python_packages.py

 import slicer

-from importlib import import_module
+from importlib.util import find_spec


Why find_spec instead of import_module?

find_spec won't import the module into memory, and by extension won't run any initialization code required to import said module. This is part of why SlicerCART boots so much faster now.

Instead, SlicerCART will now only import the package when is first used (so if pandas never gets used, it will never be imported, saving the time it would have taken to do so!)

SlicerCART/src/SlicerCART.py

maxradx · 2025-06-16T21:40:03Z

SlicerCART/src/SlicerCART.py

        Args:
        path: Description of path.
        """
+        from bids_validator import BIDSValidator


Basic question, but just to make sure I understand it correctly: why this one is only imported here and other imports are done at the beginning? If this module is not installed then Slicer should not fail since BIDSValidator has been pip installed through install_python_packages.py check and install package function?

Good question!

So, this is a "half-way" implementation of how Slicer expects us to handle external dependencies (that is, packages that neither Python nor Slicer provide for us). Their justification for this is detailed here.

The reason this is "half-way" is to avoid having that try-catch check copied in literally every function that calls an external dependency. Instead we instead have it so that it prompts the user to install any missing dependencies during SlicerCART initialization, managed by install_python_packages.py.

This also technically breaks Slicer best practices (specifically this one), but the alternative solutions are likely to introduce even more developer headache:

Copy-pasting the try-catch install check will bloat the repository, as well as make future development far more tedious.

Wrapping the try-catch in a boilerplate function, which we then import and call, would make IDE development even more painful.

We could also monkey-patch Python's import statement to mediate this for our use; this would preserve IDE compatibility, and save a large amount of boilerplate code. However, its monkey-patching, the joke answer to "how do we solve a problem?"; such a solution would be unstable at best, and risk breaking other Slicer modules if they rely on the default behavior of import.

SlicerCART/src/SlicerCART.py

SomeoneInParticular · 2025-06-16T22:36:24Z

Hey @SomeoneInParticular ! When clicking on Load segmentation button and trying to load a previous version, there is an import error (which was not previously present). Can you fix it?

Good catch! Yes, I mistakenly imported the file rather than the class within it; should be fixed with 3cb36e0

maxradx · 2025-06-17T17:10:23Z

Ok So @SomeoneInParticular is the PR ready to be reviewed again so I can merge it if ok?

SomeoneInParticular · 2025-06-17T17:22:41Z

Almost @maxradx; just need to run everything through a linter to confirm the "80 characters" test, then should be good to go. WIll do so after my meeting at 2pm.

maxradx · 2025-06-17T17:24:37Z

@SomeoneInParticular ok great! please feel free to advise me when ready!

# Conflicts resolved: # SlicerCART/src/SlicerCART.py

SomeoneInParticular · 2025-06-17T21:30:54Z

@maxradx Should be ready now!

maxradx · 2025-06-21T17:09:28Z

@SomeoneInParticular I retried again the load segmentation and using your branch I get this:

(it is a different error than before) Note that the bug is not reproduced using the current main. (when I click on LoadSegmentation) Hint: maybe look in the LoadSegmentationWindow.py file? Thanks!

SomeoneInParticular · 2025-06-21T18:20:59Z

@maxradx what version of Slicer are you running? I just tested it myself with Slicer 5.8.1, and didn't see the same error.

SomeoneInParticular · 2025-06-21T19:24:57Z

Update: so, turns out the VTK implementation provided by Slicer simply doesn't provide access to vtkSegment through Python bindings. My suspicion is that you managed to get access to it through installing QT creator previously; the mass install on the main branch can access a global qt install I believe, while the PEP8 compliant install uses only the qt bindings provided by Slicer...

I'm looking into a workaround now, but due to the bug documented in #247, its incredibly difficult to test right now. Will post an update as soon as I can.

maxradx · 2025-06-21T20:03:09Z

@SomeoneInParticular version 5.8.1
To reproduce:

Open Slicer - New Configuration - Next - Apply
Select Volume folder
Select Output folder
Enter annotator degree, name etc.
Do a segmentation on MRI (ensure MRI is selected when pushing apply in the configuration window in 1)
Save it, go on next case. Then, go back to the current case, do a segmentation (to have more than one version)
Click on load Segmentation
Select version 1
Click on load segmentation
See error:

It currently works in the main branch, so before merging, it should work.
Yes i think in Slicer v5.8.1 they handle segmentation differently in vtk.. but this works in the main so clear it is related to an import issue? maybe missing some mrmrlnode or vtk segment import?

just tried again the steps mentioned above, and it reproduces the error!

… place!

SomeoneInParticular · 2025-06-22T01:04:28Z

So, as it turns out, this was actually an easy fix (though tricky to figure out how to do it). As the loading operation doesn't need to "keep" the old segmentation, we can just modify the prior vtkSegment instance without needing to know its "true" class. @maxradx the bug you mentioned should be fixed in commit 695e0cb

…object, rather than by a function decorator.

…ler instead.

SomeoneInParticular · 2025-06-25T04:24:29Z

@maxradx I replaced your @enter_function decorate in commit 6e8a415; now that decorator is implicit on all functions within SlicerCART, not just those we remember to decorate. Doing this has also allowed us to avoid a potential cyclical loop if we want to debug configuration loading.

@AcastaPaloma give it a spin as well; I think it will help with debugging quite a bit.

UPDATE: There was exactly 1 errant leftover use of the function that could crash the program: this has been fixed in 6dcb390

…due to being commented out.

SomeoneInParticular and others added 20 commits June 6, 2025 16:17

Main SlicerCART.py no longer depends on requirements.py to be loade…

4b6c029

…d first to run (though its dependencies still do...)

ConfigPath.py no longer depends on requirements.py to be loaded fir…

b5646c0

…st to run.

constants.py no longer depends on requirements.py to be loaded firs…

a80e5ae

…t to run.

Restructured imports of debugging_helpers.py to match other refactors

01f9dd7

Refactored imports in development_helpers.py to match the other refac…

ad00f67

…tors

UITheme.py no longer depends on requirements.py to be loaded first …

ec8cf01

…to run.

UserPath.py no longer depends on requirements.py to be loaded first…

5d31002

… to run.

CompareSegmentVersionsWindow.py no longer depends on `requirements.py…

515d854

…` to be loaded first to run.

CustomInteractorStyle.py no longer depends on requirements.py to be…

0f40c22

… loaded first to run.

Updated imports + lambda signatures to avoid variable collision.

52c123e

Updated imports to no longer rely on an implicit requirements.py run

ef86b1a

Updated LoadSegmentationWindow.py imports to no longer rely on an imp…

43b0f5b

…licit requirements.py run

Deleted unused OptionalMethods.py file

f513011

ShowSegmentVersionLegendWindow.py no longer depends on the big scary …

11ac80c

…requirements.py

SlicerCARTLogic.py no longer depends on requirements.py

eec2c95

SlicerCARTTest.py no longer depends on requirements.py (though where …

f603d86

…SampleData went is still a mystery)

Timer.py no longer depends on requirements.py

d276268

WorkFiles.py no longer depends on requirements.py

f87068c

Deleted requirements.py, as it is no longer needed!

ab6d9dc

Reduced the default imports for both submodules; this makes its load …

c5b03d6

…time extremely fast!

SomeoneInParticular requested a review from maxradx June 9, 2025 00:09

Merge branch 'main' into kjoa/the_great_refactoring

48904d0

SomeoneInParticular mentioned this pull request Jun 9, 2025

Small refractoring of generating automatic documentation #189

Open

SomeoneInParticular changed the title ~~Fix QT import dependancy bug~~ Lock in QT import dependancy bug + PEP8 Import Compliance Jun 9, 2025

Removed external lib imports from SlicerCART header; they do not work…

16dd594

… until install is complete.

SomeoneInParticular marked this pull request as draft June 13, 2025 06:32

AcastaPaloma marked this pull request as ready for review June 13, 2025 15:40

AcastaPaloma mentioned this pull request Jun 13, 2025

fixes issue #143 - data probe collapsible folds upon loading SlicerCART #245

Merged

Moved lib check to be in header; temporary workaround until the Debug…

4816042

… utility can be re-written

SomeoneInParticular marked this pull request as draft June 13, 2025 19:46

SomeoneInParticular and others added 3 commits June 15, 2025 19:28

Merge branch 'main' into kjoa/the_great_refactoring

8a4f978

Moved ctk import next to vtk (as they are sibling imports)

c79259b

Swapped from a import attempt wrapped by try-catch to a "find_spec" c…

3cba28e

…all; this aligns more closely with PEP standard practice (and defers the import until its actually needed).

Ok. This commit fixes minor changes related to code formatted to ensu…

caf480f

…re respect of 08 columns (there are still remaining paragraph of code to consider, however). Please address the comments so this branch can be ready for merge. A bug has been disclosed in the comments. Thanks

maxradx requested changes Jun 16, 2025

View reviewed changes

Fixed error resulting from malformed ConfigPath import.

3cb36e0

SomeoneInParticular added 2 commits June 16, 2025 18:40

Removed (no longer relevant) TODO

dbf7754

Removed snark prior to Pandas imports.

2c61250

SomeoneInParticular added 2 commits June 17, 2025 17:12

Merge branch 'main' into kjoa/the_great_refactoring

959680d

# Conflicts resolved: # SlicerCART/src/SlicerCART.py

SliceCART now (mostly) abide by PEP8 line lengh limits.

cb53136

Minor space formatting

14fe6c9

Fixed vtk.vktSegment import error, by not importing it in the first…

695e0cb

… place!

SomeoneInParticular added 3 commits June 24, 2025 03:06

Removed no-longer-used deepcopy import

9e0f9ce

"Tracing" function calls is now handled implicitly by a DEBUG_HELPER …

6e8a415

…object, rather than by a function decorator.

Removed old tracing decorator, as it is now handled by a Python profi…

9e14e84

…ler instead.

SomeoneInParticular added 2 commits June 25, 2025 00:43

Removed some late-night snark

1f9a411

Removed remaining references to @enter_function, which were missed …

6dcb390

…due to being commented out.

Lock in QT import dependancy bug + PEP8 Import Compliance #223

Are you sure you want to change the base?

Lock in QT import dependancy bug + PEP8 Import Compliance #223

Uh oh!

Conversation

SomeoneInParticular commented Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AcastaPaloma commented Jun 16, 2025

Uh oh!

maxradx commented Jun 16, 2025

Uh oh!

maxradx left a comment

Choose a reason for hiding this comment

Uh oh!

maxradx Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

SomeoneInParticular Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maxradx Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

SomeoneInParticular Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

maxradx Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

SomeoneInParticular Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SomeoneInParticular commented Jun 16, 2025

Uh oh!

maxradx commented Jun 17, 2025

Uh oh!

SomeoneInParticular commented Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maxradx commented Jun 17, 2025

Uh oh!

SomeoneInParticular commented Jun 17, 2025

Uh oh!

maxradx commented Jun 21, 2025

Uh oh!

SomeoneInParticular commented Jun 21, 2025

Uh oh!

SomeoneInParticular commented Jun 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maxradx commented Jun 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SomeoneInParticular commented Jun 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SomeoneInParticular commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SomeoneInParticular commented Jun 9, 2025 •

edited

Loading

SomeoneInParticular Jun 16, 2025 •

edited

Loading

SomeoneInParticular Jun 16, 2025 •

edited

Loading

SomeoneInParticular commented Jun 17, 2025 •

edited

Loading

SomeoneInParticular commented Jun 21, 2025 •

edited

Loading

maxradx commented Jun 21, 2025 •

edited

Loading

SomeoneInParticular commented Jun 22, 2025 •

edited

Loading

SomeoneInParticular commented Jun 25, 2025 •

edited

Loading