search

jpy-consortium / jpy

Apache License 2.0
68 stars 16 forks source link

Status of JSR223 support #66

Open fabrice-ducos opened 2 years ago

fabrice-ducos commented 2 years ago

Hi everyone, thumbs up for the great work.

I am wondering if JPy is currently JSR223 compliant.

The ScriptEngine and ScriptEngineFactory implementations seem to be there. But no JPy engine is detected by the ScriptEngineManager when one tries to load it (I know it, because I can detect many other script engines without problem).

When checking at the jpy-0.10.0-SNAPSHOT.jar content, there is no META-INF/services/javax.script.ScriptEngineFactory, required for detection by the ScriptEngineManager.

Is it an oversight or on purpose?

devinrsmith commented 2 years ago

I think @forman can speak to that. Looking at the javadoc I see this:

Note, jpy's JSR 223 support is not yet functional. This class is only used by unit-tests so far.

forman commented 2 years ago

Well, it's been long time ago... but I think @devinrsmith is right. The JSR 223 support hasn't been fully implemented yet. But I believe, this wouldn't be too much work.

fabrice-ducos commented 1 year ago

Hi folks,

not sure if you have got any progress on this.

During a night of insomnia (caused by the current heat wave), I managed to launch a JSR223 compliant JPy interpreter (tested with the standard jrunscript JDK tool) with hopefully minimal changes in the code base. Basically:

All in all, there must be less than 10 lines of difference with the official code base. You can review them at https://github.com/fabrice-ducos/jpy/tree/jsr223

In a separate branch https://github.com/fabrice-ducos/jpy/tree/jsr223-jpyinterp, I implemented a quick-and-dirty standalone JPy interpreter (jpyinterp.sh) based on jrunscript (a tool available in all JDK6+ installations, under $JAVA_HOME/bin). Since I do not master the way JPy configures its installation path, I had to look for Python and JPy native libraries in rather hackish ways. This is just for experimentation, so I preferred to keep it separate from the jsr223 branch. But it works on Linux and MacOSX (provided it is run from the jpy directory itself).

$ ./jpyinterp.sh 
cpython> print("Hello Jpy")
Hello Jpy
None
cpython>

Tested successfully on:

devinrsmith commented 1 year ago

Thanks for the work. I do have some concerns about the best way to initialize python. I see that the constructor of ScriptEngineImpl calls PyLib.startPython - I wonder if it makes more sense to move PyLibInitializer.initPyLib and PyLib.startPython right before ScriptEngineImpl is constructed in org.jpy.jsr223.ScriptEngineFactoryImpl#getScriptEngine?

fabrice-ducos commented 1 year ago

Hi Devin,

thank you for your answer! I didn't touch ScriptEngineImpl and left the call to PyLib.startPython where it was. This was more an experiment of feasibility than a definitive proposal. PyLibInitializer.initPyLib didn't have to be called more than once (in my understanding), so an initialization block made sense to me. Now I am completely new to this project, and I do not have a global vision to give you a worthy opinion.

Feel free to rearrange the code as you see fit, for the good of the project.

fabrice-ducos commented 1 year ago

After playing a bit more with the experimental jpyinterp.sh interpreter, it looks like there is a bit more work than expected.

While pure Python statements seem to work, the jpy module (for interaction between the JVM and Python) seems to have issues.

Here is a session borrowed from jpy 0.9.0 at https://jpy.readthedocs.io/en/latest/reference.html, and tested with the current version jpy 0.12.0

