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

Extension #890

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Extension #890

wants to merge 13 commits into from

Conversation

Thrameos
Copy link
Contributor

This is a work in progress PR for extending classes within Python.

@lgtm-com
Copy link

lgtm-com bot commented Nov 15, 2020

This pull request introduces 23 alerts when merging 7fbc10b into 7256261 - view on LGTM.com

new alerts:

  • 18 for Dereferenced variable may be null
  • 2 for Container contents are never accessed
  • 1 for Useless comparison test
  • 1 for Dereferenced variable is always null
  • 1 for Array index out of bounds

@Thrameos
Copy link
Contributor Author

Thrameos commented Nov 16, 2020

Making progress, but still deep technical issues.

  • We need to be able to call super from the constructor (__init__) and no place else.
  • We need to make sure base __init__ can only be called once.
  • We need to make sure that initial assignments are to "None" or a primitive type.
  • We need to be able keep any methods or fields from being called before the init.
  • We need to grant privilege access to private and protected methods within these implementations. This privilege should not be passed to copies or methods called from implementation if possible.
  • We need to handle exceptions.
  • We need to add @JThrows to define exceptions.
  • We need to aggressively catch errors before the classloader.
  • We need a special class loader for these types.

It is still not clear if we can invokespecial from JNI.

@lgtm-com
Copy link

lgtm-com bot commented Nov 16, 2020

This pull request introduces 20 alerts when merging 472b300 into 7256261 - view on LGTM.com

new alerts:

  • 18 for Dereferenced variable may be null
  • 1 for Useless comparison test
  • 1 for Array index out of bounds

@lgtm-com
Copy link

lgtm-com bot commented Nov 16, 2020

This pull request introduces 20 alerts when merging 3855389 into 39ae320 - view on LGTM.com

new alerts:

  • 18 for Dereferenced variable may be null
  • 1 for Useless comparison test
  • 1 for Array index out of bounds

@Thrameos
Copy link
Contributor Author

I added documentation on the initial guess of where I think the Java extension system will be headed. Any input on the direction would be appreciate.

https://github.com/jpype-project/jpype/pull/890/files#diff-f55f8f25b97e5081e65a0e0db39f6c98ed08d4deea664b3137b9ce3ea52c1245

@lgtm-com
Copy link

lgtm-com bot commented Nov 16, 2020

This pull request introduces 20 alerts when merging c3facdd into 39ae320 - view on LGTM.com

new alerts:

  • 18 for Dereferenced variable may be null
  • 1 for Useless comparison test
  • 1 for Array index out of bounds

@marscher
Copy link
Member

This lists reads like we need a complete re-implementation of essential Java features. Ain't that a bit of an overkill for this feature? What do you think will be the expected performance, when cross-calling between the languages during handling extended classes?
I guess you have already a path to the truth lying around, and I want to be enlightened as well :)

@Thrameos
Copy link
Contributor Author

Extending a java class remains a requested feature. Some APIs in Java require a class to be extended. We can achieve this by writing a custom Java class and a stub interface that then has to loaded with the user code. But that is a real pain.

This is the alternative where the user declares a Java extension in Python which is "compiled" at runtime. The performance of the compile is not great but you should not do it often. The actual call cost is the same as the existing Java JProxy code. Everything must be boxed. Each gets a Java handle for Python to use. Then Python gets called. The return has to be boxed again. Then we return to Java space. So lets assume 5 arguments with 2 primitives. That is 2 box objects then 5 python objects, the one java object on the return. We also have the tuple for the call transfer. So cost is 9 object creation. Compare this to one python method call. One object for the bind. One for the tuple. Several more if keyword args. So a Java to Python call is a slightly more heavy than a Python call and much more than a native Java call.

@Thrameos
Copy link
Contributor Author

Setting up the privilege flag is challenging. I need to be able to check the flag whenever a private method is called and verify the flag, but there is no space in the object currently for it (because there memory layout.)

