RFC: ENH: allow direct execution of CLI python scripts using Slicer's python #894

ihnorton · 2018-02-20T20:07:55Z

This is a quick hack demonstrating a potential solution to this issue and related problems (e.g. running multiprocessing).

tl;dr, this detects a .pycli file in a CLI directory (cli-modules), and then configures the CLI node to run the script via Slicer's python interpreter. As an example, for SlicerRadiomicsCLI, this gives the following command line:

RadiomicsCLI command line:

/opt/bld/s5nj/python-install/bin/python inner-build/lib/Slicer-4.9/cli-modules/SlicerRadiomicsCLI.py --label 1 --out /var/folders/7l/2qsp6sqx4_b5x9kf_yzzm4lc0000gn/T/Slicer/EBACJ_vtkMRMLTableNodeB.csv /var/folders/7l/2qsp6sqx4_b5x9kf_yzzm4lc0000gn/T/Slicer/EBACJ_vtkMRMLScalarVolumeNodeB.nrrd /var/folders/7l/2qsp6sqx4_b5x9kf_yzzm4lc0000gn/T/Slicer/EBACJ_vtkMRMLLabelMapVolumeNodeB.nrrd

The command is exec'd via the existing CLI mechanism, runs out-of-process and async as any other CLI would be, but it inherits the full Slicer environment without requiring any launcher/cmake/etc. steps. Running directly also avoids OS security policy issues caused by shelling-out to protected interpreters.

Note that this is different from the current scripted CLI module path which runs in-process and blocks on the interpreter for the duration of the call (but should have no effect on that code path).

~~This PR does not handlepython-real in a factory-packaged Slicer, and other issues, but I wanted to put it up for feedback before spending time on it.~~

cc @fedorov @lassoan @pieper @ljod

fedorov · 2018-02-20T20:19:55Z

@ihnorton thank you for working on this! I am afraid that I don't know enough about the subject matter to give any helpful feedback, and can't allocate time to investigate in depth.

pieper · 2018-02-20T20:40:08Z

Looks very promising but I also didn't have time to dig into it.

ihnorton · 2018-02-21T13:05:47Z

