Add an object for SciJava scripting with Python

[ctrueden]

This is an update of @karlduderstadt's awesome work on imagej/pyimagej#159. The goal is to make it super easy to bootstrap the org.scijava:scripting-python script language plugin. It will require a little bit of adjustment on the scripting-python side, since with this version of the work, the named ScriptingPythonRunner in the ObjectService is now a java.util.function.Function which expects to be handed an object with specific baked-in fields (script, vars, and scriptContext).

I am filing this a draft for now, because we need to test it before it can be merged. There will be corresponding PRs to scripting-python and pyimagej with same branch name python-script-runner; the idea will be to use them all locally together and see what explodes!

[karlduderstadt]

Looks great! I am curious about a better way to manage stdout in conjunction with the script runner. When I have used it, I struggled with this issue and using the threads for all running scripts seemed like the only way to ensure output was correctly redirected when several scripts were running at the same time and to avoid writing other outputs to the script editor log that were not related to running scripts. But it certainly doesn't feel good taking over the stdout entirely. At the same time python doesn't seem to provide any fine control as far as I have seen.

[ctrueden]

Hmm, there is context_lib.redirect_stdout, but:

Note that the global side effect on sys.stdout means that this context manager is not suitable for use in library code and most threaded applications. It also has no effect on the output of subprocesses. However, it is still a useful approach for many utility scripts.

Given Python's limitations here, I find what you did to be really elegant, actually, with using threads to track which output goes to which script context. Now that I've studied it more, I think we can leave it the way it is; I'm happy with it. If/when something better comes along in Python core or as a robust third-party library, we can switch.

[karlduderstadt]

I see you removed the inputs for ij and sj. I think I added those to address some problems with String conversion at some point that are now fixed. It was messy to include those and I am glad you removed them.

Do I have it right that if someone specifies ImageJ as a script parameter input they would get exactly the same thing as ij as seen throughout the PyImageJ world?

The following script should still work, correct?

#@ ImageJ ij

# Load an image.
image_url = 'https://imagej.net/images/clown.jpg'
jimage = ij.io().open(image_url)
image = ij.py.from_java(jimage)

Obviously, if the image was open in Fiji and specified as a script parameter input, it would already be converted. But it would be nice if many things from the PyImageJ tutorials worked without changes.

[ctrueden]

@karlduderstadt Ooh, good point about #@ ImageJ ij. No, I don't think you could call ij.py from that input (but I am not 100% sure... I will test). But I have an idea how to make it work, using a PreprocessorPlugin implemented on the Python side with higher priority than the normal GatewayPreprocessor. 😄 I'll try to implement it when I work on the changes to scripting-python soon.

Edit: thinking about this more, I think it's likely that #@ ImageJ ij will give back an object that has the .py extensions, thanks to how JPype's @JImplementationFor works. I'll still test it of course, and implement my idea if needed.

[ctrueden]

@karlduderstadt There is now a corresponding python-script-runner branch of scripting-python, with the changes needed to match the implementation here in this PR. Comments:

• In doing this work, I found some problems with the code here, and force-pushed an update, so if you test it, make sure you have the latest version of both repositories, and pip install -e . this branch of scyjava into your conda environment. You can then run scripting-python's repl.sh script to launch the SciJava ScriptREPL preconfigured to the Python script language. It ostensibly works, but I didn't test it in the Script Editor yet with actual SciJava script inputs and outputs.
• I had to tweak how the exec/eval logic worked, because exec does not return any value, but the SciJava script framework assumes that evaluating a script will return the value of the last expression in the executed block; fortunately, I found a way to do this using Python's built-in ast module.
• It seems the ScriptREPL does not attach an error writer to the script context, and so the logic on the Python side that dumps exceptions when something goes wrong during evaluation was throwing an NPE in my tests. I put a guard around it, but maybe we should change something in SciJava Common? Because right now, if you try executing something with faulty syntax, it just returns None without any error messages.

Anyway, let me know what you think. I will be out of the office for the next few days due to USA Thanksgiving, but look forward to hearing how it goes for you once I am back. Getting closer! :+1:

[karlduderstadt]

@ctrueden I am terribly sorry I dropped the ball here. I got completely consumed with grant writing, among other things. I am sorry for the lack of response. I would like to help make sure this work gets finished.

Originally, I thought it was good to have no return value. I was worried this would often result in error messages because there are many types that are not fully supported. I figured it was better to have the user specify the outputs they wanted. However, I realize that for the other scripting languages they always return a value, even when nothing is specified as an output.

I also didn't know how to do it. Great that you solved it. I will start testing as you suggested and let you know how it goes.

Anything else I should test/consider/work on?

[karlduderstadt]