Options are:

  • Create a second dynamic type for the class which holds the private fields and methods. The assign the first argument to it rather than the actual type of the object. Only extension objects require this second type but it does require restructuring of class creation functions to generate 2 classes rather than one.
    This only allows access to private methods, and fields rather protected as the tree is unlike to handle a second protection branch. The downside is that anything can capture this object and then get privileged access. Not really what I intend.
  • use the object dictionary to set the privilege flag then then the flag whenever a private method or field is called. This has the downside that it forces a dictionary on every object and costs a lot of time in lookups. Of course this is only used when privileged object is created so the memory and time would be reasonable.
  • add another storage spot in the JPValue for the privilege state as flags. That is a lot of memory for a new feature given every object will need it. But this would be O(1). There may be other features that will require a flags field on JPValue.
  • Hack a space for it in Python structures. This would be non portable.
  • fail to enforce privilege. Well this has significant downsides in terms of allowing private fields or methods to be called.
  • Create a special "This" type that points to another object and unpacks to the original, redirects method and field lookups to the original. This would need some work on the type matching system as it needs to be recognized as the original object when used as a call. The downside would be the type of the object when reporting would not be the type expected. It can also be captured which breaks down the privilege model.
  • Force privilege access to go through a special member such as "private" when can delegate the binding. This has most of the downsides of a special class for the call.
  • Use a double redirection scheme which places the privilege instead of in the object but on the stack. In this case we call an function which adds to the Python call stack the emitter. When we want to check for privileged access we poll the stack to see if we were called from the privileged method. This would work so long as one method does not call another. If it does then the privilege is lost. This has the advantage that privilege is automatically lost when the method exits regardless of the reference or the path taken. This has the upside that privilege is not granted to a method which is called from the internal.

@Thrameos Thrameos added the enhancement Improvement in capability planned for future release label Nov 26, 2020
@Thrameos Thrameos self-assigned this Nov 26, 2020
@Thrameos Thrameos added this to the JPype 2.0.0 milestone Nov 26, 2020
@astrelsky
Copy link
Contributor

astrelsky commented Sep 22, 2024

Making progress, but still deep technical issues.

  • We need to be able to call super from the constructor (__init__) and no place else.
  • We need to make sure base __init__ can only be called once.
  • We need to make sure that initial assignments are to "None" or a primitive type.
  • We need to be able keep any methods or fields from being called before the init.
  • We need to grant privilege access to private and protected methods within these implementations. This privilege should not be passed to copies or methods called from implementation if possible.
  • We need to handle exceptions.
  • We need to add @JThrows to define exceptions.
  • We need to aggressively catch errors before the classloader.
  • We need a special class loader for these types.

It is still not clear if we can invokespecial from JNI.

I did some work on this over the weekend. I got as far as successfully extending a Java class, constructing this Python class from Python, subclassing that Python class, and then constructing that class. I didn't expect subclassing the Python class to work, but it did and worked just like the original.

  • We need to be able to call super from the constructor (__init__) and no place else.

  • We need to make sure base __init__ can only be called once.

  • We need to be able keep any methods or fields from being called before the init.

  • We need to grant privilege access to private and protected methods within these implementations. This privilege should not be passed to copies or methods called from implementation if possible.

  • Calling super().__init__() has no effect so it is irrelevant.

  • Accessing methods or fields without an instance produces the same effect as attempting to do so on any other class.

Still need to do the privilege stuff but honestly, that can be done at the end. The way I see it is if it's not possible to prevent it, then oh well..

Need to be able to add annotations to the class. This isn't difficult, there is just no pythonic way of doing it yet. Maybe in the future PEP 638 can help with that. My use case would actually requires being able to add annotations to the class. I don't care if it's gross as long as it works. I've done it in the past when I hacked around jpype limitations from an external python module, i just haven't implemented it here yet.

It is still not clear if we can invokespecial from JNI.

I'm not sure what this is needed for.

I had to reformat some of the Java files because I couldn't stand it anymore. I also took care of the 100+ warnings about unparameterized generics, resource leaks, etc.

Oh and I made it so you can add python attributes to the class that aren't visible to Java. Which can be important considering it "is a Python class".

@astrelsky
Copy link
Contributor

@Thrameos how did you intend for declaring fields to work? I'm was trying to implement test cases for adding and accessing fields in an extended class but the intent isn't clear. You can't put a decorator on a class variable. It could be done by decorating a property or maybe by an annotation by having JPublic be a class and implement __class_getitem__. I think I like the annotation.

@Thrameos
Copy link
Contributor Author

Thrameos commented Oct 5, 2024

I believe it should work no different than the proxy. You create the stub class which forwards to the interface and an interface to implement. You then Create a proxy. The tricky part is connecting the proxy class. The best policy would be to rename the overriden methods so that user called points go back to Java which route back to Python. The Java declared fields would appear on the "super", the Python declared ones is more challenging. You don't declare fields in python, they just appear in the constructor. That means that we need to make the Python wrapper open to adding new fields. We could make a "this" in addition to the "super" which is specifically for holding the fields if it would keep the class closed.

@astrelsky
Copy link
Contributor

astrelsky commented Oct 5, 2024