Here is a little more context. The goal is to run python scripts outside Slicer, but using Slicer's python (and environment). SlicerRadiomicsCLI currently does this via batch files and shell scripts as wrappers used to re-invoke Slicer's python on a script (pieper's solution) -- but this has the limitations detailed in the linked forum thread due to security policy. We would like to do something similar for WhiteMatterAnalysis, and will face the same constraints, especially the need to run the computations asynchronously.

There is a different approach via a custom launcher, but it would require a net addition of CMakeScript, and still require the batch and shell scripts wrapper layers. I think the approach here could be simpler for extension developers to maintain.

The approach of this PR is to teach the CLI mechanism to run python scripts directly, which has two steps:

identify .pycli files in CLI search paths
during instanciation, configure the CLI node to execute .pycli files using python-real (or python) interpreter. Specifically, the CLI logic already has support for a "Location" parameter, which I believe is currently only used for shared module entry points. If the "Location" parameter is set, then the logic already uses it as the first command line argument. This PR sets up the "Location" for .py files to be the python interpreter, which leads to the command line shown above.

pieper · 2018-02-21T13:30:48Z

Thanks @ihnorton that makes sense now. Definitely cleaner to centralize the improvements in the Slicer CLI code so all extensions can be cleaner. 👍

pieper · 2018-02-21T16:03:12Z

@ihnorton we discussed this on the radiomics call and like the direction. Do you know what the circleci issue is? What will it take to support for the build vs install versions of Slicer's python?

lassoan · 2018-02-21T16:46:32Z

Note that on Windows, python-real does not run (cannot find DLLs) but you have to use SlicerPython.exe launcher.

While you are at it, it would be great to change SlicerPython.exe executable name to PythonSlicer.exe, as PyCharm (and maybe other software?) requires Python interpreter name to start with "Python".

Is this going to be configurable to use various Python interpreters? We usually want to use Slicer's Python but it could be nice to be able to use others.

Eliminated need to wrapper stub, which is blocked by macOS SIP policy and has some other drawbacks. Ref: Slicer/Slicer#894

ihnorton · 2018-02-21T22:18:55Z

@pieper

discussed this on the radiomics call and like the direction

Thanks!

Do you know what the circleci issue is?

Still building against Qt 4.7, which appears not to have QStandardPaths.

What will it take to support for the build vs install versions of Slicer's python?

Well, to keep this quick-and-simple for now, I just updated the PR to

consider .pycli as the "external Python scripted" extension, so that the wrappers are no longer necessary.
look first for python-real, then python in PATH, which is what the SlicerRadiomicsCLI wrapper was doing (and what some code in the extensionwizard utility does).

If we want to do something more involved (as per below -- which I think is a good idea), then I'd like to work out a plan on a call before implementing.

@lassoan

on Windows, python-real does not run (cannot find DLLs)

I haven't tried this change on Windows, but I believe that issue would be mitigated in this particular context, because python-real would be invoked with Slicer's PATH (equivalent to Slicer --launch python-real)

it would be great to change SlicerPython.exe executable name to PythonSlicer.exe,

Agreed. I'll keep that in mind to look at in the future (probably it's only a few lines, but making that kind of change usually ends up with CMake and packaging issues that I can't debug right now).

Is this going to be configurable to use various Python interpreters?

I thought about that a little bit but don't have a good suggestion yet. It would be very nice to support multiple python (e.g. conda) and even non-python interpreters via CLI, but that would require some deeper refactoring of the CLI initialization. The loader first needs to decide at startup whether to consider a given file as a valid executable, what kind of CLI factory to use, before even approaching the interpreter question. The current logic for "ExecutableCLI" is: QFileInfo::isExecutable(...) == true && suffix == {.bat | .exe } (Windows) or empty (POSIX). I added a check for .pycli in this PR. I'm also not sure about the best way to expose/configure such a feature. Maybe we can figure out a good approach on the next Slicer call.

jcfr · 2018-02-21T22:59:36Z

I thought about that a little bit but don't have a good suggestion yet. It would be very nice to support multiple python (e.g. conda)

within C++ CLI, we support restoring the environment excluding Slicer one, this allows us to use script expected to work with anaconda, etc ....

We add the following at the top of the main function of the c++ CLI

#include <ctkAppLauncherEnvironment.h>
#include <QProcess>

[...]

int main(int argc, char * argv[])
{
[...]
    //construct the command line  
    std::string command = "python \path\to\script.py";

    //append all the parameters to command
    command += " --fooImage=" + fooImage;


    QProcessEnvironment origEnv = ctkAppLauncherEnvironment::environment(0);
    ctkAppLauncherEnvironment::updateCurrentEnvironment(origEnv);

    [....]

    QProcess qp;
    qp.setProcessChannelMode(QProcess::ForwardedChannels);
    qp.start(QString::fromStdString(command));
    bool success = qp.waitForFinished(-1);
    QProcess::ExitStatus es = qp.exitStatus();

    if (!success || es != QProcess::ExitStatus::NormalExit)
    {
        std::cerr << "Python script exited abnormally, exit code: " << qp.exitCode() << std::endl;
        return EXIT_FAILURE;
    }
    else
    {
        std::cout << "Python script  finished normally, exit code: " << qp.exitCode() << std::endl;
    }
    return qp.exitCode();
}

[...]

find_package(SlicerExecutionModel REQUIRED)
include(${SlicerExecutionModel_USE_FILE})

find_package(CTKAppLauncherLib REQUIRED)

set(MODULE_TARGET_LIBRARIES
  ${CTKAppLauncherLib_LIBRARIES}
  )

SEMMacroBuildCLI(
  NAME ${MODULE_NAME}
  TARGET_LIBRARIES ${MODULE_TARGET_LIBRARIES}
  ADDITIONAL_SRCS ${MODULE_SRCS}
  EXECUTABLE_ONLY
  )

Ideally, we should extend the SlicerExecutionModel to allow ignoring the parent environment.

ihnorton · 2018-03-02T20:39:48Z

@jcfr to follow up on your comment, the goal of this PR was to allow running python code in the SlicerPython interpreter without any intermediate step (currently: .sh+.bat wrapper, or CLI C++ executable as you indicated). This could make the end-developer experience simpler in my opinion, at a low complexity cost as in this PR (the CLI logic already supports it so we just need to detect/expose it during setup).

If we add that feature, then there's also the question raised by Andras about whether to support arbitrary interpreters. Which could be conda-python, lua, whatever -- and of course there are work-arounds to do that right now, with some downside/complexity. Technically we could, I'm just not sure how to configure it.

ihnorton · 2018-03-02T21:00:44Z

What would folks think about changing the CLI identification logic from:

QFileInfo::isExecutable(file) to
QFileInfo::isExecutable(file) || QFile(file).startsWith(0x23 0x21) ?

Then any script starting with #! (interpreted as a magic number) would be treated as executable by the CLI loader. (the reason: on Windows,QFileInfo::isExecutable only considers the extensions .bat .exe and .com to be executable -- which is mirrored in qSlicerUtils::isCLIExecutable)

lassoan · 2018-03-05T13:50:10Z

Shouldn't we just check for .py extension as well? Would you remove the .py extension from Python files?

ihnorton · 2018-03-05T15:29:16Z

Shouldn't we just check for .py extension as well?

Currently the module loader seems to treat all .py files in all module directories as potential scripted modules. We could change that of course.

Would you remove the .py extension from Python files?

Maybe yes, for this type of "CLI direct exec" Python script, but depends on what we do for the above issue.

lassoan · 2018-03-05T16:00:35Z

I see. The problem is that .py files are always treated as Python scripted modules and there is no way to indicate that they are CLI modules. Giving a hint in the file content could be a good idea.

We already have dual-mode Python files, which can act as a module or as a slicelet, for example LabelStatistics.py.

We should implement a solution that provides simple and consistent solution for all 3 cases:

A. Python scripted module
B. Python script to be executed in Slicer application's interpreter
C. CLI implemented in Python

Using #! to indicate that it is a CLI seems to be a good idea, as it would mimic what a unix-like OS would do, and if the file extension is kept as .py then Slicer application could emulate the same behavior on Windows.

Dual-mode (A and B) modules could be simplified by splitting to 2 Python files - one would remain a scripted module only, and a second could be just a script file.

Probably we would need to define a special !# code for type B script. Could that be done in a standard way so that on a unix-like platform the script would actually executed by launching Slicer application like this?

./Slicer --no-main-window --python-script lib/Slicer-4.7/qt-scripted-modules/LabelStatistics.py

Could there be a variant to run with GUI?

./Slicer --python-script lib/Slicer-4.7/qt-scripted-modules/LabelStatistics.py

fedorov · 2018-03-05T16:19:18Z

Dual-mode (A and B) modules could be simplified by splitting to 2 Python files - one would remain a scripted module only, and a second could be just a script file.

Another way to deal with this is to have main() function defined in the scripted module. This way functionality does not need to be separated, it becomes more clear that the logic in the python script is shared/reused from the scripted module, and main() can only be interpreted when invoked as a python script. It also has the advantage that we don't need to change anything in the code, just introduce a new convention that is already implemented in some modules. See example here: https://github.com/SlicerProstate/mpReview/blob/master/mpReviewPreprocessor.py.

lassoan · 2018-03-05T16:32:29Z

By using a main() function, do you mean adding code that should be run as script after if __name__ == "__main__": ? That would still be supported anyway, just there would be an option to have generic (non-CLI) Python scripts that can be in the module folder but would not be attempted to be used as modules.

fedorov · 2018-03-05T16:41:11Z

@lassoan yes, just wanted to mention it, since you mentioned splitting to separate modules from script files to handle dual-mode (A and B).

ihnorton · 2018-03-07T19:49:40Z

@lassoan

Dual-mode (A and B) modules could be simplified by splitting to 2 Python files - one would remain a scripted module only, and a second could be just a script file.
Probably we would need to define a special !# code for type B script.

Assuming we make #! to identify CLI-only scripts, I think @fedorov's idea could work already for A vs B, in a single file and without any other distinguishing code, because:

(mode A): when the qScriptedLoadableModule parses a file, the __name__ for the execution environment is set to className.
(mode B): during direct execution (via --python-script), the environment has __name__ == '__main__' as expected.

So I think the simplest solution is to leave the decision about what to do in mode B (with/without --main-window) up to the implementer.

if __name__ == '__main__': # this block is ignored when loading as scripted module!
  if slicer.util.mainWindow is None:
    runSlicelet()
  else:
    runSomethingElse()

Regarding:

Could that be done in a standard way so that on a unix-like platform the script would actually executed by launching Slicer application like this?
./Slicer --no-main-window --python-script lib/Slicer-4.7/qt-scripted-modules/LabelStatistics.py
Could there be a variant to run with GUI?
./Slicer --python-script lib/Slicer-4.7/qt-scripted-modules/LabelStatistics.py

If the above sketch is not sufficient, can you explain a little bit the expected behavior difference between these two invocations? (note that --python-script implies that Slicer quits immediately after running)

ihnorton · 2018-03-07T19:57:18Z

Oh, for this point:

That would still be supported anyway, just there would be an option to have generic (non-CLI) Python scripts that can be in the module folder but would not be attempted to be used as modules.

The convention is that the script has a GUI if a class called [className]Widget exists. (where className is the filename without .py extension).

lassoan · 2018-03-07T20:00:04Z

There are python scripted modules that don't have widget class. So, if the widget class is missing, it may still be a (non-CLI) scripted module.

ihnorton · 2018-03-08T01:52:13Z

There are python scripted modules that don't have widget class. So, if the widget class is missing, it may still be a (non-CLI) scripted module.

Non-CLI scripted module handling should be be unaffected unless they happen to have #! at the start. That is currently a no-op, but if we make it signify CLI status, then the line would need to be removed/prohibited for non-CLI scripted modules (I don't see any scripted modules currently using #! in base Slicer or in SlicerRT).

lassoan · 2018-03-08T02:11:08Z

Base/QTCore/qSlicerUtils.cxx

@@ -51,6 +51,11 @@ bool qSlicerUtils::isExecutableName(const QString& name)
 //------------------------------------------------------------------------------
 bool qSlicerUtils::isCLIExecutable(const QString& filePath)
 {
+  if (filePath.endsWith(".pycli"))


it should be Qt::CaseInsensitive comparison (at least on Windows)

lassoan · 2018-03-08T02:13:48Z

Looks good overall. Even if some issues come up we can always tune it further.

jcfr · 2018-03-08T02:50:21Z

Great. I will also review later tonight.

…

On Mar 7, 2018 9:13 PM, "Andras Lasso" ***@***.***> wrote: Looks good overall. Even if some issues come up we can always tune it further. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <Slicer/Slicer#894 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AANXoygIkhhLfq9OADSd1sYe3cOa3JQRks5tcJPdgaJpZM4SMlmr> .

ihnorton · 2018-03-08T04:05:11Z

I will update to reflect the discussion.

Eliminates need for wrapper stub, which is blocked by macOS SIP policy and has some other drawbacks. Ref: - https://discourse.slicer.org/t/tutorial-for-using-pyradiomics-no-module-named-collections/2111 - Slicer/Slicer#894

ihnorton · 2018-03-19T21:35:00Z

Updated and added test for this functionality.

ihnorton · 2018-03-27T15:34:49Z

I'd like to integrate this week. Please let me know any comments/objections if anyone has a chance to review (or tell me if I should wait). Thanks!

lassoan · 2018-03-27T15:49:30Z

I think we've discussed this enough, it should be integrated and we can improve later if needed. It should not delay the integration, but if you have time, it would be great if you could add a PyCLI module template to the ExtensionWizard.

Thanks a lot for working on this, it will be great to have this feature!

fedorov · 2018-04-08T20:28:01Z

@ihnorton do you still plan to integrate, or there are some extra items that need to be resolved first?

jcfr · 2018-04-08T20:49:30Z

Base/QTCore/qSlicerUtils.cxx

@@ -51,6 +51,18 @@ bool qSlicerUtils::isExecutableName(const QString& name)
 //------------------------------------------------------------------------------
 bool qSlicerUtils::isCLIExecutable(const QString& filePath)
 {
+  // check if .py file starts with `#!` magic
+  //   note: uses QTextStream to avoid issues with BOM.
+  QFile scriptFile(filePath);


I suggest to move this code into a function that can be reused from Base/QTGUI/qSlicerScriptedLoadableModuleFactory.cxx

May be a function named isCLIScriptedExecutable(cons QString& filePath)

jcfr · 2018-04-08T20:53:46Z

Base/QTGUI/qSlicerScriptedLoadableModuleFactory.cxx

+       (scriptStream.readLine(2).startsWith("#!")) )
+    {
+    return false;
+    }


Then, the code above would be simplified to:

if (qSlicerUtils::isCLIScriptedExecutable(file.filePath())) { return false; }

jcfr · 2018-04-08T20:55:29Z

I also suggest to re-base and force push the topic, then CircleCI will be able to build the topic.

ihnorton · 2018-04-09T17:39:13Z

Thanks. I should be able to address comments and template tomorrow, then merge.

Eliminates need for wrapper stub, which is blocked by macOS SIP policy and has some other drawbacks. Ref: - https://discourse.slicer.org/t/tutorial-for-using-pyradiomics-no-module-named-collections/2111 - Slicer/Slicer#894

Eliminates need for wrapper stub, which is blocked by macOS SIP policy and has some other drawbacks. Ref: - https://discourse.slicer.org/t/tutorial-for-using-pyradiomics-no-module-named-collections/2111 - Slicer/Slicer#894 git-svn-id: http://svn.slicer.org/Slicer4/trunk@27143 3bd1e089-480b-0410-8dfb-8563597acbee

ihnorton · 2018-04-12T02:04:43Z

Committed in r27143-r27145 with @jcfr comments addressed. I will make a separate PR for the template.

pieper · 2018-04-13T22:54:11Z

Base/QTCLI/qSlicerCLIExecutableModuleFactory.cxx

@@ -20,6 +20,7 @@

 // Qt includes
 #include <QProcess>
+#include <QStandardPaths>


This was added in Qt5, so it breaks a Qt4 build. @jcfr had some use cases where he wanted to keep Qt4 working at least for a while, so for now I'll just stub out this functionality unless building with Qt5 (his other projects won't need it). Hopefully soon we can make a pass to remove all Qt4 support.

Committed as r27146

fedorov · 2018-04-17T16:04:55Z

Unfortunately, this did not resolve the issue mentioned in the opening comment for this PR: AIM-Harvard/SlicerRadiomics#43:

ImportError: No module named _collections

ihnorton · 2018-04-17T16:12:57Z

@fedorov it doesn't look like there was any change made to use this feature yet? You would need to install this script as a (Python) CLI directly, instead of using the sh/bat wrappers. IIRC SlicerRadiomics has a macro to do that already (used for the wrappers right now).

I did that test manually in local SlicerRadiomics build tree, while developing this branch, and it worked. Let me know if it doesn't and I'll check again.

fedorov · 2018-04-17T16:40:43Z

Ah, I missed it completely. Thanks for the pointers Isaiah!

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.

fedorov · 2018-04-30T20:52:19Z

I confirm this seems to work in a local build. This feature was now integrated into SlicerRadiomics in AIM-Harvard/SlicerRadiomics#46 by @JoostJM. Need to confirm that it works in the nightly. Kudos to @ihnorton!

ihnorton mentioned this pull request Feb 20, 2018

Radiomics extension CLI fails on mac 4.8.1 AIM-Harvard/SlicerRadiomics#43

Closed

ihnorton changed the title ~~RFC: ENH: allow direct execution of CLI python scripts from Slicer's python~~ RFC: ENH: allow direct execution of CLI python scripts using Slicer's python Feb 20, 2018

ihnorton force-pushed the pythonscripted_cli branch from 381f3f0 to e3879ac Compare February 21, 2018 22:05

ihnorton referenced this pull request in ihnorton/Slicer Feb 21, 2018

ENH: recognize .pycli scripts in CLI loader, execute via python[-real]

e3879ac

Eliminated need to wrapper stub, which is blocked by macOS SIP policy and has some other drawbacks. Ref: Slicer/Slicer#894

lassoan reviewed Mar 8, 2018

View reviewed changes

ihnorton force-pushed the pythonscripted_cli branch from e3879ac to c58c66f Compare March 13, 2018 21:21

ihnorton force-pushed the pythonscripted_cli branch from c58c66f to 37da5c7 Compare March 19, 2018 20:54

ihnorton force-pushed the pythonscripted_cli branch from 37da5c7 to 3f761e2 Compare March 19, 2018 20:56

jcfr reviewed Apr 8, 2018

View reviewed changes

ihnorton added 3 commits April 10, 2018 10:09

ENH: add test for CLI-only Python script

a602682

ENH: address review comments, move PyCLI check to helper

dd39fd5

ihnorton force-pushed the pythonscripted_cli branch from 3f761e2 to dd39fd5 Compare April 11, 2018 21:24

ihnorton closed this Apr 12, 2018

ihnorton deleted the pythonscripted_cli branch April 12, 2018 02:05

pieper reviewed Apr 13, 2018

View reviewed changes

JoostJM mentioned this pull request Apr 30, 2018

ENH: Update CLI definition AIM-Harvard/SlicerRadiomics#46

Merged

1 task

jcfr mentioned this pull request May 9, 2018

Add ctk cli2 #497

Open

ihnorton mentioned this pull request Jul 7, 2018

ENH: remove import warning for non-embedded interpreter #986

Closed

RFC: ENH: allow direct execution of CLI python scripts using Slicer's python #894

RFC: ENH: allow direct execution of CLI python scripts using Slicer's python #894

Conversation

ihnorton commented Feb 20, 2018 • edited Loading

fedorov commented Feb 20, 2018

pieper commented Feb 20, 2018

ihnorton commented Feb 21, 2018 • edited Loading

pieper commented Feb 21, 2018

pieper commented Feb 21, 2018

lassoan commented Feb 21, 2018

ihnorton commented Feb 21, 2018

jcfr commented Feb 21, 2018

ihnorton commented Mar 2, 2018 • edited Loading

ihnorton commented Mar 2, 2018 • edited Loading

lassoan commented Mar 5, 2018

ihnorton commented Mar 5, 2018

lassoan commented Mar 5, 2018

fedorov commented Mar 5, 2018

lassoan commented Mar 5, 2018

fedorov commented Mar 5, 2018

ihnorton commented Mar 7, 2018 • edited Loading

ihnorton commented Mar 7, 2018

lassoan commented Mar 7, 2018

ihnorton commented Mar 8, 2018 • edited Loading

lassoan Mar 8, 2018

Choose a reason for hiding this comment

lassoan commented Mar 8, 2018

jcfr commented Mar 8, 2018 via email

ihnorton commented Mar 8, 2018

ihnorton commented Mar 19, 2018

ihnorton commented Mar 27, 2018

lassoan commented Mar 27, 2018 • edited Loading

fedorov commented Apr 8, 2018

jcfr Apr 8, 2018 • edited Loading

Choose a reason for hiding this comment

jcfr Apr 8, 2018

Choose a reason for hiding this comment

jcfr Apr 8, 2018

Choose a reason for hiding this comment

jcfr commented Apr 8, 2018

ihnorton commented Apr 9, 2018 via email • edited Loading

ihnorton commented Apr 12, 2018 • edited Loading

pieper Apr 13, 2018

Choose a reason for hiding this comment

pieper Apr 13, 2018

Choose a reason for hiding this comment

fedorov commented Apr 17, 2018

ihnorton commented Apr 17, 2018 • edited Loading

fedorov commented Apr 17, 2018

fedorov commented Apr 30, 2018

ihnorton commented Feb 20, 2018 •

edited

Loading

ihnorton commented Feb 21, 2018 •

edited

Loading

ihnorton commented Mar 2, 2018 •

edited

Loading

ihnorton commented Mar 2, 2018 •

edited

Loading

ihnorton commented Mar 7, 2018 •

edited

Loading

ihnorton commented Mar 8, 2018 •

edited

Loading

lassoan commented Mar 27, 2018 •

edited

Loading

jcfr Apr 8, 2018 •

edited

Loading

ihnorton commented Apr 9, 2018 via email •

edited

Loading

ihnorton commented Apr 12, 2018 •

edited

Loading

ihnorton commented Apr 17, 2018 •

edited

Loading