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

Radiomics extension CLI fails on mac 4.8.1 #43

Closed
pieper opened this issue Feb 7, 2018 · 7 comments · Fixed by #46
Closed

Radiomics extension CLI fails on mac 4.8.1 #43

pieper opened this issue Feb 7, 2018 · 7 comments · Fixed by #46

Comments

@pieper
Copy link
Collaborator

pieper commented Feb 7, 2018

Tested Slicer 4.8.1 and it has an old version. Once we are comfortable with the CLI we should update the version of SlicerRadiomics for the stable release.

Currently the mac nightly is not available due to the qt5 transition, so there's no version for mac at the moment.

RadiomicsCLI standard error:

Traceback (most recent call last):
  File "/Applications/Slicer-4.8.1.app/Contents/Extensions-26813/SlicerRadiomics/lib/Slicer-4.8/cli-modules/SlicerRadiomicsCLIScript", line 6, in <module>
    from radiomics.scripts import commandline
  File "/Applications/Slicer-4.8.1.app/Contents/Extensions-26813/SlicerRadiomics/lib/python2.7/site-packages/radiomics/__init__.py", line 4, in <module>
    import collections  # noqa: F401
  File "/Applications/Slicer-4.8.1.app/Contents/lib/Python/lib/python2.7/collections.py", line 20, in <module>
    from _collections import deque, defaultdict
ImportError: No module named _collections



RadiomicsCLI completed with errors
@JoostJM
Copy link
Contributor

JoostJM commented Feb 8, 2018

Related to #41.

With the exception of the build error on Mac, the extensions seems to be working very nicely. The windows test fails, but this is due to the fact that it can't find the CLI. If you test manually on windows the test passes normally.

@JoostJM
Copy link
Contributor

JoostJM commented Feb 19, 2018

Some interesting points concerning this issue are raised in this topic on slicer discourse. Apparently due to Apple SIP policy, slicer DYLD_LIBRARY_PATH is not exported, meaning that the SlicerRadiomicsCLI script is unable to find the correct python libraries.

@ihnorton
Copy link

ihnorton commented Feb 19, 2018

A simple, only-slightly-hacky solution would be to set a var like __MY_DYLD_LIBRARY_PATH to contain the same paths as DYLD_LIBRARY_PATH in the calling environment. Then check for that in the wrapper script, and if it exists, export the proper var in the subshell.

@JoostJM
Copy link
Contributor

JoostJM commented Feb 19, 2018

@ihnorton Thanks for tracking this down! We'll look into fixing this, hopefully implementing some not-really hacking solution soon, but going for a slightly more hacky solution if needed.

@jcfr
Copy link
Collaborator

jcfr commented Feb 20, 2018

Few approaches:

(1) You could improve the launcher so that it also set variables like APPLAUNCHER_DYLD_LIBRARY_PATH, APPLAUNCHER_LD_LIBRARY_PATH and APPLAUNCHER_PATH. Once the fix implemented, we could easily release new launcher binaries by tagging a new version.

(2) Otherwise, an easier approach could be to simply use the AppLauncher CMake function in the pyradiomics CMakeLists.txt to configure a launcher per CLI. For example, see here

Let me know if you have questions

@ihnorton
Copy link

Note that the SIP restriction only applies to system-provided binaries (/bin/sh, /usr/bin/python, etc.). That's why the executable C++ based CLIs work fine. So, clean and simple solution would be to make RadiomicsCLI a "real" CLI in C++, but one which just does the same thing as the current script (find python-real, launch it with pass-through arguments).

Unfortunately process control is hard to do correctly in a cross-platform way in C++ (you can't use std::system because it just shells-out, so the same restriction applies). If you go that route, I would suggest to use kwsys because I think you already have the ITK dependency (which -> SystemTools::FindProgram, and you can look at the process creation steps in e.g. Slicer CLI execution code).

@ihnorton
Copy link

I had another idea for a potential solution which could avoid adding any environment variables, and avoid additional layers of cmake/launchers (plus would allow removing the .bat and .sh wrapper scripts).

Slicer/Slicer#894

JoostJM added a commit that referenced this issue Apr 30, 2018
Remove use .bat and .sh wrapper script, and instead define the CLI entry point as a python script.
This makes use of the new Slicer functionality described in Slicer/Slicer#894 and implemented in r27143-r27145.

This fixes #43.

Additionally, check if a `--label <label>` argument is present, and if so, update it to the new-style: `--setting=label:<label>`. Parameter `--label` is deprecated by PR AIM-Harvard/pyradiomics#347.
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 a pull request may close this issue.

4 participants