I believe it should work no different than the proxy. You create the stub class which forwards to the interface and an interface to implement. You then Create a proxy. The tricky part is connecting the proxy class. The best policy would be to rename the overriden methods so that user called points go back to Java which route back to Python. The Java declared fields would appear on the "super", the Python declared ones is more challenging. You don't declare fields in python, they just appear in the constructor. That means that we need to make the Python wrapper open to adding new fields. We could make a "this" in addition to the "super" which is specifically for holding the fields if it would keep the class closed.

I did some testing with the annotation and noticed a useable pattern right when I was about to give up. You can do the following;

def testPublicField(self):
    class MyObject(JClass("jpype.extension.TestBase")):

        myField: JPublic[JInt]

        def __init__(self):
            self.pythonMember = None

        @JPublic
        def __init__(self):
            # java exposed constructor, called before python __init__
            ...


    o = MyObject()
    o.myField

When _JExtension is called, __annotations__ has already been filled out so the fields can be properly created before the Java class is generated.

image

At the moment I have all this functional, with some minor things broken elsewhere that I have to go back and fix.
https://github.com/astrelsky/jpype/blob/extension/test/jpypetest/test_extension.py

Edit: Ignore the "FIXME" in testInitOnce, I already fixed it just forgot to remove the comment.

@astrelsky
Copy link
Contributor

astrelsky commented Oct 6, 2024

I've gotten quite a bit done on this. I think all I have left to implement is make some way to apply annotations, even if it is crufty (my use case requires it at least on the class). After that it's just document everything. I'm currently flying blind with respect to test coverage and leaks since test_leak.py and test_leak2.py are skipped.

I expect to have something presentable in a few more weekends. Once it is there, how would you prefer it to be submitted? Should I open a pull request to Thrameos's fork or should I open a new pull request?

Oh, I also wanted to mention that I switched to testing on 3.13rc3 this morning so I'm pretty confident things are working well for 3.13, at least in normal GIL mode. I don't expect the experimental free-threaded mode to work since heap types aren't shared and the global type pointers in jpype will probably throw a wrench into that. The experimental free-threaded mode is disabled by default and you have to explicitly start that version of python and build your wheels with that version. This means it is unlikely many people will want it, so there is probably no need to care until someone complains about it.

@Thrameos
Copy link
Contributor Author

Thrameos commented Oct 6, 2024

Either way works for me. I ma make some changes to optimize pathways but lets wait until it is submitted before making any decisions. As I understating it you are adding JPublic and JProtected to the API and the rest of the mechanics is on the class new. Is that correct?

@astrelsky
Copy link
Contributor

Either way works for me. I ma make some changes to optimize pathways but lets wait until it is submitted before making any decisions. As I understating it you are adding JPublic and JProtected to the API and the rest of the mechanics is on the class new. Is that correct?

Yes

@astrelsky
Copy link
Contributor

I would like to know what the deal is with marking entire *.cpp files as extern "C". This is a great way to shoot yourself in the foot with exceptions. As far as I can tell the only things that need to have c linkage are the native java methods and the python module unit.

@Thrameos
Copy link
Contributor Author

Thrameos commented Oct 7, 2024

Mostly historical. The entire python module was grown out of a single Python hello-world application which unfortunately had extern "C" declared. Sections were moved out as needed. As far as I am aware the fast majority of those calls are directly connected to Python slots so they really need to be extern "C". We can of course mark individual functions rather than blanket, but that will take some effort.

@astrelsky
Copy link
Contributor

Mostly historical. The entire python module was grown out of a single Python hello-world application which unfortunately had extern "C" declared. Sections were moved out as needed. As far as I am aware the fast majority of those calls are directly connected to Python slots so they really need to be extern "C". We can of course mark individual functions rather than blanket, but that will take some effort.

This is the situation I was mostly concerned about and it appears to be moot. I expected this to terminate but it doesn't. https://gcc.godbolt.org/z/fxPKzxhhK

I'll leave it alone but I won't pretend like it doesn't bother me 😂.

@astrelsky
Copy link
Contributor

Oh, I also wanted to mention that I switched to testing on 3.13rc3 this morning so I'm pretty confident things are working well for 3.13, at least in normal GIL mode. I don't expect the experimental free-threaded mode to work since heap types aren't shared and the global type pointers in jpype will probably throw a wrench into that. The experimental free-threaded mode is disabled by default and you have to explicitly start that version of python and build your wheels with that version. This means it is unlikely many people will want it, so there is probably no need to care until someone complains about it.

I retract this statement. It seems I had this mixed up with PEP 684. I've enabled it the free threaded build and it seems to work fine. I'll put in a pr shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement in capability planned for future release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants