jpype icon indicating copy to clipboard operation
jpype copied to clipboard

Fixes for 3.11

Open Thrameos opened this issue 3 years ago • 3 comments

Fixes #1086

Thrameos avatar Sep 01 '22 20:09 Thrameos

Codecov Report

Merging #1087 (64223ad) into master (82a7658) will decrease coverage by 0.01%. The diff coverage is 89.28%.

@@            Coverage Diff             @@
##           master    #1087      +/-   ##
==========================================
- Coverage   88.64%   88.62%   -0.02%     
==========================================
  Files         112      111       -1     
  Lines       10247    10207      -40     
  Branches     4012     4014       +2     
==========================================
- Hits         9083     9046      -37     
+ Misses        705      701       -4     
- Partials      459      460       +1     
Impacted Files Coverage Δ
native/common/jp_exception.cpp 79.34% <80.00%> (-0.13%) :arrow_down:
jpype/_jvmfinder.py 69.23% <100.00%> (ø)
native/common/jp_context.cpp 81.53% <100.00%> (-0.09%) :arrow_down:
native/python/pyjp_value.cpp 82.31% <100.00%> (+0.88%) :arrow_up:
native/common/jp_method.cpp 97.68% <0.00%> (-1.16%) :arrow_down:
jpype/__init__.py
jpype/_core.py 95.55% <0.00%> (+0.88%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

codecov[bot] avatar Sep 01 '22 20:09 codecov[bot]

We still have compile errors:

ative/common/jp_exception.cpp: In function ‘PyObject* tb_create(PyObject*, PyObject*, const char*, const char*, int)’:
native/common/jp_exception.cpp:527:9: error: ‘PyThreadState’ {aka ‘struct _ts’} has no member named ‘frame’; did you mean ‘cframe’?
  527 |   state.frame = (PyFrameObject*) prev.get();
      |         ^~~~~
      |         cframe
native/common/jp_exception.cpp:530:9: error: ‘PyThreadState’ {aka ‘struct _ts’} has no member named ‘frame’; did you mean ‘cframe’?
  530 |   state.frame = NULL;
      |         ^~~~~
      |         cframe
native/common/jp_exception.cpp:540:84: error: invalid use of incomplete type ‘PyFrameObject’ {aka ‘struct _frame’}
  540 |  JPPyObject lasti = JPPyObject::claim(PyLong_FromLong(((PyFrameObject*)frame.get())->f_lasti));
      |                                                                                    ^~
In file included from /opt/_internal/cpython-3.11.0rc1/include/python3.11/Python.h:42,
                 from native/common/jp_exception.cpp:16:
/opt/_internal/cpython-3.11.0rc1/include/python3.11/pytypedefs.h:22:16: note: forward declaration of ‘PyFrameObject’ {aka ‘struct _frame’}
   22 | typedef struct _frame PyFrameObject;
      |                ^~~~~~
error: command '/usr/bin/ccache' failed with exit code 1

Perhaps worth enabling CI for Python 3.11 (you might have to use the Python available on the manylinux image, rather than what Azure provide). I note that currently a release of JPype would fail because it would try to build a Python 3.11 wheel (since we glob the Python versions available on manylinux), even though that isn't tested (something we could try to address another time).

pelson avatar Sep 05 '22 07:09 pelson

native/common/jp_exception.cpp:527:9: error: ‘PyThreadState’ {aka ‘struct _ts’} has no member named ‘frame’; did you mean ‘cframe’?

Changes of the [PyThreadState](https://docs.python.org/3.11/c-api/init.html#c.PyThreadState) structure members:

    frame: removed, use [PyThreadState_GetFrame()](https://docs.python.org/3.11/c-api/init.html#c.PyThreadState_GetFrame) (function added to Python 3.9 by [bpo-40429](https://bugs.python.org/issue?@action=redirect&bpo=40429)).

see https://docs.python.org/3.11/whatsnew/3.11.html#id6

rapgro avatar Sep 14 '22 23:09 rapgro

see https://docs.python.org/3.11/whatsnew/3.11.html#id6

Thanks @rapgro, but the problem is that we are setting the frame, not getting the frame in this case. I appreciate that this hasn't been public API in any Python version from Python 2.7. Some interesting context on the motivation at https://github.com/jpype-project/jpype/discussions/1071#discussioncomment-2835357.

pelson avatar Oct 04 '22 07:10 pelson

@pelson I found the issue with the broken test. So I have a fully working copy for 3.11. Now we just need to get the multi version going and we should be good to go.

Thrameos avatar Oct 10 '22 16:10 Thrameos

@marscher Some background on this. Python 3.11 removed a number of internal methods that we were using. These methods were used because the Python API is completely deficient in some areas like handling stack frames or using custom allocators with GC objects. They are planning replacements for what we need but none of the replacements are in 3.11. So they gutted the interface we are standing on, and there is no replacement. Meaning unless I do something drastic we won't support 3.11 at all.

So I had to come up with a few creative kludges. The biggest danger is these kludges will introduce reference leaks as they are doing things like initializing an object twice and swapping the memory layout. They are safer in the sense that I managed to get them to use the public API, but in horrible ways (basically creating using side effects of the public implementations). Hopefully these last until the replacement API is released.

Thrameos avatar Oct 10 '22 16:10 Thrameos

Python 3.11 removed a number of internal methods that we were using. These methods were used because the Python API is completely deficient in some areas like handling stack frames or using custom allocators with GC objects. They are planning replacements for what we need but none of the replacements are in 3.11. So they gutted the interface we are standing on, and there is no replacement. Meaning unless I do something drastic we won't support 3.11 at all.

That's bad news, lets hope that 3.12 will add support for what is needed.

So I had to come up with a few creative kludges. The biggest danger is these kludges will introduce reference leaks as they are doing things like initializing an object twice and swapping the memory layout. They are safer in the sense that I managed to get them to use the public API, but in horrible ways (basically creating using side effects of the public implementations). Hopefully these last until the replacement API is released.

I wonder how bullet proof our leakage testing is. This could be the time to check and eventually improve this. I know this is hard to achieve, but would safe us lots of troubles some day. If we now can compile and run the testsuite on 3.11m, I'd recommend adding it to the testing matrix.

marscher avatar Oct 12 '22 17:10 marscher

Our testing matrix checks common leak paths in objects. This particular spot is on types not general objects, so it only really leaks after the JVM is shutdown which is less of an issue than an object leak. Thus unless you were adding lots and lots of dynamically allocated types form Java then the leak may be a problem. But as we always cache types they are never freed in normal situations anyway.

Thrameos avatar Oct 12 '22 17:10 Thrameos

@Thrameos & @marscher - I'm unavailable for a bit now, and won't be back before the Python 3.11 release. Do you agree that we should merge and release a v1.4.0.post0 wheel, with only a Python 3.11 build? I believe this should work out just fine for existing users, and new Python 3.11 users will get the newer version.

pelson avatar Oct 19 '22 09:10 pelson

@marscher I wpuld look to you regarding this decision.

I would be fine with releasing a new patch or new minor with the fixes.

I can start the build process once you decide which.

Thrameos avatar Oct 19 '22 11:10 Thrameos

I'm also fine with a micro patch release for this kind of change. Sorry for the delay!

marscher avatar Oct 26 '22 21:10 marscher

Will do. Thanks.

Thrameos avatar Oct 26 '22 22:10 Thrameos

native/python/pyjp_value.cpp:

		// Horrible kludge because python lacks an API for allocating a GC type with extra memory
		// The private method _PyObject_GC_Alloc is no longer visible, so we are forced to allocate
		// a different type with the extra memory and then hot swap the type to the real one.

FYI @encukou is working on a Python API for that.

vstinner avatar Nov 03 '22 15:11 vstinner

Yep. Have looked at that API, but it doesn't exist for Py 3.11 so we are stuck because our old method using private functions doesn't work. Worse it appears this kludge may break something in the garbage collector at least in one projects testing. So I need to investigate further this weekend.

Thrameos avatar Nov 03 '22 16:11 Thrameos

Thanks for taking the shot!

Should we try to pin JPype to use Python<3.11 in the meantime, if we run into such horrible subtle errors like garbage collection issues?

marscher avatar Nov 07 '22 11:11 marscher

Thanks for taking the shot!

Should we try to pin JPype to use Python<3.11 in the meantime, if we run into such horrible subtle errors like garbage collection issues?

IMO, if we can engage with the Python RC process, there should be enough time to avoid nasty surprises. If we find that we absolutely can't be compatible with Python 3.12, we can always release something then which has that constraint. In general, upper pins are discouraged (long read).

I think some more dialogue between the C API team and JPype should come out of this :crossed_fingers:, so I truly hope things will get better, not worse, in the future.

pelson avatar Nov 07 '22 14:11 pelson

A first step could be testing against unreleased versions of Python, right? I'm sure they provide a nightly build, which we can integrate into our CI process on a regular basis.

@pelson Thanks for the reference about version pinning. I wasn't aware, that is becoming a disease in the eco system :) How do you suggest to get in touch with the C-API team? If the nightly Python job fails, we could get in touch like on their bug-tracker or IRC, whatever they use nowadays.

marscher avatar Nov 07 '22 15:11 marscher

The first Alpha of CPython 3.12 is already out. I think the alphas/betas would work bit better for you than nightly builds, since they're proper releases that pass the full test suite (i.e. stable buildbots) & have release blockers sorted out.

There's no formal “C API” team, but to get in touch with the relevant people, use the expert-C-API label on the CPython bug tracker, or the just-opened C-API category on Discourse: https://discuss.python.org/c/core-dev/c-api/30

encukou avatar Nov 07 '22 16:11 encukou

Thanks a lot @encukou for these useful hints!

marscher avatar Nov 07 '22 17:11 marscher