$ ./jpyinterp.sh 
cpython> import jpy
None
cpython> String = jpy.get_type('java.lang.String')
None
cpython> s = String('Hello jpy!')
java.lang.RuntimeException: Error in Python interpreter:
Type: <class 'TypeError'>
Value: 'java.lang.Class' object is not callable
Line: 1
Namespace: <module>
File: <string>
Traceback (most recent call last):
  File "<string>", line 1, in <module>

    at org.jpy.PyLib.executeCode(Native Method)
    at org.jpy.PyObject.executeCode(PyObject.java:138)
    at org.jpy.jsr223.ScriptEngineImpl.eval(ScriptEngineImpl.java:118)
    at javax.script.AbstractScriptEngine.eval(AbstractScriptEngine.java:264)
    at com.sun.tools.script.shell.Main.evaluateString(Main.java:298)
    at com.sun.tools.script.shell.Main.processSource(Main.java:267)
    at com.sun.tools.script.shell.Main.access$100(Main.java:37)
    at com.sun.tools.script.shell.Main$1.run(Main.java:183)
    at com.sun.tools.script.shell.Main.main(Main.java:48)
devinrsmith commented 1 year ago

I've added a few commits on top of your branch, let me know what you think - see https://github.com/devinrsmith/jpy/tree/jsr223-jpyinterp. I think we could get the script engine changes merged soon if all looks good.

fabrice-ducos commented 1 year ago

Looks good for me! Just not sure about the rationale behind the volatile modifier you added at ScriptEngineFactoryImpl.java:46

While I am sure that you added it for a very good reason, maybe a short comment would be welcome for explaining its necessity?

devinrsmith commented 1 year ago

Mis-implemented double-checked locking patterns are pretty common - here's an article about it: https://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html. Arguably, could be fixed as part of it's own issue / PR, but I've just gone ahead and fixed it here.

fabrice-ducos commented 1 year ago

Thanks. Personally, I like to put a reference (under the form of a short comment, e.g. your reply in the former post, with the link) in the code itself (instead of a commit message) for this kind of clever implementation (not everyone is knowledgeable of these patterns). Of course, the link may become stale, but it is clear enough to be easy to find again (and to be updated) with a search engine.

devinrsmith commented 1 year ago

I've added an additional commit with inline comments; as well as improved the getScriptEngine call.

fabrice-ducos commented 1 year ago

Great! I think it's clear enough now.

While looking at the PyLib_CallAndReturnObject function from src/main/c/jni/org_jpy_PyLib.c,

I unearthed a commented piece of code:

   // Check why: for some reason, we don't need the following code to invoke object methods.
    /*
    if (isMethodCall) {
        PyObject* pyMethod;

        pyMethod = PyMethod_New(pyCallable, pyObject);
        if (pyMethod == NULL) {
            JPy_DIAG_PRINT(JPy_DIAG_F_EXEC, "PyLib_CallAndReturnObject: error: callable '%s': no memory\n", nameChars);
            PyLib_HandlePythonException(jenv);
            goto error;
        }
        JPy_DECREF(pyCallable);
        pyCallable = pyMethod;
    }
    */

Reenabling this snippet (as is, without change) does no good. Unexpectedly, isMethodCall is true even on a statement like print("Hello")

Any chance that it be related with the issue on the jpy module that was illustrated in my post of the 15 june?

cpython> import jpy
None
cpython> String = jpy.get_type('java.lang.String')
None
cpython> s = String('Hello jpy!')
java.lang.RuntimeException: Error in Python interpreter:
Type: <class 'TypeError'>
Value: 'java.lang.Class' object is not callable
devinrsmith commented 1 year ago

Can you link to the issue?

I think the implementation of ScriptEngineImpl may need to be changed to call into python "correctly". Also, I'm not sure how it's decided which method to call into wrt javax.script.ScriptEngine and javax.script.Invocable.

fabrice-ducos commented 1 year ago

By linking to the issue, do you mean this ?

devinrsmith commented 1 year ago

Oh sorry, I thought you meant a different issue. The object is not callable I think can be solved, but I think will take some debugging down into ScriptEngineImpl and maybe more knowledge on my part about JSR223.

When running interactively via python, it looks like this:

>>> import jpy
>>> String = jpy.get_type('java.lang.String')
>>> s = String('Hello jpy!')
>>> s
java.lang.String(objectRef=0x5568d3c6a7e0)
fabrice-ducos commented 1 year ago

Maybe Using Java from Scripts can help.