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 DWI shell extraction module import sorting #202

Conversation

jhlegarreta
Copy link
Contributor

@jhlegarreta jhlegarreta commented Nov 4, 2023

Fix DWI shell extraction module import sorting: sort the imported packages following the class hierarchy and library dependencies.

Fixes:

loading:  SlicerDMRI/SlicerDMRI/Testdata/3x3x3_13_b1000_b3000.nrrd
Traceback (most recent call last):
  File "SlicerDMRI-build/inner-build/lib/Slicer-5.5/cli-modules/ExtractDWIShells.py",
  line 207, in <module>
    main()
  File "SlicerDMRI-build/inner-build/lib/Slicer-5.5/cli-modules/ExtractDWIShells.py",
  line 127, in main
    sn.ReadData(node_in)
TypeError: ReadData argument 1: method requires a VTK object

raised for example at:
https://github.com/SlicerDMRI/SlicerDMRI/actions/runs/6722459285/job/18270424711?pr=191#step:8:2631

Related discussion:
Slicer/Slicer#6484

Fix DWI shell extraction module import sorting: sort the imported
packages following the class hierarchy and library dependencies.

Fixes:
```
loading:  SlicerDMRI/SlicerDMRI/Testdata/3x3x3_13_b1000_b3000.nrrd
Traceback (most recent call last):
  File "SlicerDMRI-build/inner-build/lib/Slicer-5.5/cli-modules/ExtractDWIShells.py",
  line 207, in <module>
    main()
  File "SlicerDMRI-build/inner-build/lib/Slicer-5.5/cli-modules/ExtractDWIShells.py",
  line 127, in main
    sn.ReadData(node_in)
TypeError: ReadData argument 1: method requires a VTK object
```

raised for example at:
https://github.com/SlicerDMRI/SlicerDMRI/actions/runs/6722459285/job/18270424711?pr=191#step:8:2631

Related discussion:
Slicer/Slicer#6484
@jhlegarreta jhlegarreta force-pushed the FixExtractDWIShellsImportSorting branch from bf9450c to 3742fb5 Compare November 4, 2023 15:16
@jhlegarreta
Copy link
Contributor Author

Rebase on master after PR #191 gets merged to demonstrate that the patch set fixes the issue.

Copy link
Contributor

@jcfr jcfr left a comment

Choose a reason for hiding this comment

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

Instead of fixing the ordering on a few files, the complete code base should be updated using tooling like Ruff.

To help with this, you may look at this pull request where I introduced the use of Ruff:

Additionally, a GitHub workflow for running pre-commit should be added.

@jhlegarreta
Copy link
Contributor Author

@jcfr this PR does not aim at fixing the style of imports; it aims at fixing the test. The fix was applied after reading Slicer/Slicer#6484 (comment), and Steve had also mentioned the import sorting in https://discourse.slicer.org/t/fixing-extension-testing-failures/32584/2

Applying Ruff would change virtually all files in the repository, and before doing that, first is to have the tests passing.

I ignore how Ruff deals with the import sorting, but note that before Ruff was adopted by 3D Slicer on Nov 3 Slicer/Slicer#7338, I had submitted PRs #186, #187 and #188 to SlicerDMRI late October. Note that isort would sort imports alphabetically (unless an explicit exclusion statement is added to the import line to tell isort not to apply its rules). Alphabetical sorting is clear and consistent, and easy to follow when reading code. However, following the above posts, 3D Slicer seems to require manually sorting some of the imports. As a side note, this 3D Slicer PR is relevant to the discussion Slicer/Slicer#6613, as well as this comment Slicer/Slicer#6613 (comment).

So, please, @jcfr consider the scope of the PR and re-consider the review.

@jcfr
Copy link
Contributor

jcfr commented Nov 16, 2023

consider the scope of the PR and re-consider the review.

Thanks for clarifying.

Since adjusting the order of the import is prone to error, I addressed the issue in Slicer itself. This change is superseded by:

@jhlegarreta
Copy link
Contributor Author

Since adjusting the order of the import is prone to error, I addressed the issue in Slicer itself. This change is superseded by:

👍 hats off to the PR in 3D Slicer. Thank you.

@jhlegarreta
Copy link
Contributor Author

The ExtractDWIShells_Test test is passing now:
https://slicer.cdash.org/viewTest.php?onlyfailed&buildid=3212756
vs
https://slicer.cdash.org/viewTest.php?onlyfailed&buildid=3211674

Closing. Thanks @jcfr.

@jhlegarreta jhlegarreta deleted the FixExtractDWIShellsImportSorting branch November 17, 2023 13:21
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.

None yet

2 participants