• It seems the ScriptREPL does not attach an error writer to the script context, and so the logic on the Python side that dumps exceptions when something goes wrong during evaluation was throwing an NPE in my tests. I put a guard around it, but maybe we should change something in SciJava Common? Because right now, if you try executing something with faulty syntax, it just returns None without any error messages.

Ok, I got the ScriptREPL running and I also see this issue.

@ctrueden Do you think it would be difficult to go ahead and test out the script engine using PyImageJ in gui mode?

Ultimately, the context has to be injected in scyjava somewhere in PyImageJ, right?

scyjava.enable_python_scripting(context)

Should I make a branch over there with the required changes? I am just not sure where that should be added.

Otherwise, I could adapt your shell script to launch Fiji at the end, I suppose.

[karlduderstadt]

To allow for testing in Fiji, I created a launcher script with the following line added right after creating the gateway:

scyjava.enable_python_scripting(ij.IJ.runPlugIn("org.scijava.Context", ""))

@ctrueden Is there a better way to get the context? At some point this should live in PyImageJ. I think it should be in __init__.py in the _create_gateway() function, somewhere after ij = ImageJ(). For now I can use my launcher script for testing.

In any case, I have been testing with the following simple script

#@OUTPUT net.imglib2.python.ReferenceGuardingRandomAccessibleInterval imageOut
import numpy as np

imageOut = np.tile(np.arange(15), (15, 1))

So far I encountered mainly one issue. It seems the way you changed the logic of handling outputs results in errors for all scripts. https://github.com/scijava/scyjava/blob/06bee3344a031c425c43ff262f07d77566af35af/src/scyjava/_script.py#L97-L102

At a minimum, I always get the following error message:

Traceback (most recent call last):
  File "/Users/karlduderstadt4/git/scyjava/src/scyjava/_script.py", line 100, in apply
    arg.vars[key] = to_java(script_locals[key])
  File "/Users/karlduderstadt4/git/scyjava/src/scyjava/_convert.py", line 170, in to_java
    return _convert(obj, java_converters, **hints)
  File "/Users/karlduderstadt4/git/scyjava/src/scyjava/_convert.py", line 72, in _convert
    return prioritized.convert(obj, **hints)
  File "/Users/karlduderstadt4/git/scyjava/src/scyjava/_convert.py", line 65, in convert
    else self.converter(obj)
  File "/Users/karlduderstadt4/git/scyjava/src/scyjava/_convert.py", line 82, in _raise_type_exception
    raise TypeError("Unsupported type: " + str(type(obj)))
TypeError: Unsupported type: <class 'module'>

because the first input/output is always the ScriptModule, which is not a supported type. For more complex scripts there are tons of exceptions for all the python types that are not supported.

Previously, I had used the following logic:

outputs = {}
for key in inputs.keys():
    if key not in inputKeys:
        try:
            outputs[key] = ij.py.to_java(inputs[key])
        except:
            pass
        else:
            #outputs must be placed back in vars
            #to ensure script parameter outputs are returned
            vars[key] = outputs[key]

So that outputs that are not supported are passed over and simply not included without an exception being thrown. Also, I excluded the inputs. What should we do to avoid exceptions? Do we go back to my logic or use yours with other changes?

I don't see how to_java can support all python types.

[ctrueden]

@karlduderstadt Thanks for pushing on this. I just got back from a hackathon in Milan. On my immediate priority list is writeups and loose ends from that, as well as finally getting ImageJ2 v2.10.0 and Fiji v2.10.0 released, so that we can release napari-imagej 0.1.0. I am highly motivated to get this Python SciJava scripting work merged and released also; I would like to do so as part of the 2.10.0 releases, but I'm not sure yet whether I will be able to do that. There is a lot going on right now. I'll do my best to follow up on your messages here as soon as I can—just wanted to let you know that I am seeing them and it's on my radar to focus on this again soon.

About the unsupported types... my first idea would be to return some simple Java object that merely has the Python object as a property. Maybe something like:

@JImplements('java.util.function.Supplier')
class PythonThing:
  def __init__(self, obj):
    self.obj = obj
  @JOverride
  def get(self):
    return self.obj

I didn't test this, and it might be overly complex, or useless. But I like the idea of returning something for each output, if only to mark that the Python code did have something, even if it's not very accessible on the Java side.

[karlduderstadt]

@ctrueden thank you very much for the fast response and explanation! I really appreciate it. It would be amazing if this could make it into v2.10.0 but I understand there are other high priority projects to coordinate. I will keep testing in the hopes we can get it there.

About the unsupported types... my first idea would be to return some simple Java object that merely has the Python object as a property. Maybe something like:

@JImplements('java.util.function.Supplier')
class PythonThing:
  def __init__(self, obj):
    self.obj = obj
  @JOverride
  def get(self):
    return self.obj

