ninia / jep

Embed Python in Java
Other
1.27k stars 146 forks source link

Support for keyword on methods #501

Open patrickdalla opened 9 months ago

patrickdalla commented 9 months ago

When I call a java class method with keywords I receive "Keywords not supported". I saw that the issue "#57" solved this issue for functions, why not for classe methods?

patrickdalla commented 9 months ago

One solution that would solve my specific use case necessity would be to ignore keywords parameters instead of raising an error.

ndjensen commented 9 months ago

57 added the method Jep.invoke(String methodName, Map<String, Object> kwargs) and some variations of that. Are you asking if you could write Python code and call Java methods with keywords args? Like

from java.util import ArrayList
ArrayList().add(element="abc", index=0)

I specifically reversed the args to indicate it has to work with keyword args to work. I don't think we can implement that because there's no public methods on java.lang.reflect.Method to get at argument names. And depending on how the code was compiled, the argument names may not be present either.

As for ignoring keyword args, I don't think that's good practice to just silently toss out arguments that a developer passed to a method.

patrickdalla commented 9 months ago

My need: I need to "override" a python library class, with a java class. Every other code of this library, when calling the original class, would transparently call the java class. I could implement this with a specific FileLoader registered before JEP file loader in sys.meta_path. The remaining problem is that the original python class have method with keywords, and many other code call these methoed with these keywords, and JEP does not support it. I made some changes, and implemented a python Wrapper class of this java class, so it removes these keywords and call the java class without them. But it would be really more straightforward and simple if JEP could support this keywords, or ignore them, as the default functionality of these methods (with default keyword values) could still be executed.

patrickdalla commented 9 months ago

Maybe the keywords could be passed as a dict object, as the last parameter. JEP could try to find a method with a Map as the last parameter and pass this dict. If not found, it could still try to call a method ignoring the keywords.

patrickdalla commented 9 months ago

I am using a code (ALeapp) that extracts many data and format them as HTML. There is a python class to format as HTML that accepts these artifacts. As I need only the data, not the HTML, I created a Java class and with the hook I replaced the original HTML formatter class with my java class. The problem occurred because the most important method accepts keywords, and when informed JEP raises this error. When the method is called without any informed keywords, it works perfectly. In my case, as the keywords are options on how to format the HTML, I don't need them at all.

patrickdalla commented 9 months ago

I could reach my objective with a python wrapper to my java, but it took me a lot more effort and time than simply replacing the class with the java class.

patrickdalla commented 9 months ago

By the way, all this functionality this could also be an improvement of JEP. Take a look at https://github.com/sepinf-inc/IPED/blob/ALeappTask/iped-engine/src/main/java/iped/engine/task/leappbridge/PythonHook.java Python hook installs a FileLoader in sys.meta_path of a JEP engine that intercepts the module loading. Successive calls to this python hook can be used to register some intended modifications on original source code before compilation. Method overrideModuleFunction replaces a function definition so it calls a java method. Method overrideClass replaces all the class.

PythonHook pt = new PythonHook(jep); pt.overrideModuleFunction("scripts.ilapfuncs", "logfunc", LeappBridgeTask.class.getMethod("logfunc", String.class)); pt.overrideModuleFunction("scripts.ilapfuncs", "logdevinfo", LeappBridgeTask.class.getMethod("logdevinfo", String.class)); pt.overrideClass("scripts.artifact_report", "ArtifactHtmlReport", ArtifactJavaReport.class);

I noted the limitation of method calling with keywords. So for my case, the overrideClass wasn't enough. So I created the overrideHtmlReport to make a wrapper to the java class, but it is not generalized and is a specific implementation for the case.

patrickdalla commented 9 months ago

I have made some change to the last referenced file, including moving to a new package. So this is the right link: https://github.com/sepinf-inc/IPED/blob/ALeappTask/iped-utils/src/main/java/iped/utils/pythonhook/PythonHook.java

patrickdalla commented 9 months ago

I could write a PythonWrapper to skip any kargs. I will try now implement some way to pass them as a Map or any other java object.

jsnps commented 8 months ago

Hey @ndjensen, @bsteffensmeier,

do you see a way with the existing infrastructure to hook an invocation handler on pyjmethod or some kind of wrapper around the methods on pyjtype? Or what would be the most elegant way to allow for a custom invocation handler? If this is simple enough to add, it could be a good foundation for args, kwargs signature support. The method arbitration, which currently happens in pyjmultimethod, could then be (optionally) handled on the java side and extended with any desired smartness on handling arguments (e.g. implicit conversions). I guess, for performance reasons, it would make sense to make this only optional and decide either per method or per type and already at pyjtype creation time. E.g. multiple invocation handlers could be registered and queried, if one supports a certain method, we register a generic invocation handler method in the slot calling that specific handler. We could also think of a protocol to allow to "cancel" the interception and fallback to the standard way.

What are your general thoughts about this? Would it make sense at all? Do you see any problems with it? Can you already think of a way to achieve something like this with the current jep implementation? I already thought of controlling what is handed to the jep side and create dynamic proxies before passing them to the jep side, but I'm not sure what implications this has with regards to the pyjtype creation. I like this explicit mapping between java types and the dedicated python types. If everything becomes a proxy, this might become a bit messy and/or have negative side effects.

bsteffensmeier commented 8 months ago

@jsnps your comments seem to be more closely aligned with #313. I have some ideas about invocation handler that I will put on that issue.

For kwargs support I still think it would be best handled by an annotation on the method or the parameters in java and then the pyjmethod can have a flag that would make it convert the kwargs dict to Map as the last arg. I don't think that would be very difficult to implement. From the java side I am talking about something that looks like this:

public class Test {
    @JepKwarg
    public void someMethod(Object regularArg,  Map<String,Object> kwargs){
    }
}

or

public class Test {
    public void someMethod(Object regularArg,  @JepKwarg Map<String,Object> kwargs){
    }
}

Another thought I have is that with the way our conversions are setup in the native code we could easily allow pyjmethod to convert the kwargs to any Java object that is compatible with dict. Right now I think that would just be Map or PyObject but if anyone wants to delegate back to python functions then PyObject would be more efficient than Map and PyObject would also allow more customizable conversion of the values if the default conversions aren't right. So theoretically any java class could pick either way(or both) to handle kwargs):

public class Test {
    public void someMethod(Object regularArg,  @JepKwarg Map<String,Object> kwargs){
    }
    public void someMethod(Object regularArg,  @JepKwarg PyObject kwargs){
    }
}
bsteffensmeier commented 7 months ago

@patrickdalla can you look at #510 and see if that PR would help your use case. I added an annotation that can be added to amethod so that jep will automatically convert kwargs to the last Java argmument.

patrickdalla commented 7 months ago

Hi @bsteffensmeier, sorry for the delay. Yes it seems to help me. I couldn't test it because I solved with the workaround informed before, but as soon we update our JEP dependency we will use this.

Thanks.