I see how it would be nice to always return something. Do you think this code should live in the to_java method or directly in _script.py (in the lines I highlighted below) ? https://github.com/scijava/scyjava/blob/06bee3344a031c425c43ff262f07d77566af35af/src/scyjava/_script.py#L97-L102 Once I know where to put it I can try it.

I discovered two more issues:

1. Variables defined in the last line of scripts are not added to script_locals and returned in arg.vars. The result is certainly returned at the end of the function, but defined output variables are not added to arg.vars.
2. Specified outputs are not displayed. Somehow they are not correctly passed for display. I guess this is a problem in scripting-python? But I don't understand why...

For testing, I added the following print statements to your code in _script.py:

return_value = None
try:
    # NB: Execute the block, except for the last statement,
    # which we evaluate instead to get its return value.
    # Credit: https://stackoverflow.com/a/39381428/1207769

    block = ast.parse(str(arg.script), mode="exec")
    last = None
    if len(block.body) > 0 and hasattr(block.body[-1], "value"):
        # Last statement of the script looks like an expression. Evaluate!
        last = ast.Expression(block.body.pop().value)

    _globals = {}
    exec(compile(block, "<string>", mode="exec"), _globals, script_locals)
    print("exec keys: ", script_locals.keys())
    if last is not None:
        return_value = eval(
            compile(last, "<string>", mode="eval"), _globals, script_locals
        )
        print("eval keys: ", script_locals.keys())
        print("return_value", return_value)
except Exception:
    error_writer = arg.scriptContext.getErrorWriter()
    if error_writer is not None:
        error_writer.write(to_java(traceback.format_exc()))

When I run the following script:

#@OUTPUT net.imglib2.python.ReferenceGuardingRandomAccessibleInterval imageOut
import numpy as np

imageOut = np.tile(np.arange(3), (3, 1))

I get the following output

Started New_ at Thu Feb 16 16:14:49 CET 2023
exec keys:  dict_keys(['org.scijava.script.ScriptModule', 'javax.script.filename', 'np'])
eval keys:  dict_keys(['org.scijava.script.ScriptModule', 'javax.script.filename', 'np'])
return_value [[0 1 2]
 [0 1 2]
 [0 1 2]]

As you can see, imageOut is not added to script_locals after eval, but there is a return_value. Also, nothing is displayed in Fiji. If I remove the line #@OUTPUT net.imglib2.python.ReferenceGuardingRandomAccessibleInterval imageOut then the image is displayed.

When I run the following script:

#@OUTPUT net.imglib2.python.ReferenceGuardingRandomAccessibleInterval imageOut
import numpy as np

imageOut = np.tile(np.arange(3), (3, 1))
x = 10

I get the following output

Started New_ at Thu Feb 16 16:38:02 CET 2023
exec keys:  dict_keys(['org.scijava.script.ScriptModule', 'javax.script.filename', 'np', 'imageOut'])
eval keys:  dict_keys(['org.scijava.script.ScriptModule', 'javax.script.filename', 'np', 'imageOut'])
return_value 10

As you can see, exec does add imageOut when in the second to last line. I guess this is because x = 10 is a statement and not an expression, so it can only be run by exec to retain a definition in script_locals.

This seems to be a major downside to the approach. I guess the real issue is that ast.Expression shouldn't say it is an expression when it is clearly a statement.

EDIT: I think the problem is with hasattr(block.body[-1], "value") that returns true also for assignments. Need to change that somehow.

[karlduderstadt]

Aaah checking if ast found an Assign object on the last line works:

if len(block.body) > 0 and hasattr(block.body[-1], "value") and not isinstance(block.body[-1],ast.Assign):

I will keep working on the other problems and then try and commit some changes.

[karlduderstadt]

@ctrueden I tested your idea by adding a PythonObjectSupplier class to _script.py and just returning wrapped objects for unsupported types resulting in an exception. In my tests it works! Of course, I didn't try to use those objects yet for anything on the java side, but at least there are no exceptions and something is always returned.

I went ahead and added a fix to avoid eval processing of assignments on the last line of scripts in commit 0ecc8d6 and the PythonObjectSupplier in commit 46a78fa.

I also realized my test script didn't have the correct return type and that was why it wasn't displayed in Fiji. Using the following updated script, it works:

#@OUTPUT net.imglib2.img.ImgView imageOut
import numpy as np

imageOut = np.tile(np.arange(15), (15, 1))

Maybe this is due to changes to scyjava since I first wrote it. So all the issues I mentioned above appear to be addressed now.

I will keep testing but so far it is now working as expected.

[karlduderstadt]

🎉 🍾 🍰 !

[imagesc-bot]

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/fiji-conda/59618/27

[ctrueden]

I released a new version of scyjava: version 1.9.0. Available now on PyPI; should propagate to conda-forge soon.