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

Add support for GraalPy #5380

Merged
merged 7 commits into from
Oct 7, 2024
Merged

Conversation

msimacek
Copy link
Contributor

Description

Hi, I'm a GraalPy developer and I'd like to add support for GraalPy to pybind11. We've been using pybind11 with GraalPy for quite a while, but we relied on a downstream patch. Most of the changes to non-test code just add another condition to existing ifdefs that were there for PyPy. In order to pass the tests on GraalPy, I had to skip several tests that rely on reference counting and finalizers, because unfortunately it's not really possible to always reliably trigger the GC-dependent finalizers at a specific point on GraalPy. I also marked a few failing tests with xfail for now if they seemed like bugs that should be fixed on GraalPy's side. I'll try to look into fixing those in the near future. If you're not opposed, I'd also like to try adding current version of GraalPy to the CI matrix.

CC @timfel @steve-s

Suggested changelog entry:

Added support for GraalPy Python implementation (https://github.com/oracle/graalpython)

@henryiii
Copy link
Collaborator

CI addition sounds good, thanks!

@msimacek msimacek force-pushed the msimacek/graalpy-support branch 7 times, most recently from 279bd89 to da4ecd8 Compare September 20, 2024 12:51
@msimacek msimacek force-pushed the msimacek/graalpy-support branch 4 times, most recently from 7ad7a9d to 9b337ef Compare October 3, 2024 14:44
@msimacek
Copy link
Contributor Author

msimacek commented Oct 3, 2024

Hi @rwgk, I finally got the CI working. The issue with the "unstable" (maximum internals version) tests was because in GraalPy PyCapsule_GetName currently returns a copy of the name and pybind11 does pointer comparison with the original string it passed in when creating the capsule. I'll fix that for the next release of GraalPy, but for now, I disabled using the capsule names on GraalPy (like this). Let me know if that's ok. I could also do a full string comparison, but that sounds like it could kill performance. Other than that, I tried to "localize" the skipping of GC/refcount-dependent test parts, as you requested.

# assert cstats.move_constructions >= 0 # Don't invoke any
assert cstats.copy_assignments == 0
assert cstats.move_assignments == 0
if not env.GRAALPY:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about using:

if env.GRAALPY:
pytest.skip("ConstructorStats is incompatible with GraalPy.")

here (and in similar places elsehwere)? — Then we don't have to indent the entire block below.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That will report that it skipped the whole test, when half of it did run. If you do that, then you could just put it as a decorator and actually skip the whole test. Maybe it's possible to split this in half?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tried out my suggestion locally. The pytest output looks fine to me (ignoring the existing print noise):

test_buffers.py ................ss.........### test_submodule_buffers(module_&)::Matrix @ 0x3175a00 destroyed 2x3 matrix
### test_submodule_buffers(module_&)::Matrix @ 0x3175b80 destroyed 5x4 matrix
                                                                                                [100%]

============================================================ short test summary info =============================================================
SKIPPED [1] test_buffers.py:86: ConstructorStats is incompatible with GraalPy.
SKIPPED [1] test_buffers.py:125: ConstructorStats is incompatible with GraalPy.
========================================================= 25 passed, 2 skipped in 0.02s ==========================================================

That will report that it skipped the whole test

What makes you think that?

The main motivation for my suggestion was that the diff will be less noisy:

diff --git a/tests/test_buffers.py b/tests/test_buffers.py
index 535f33c2..8f0c849e 100644
--- a/tests/test_buffers.py
+++ b/tests/test_buffers.py
@@ -82,6 +82,9 @@ def test_from_python():
         for j in range(m4.cols()):
             assert m3[i, j] == m4[i, j]
 
+    if env.GRAALPY:
+        pytest.skip("ConstructorStats is incompatible with GraalPy.")
+
     cstats = ConstructorStats.get(m.Matrix)
     assert cstats.alive() == 1
     del m3, m4
@@ -118,6 +121,9 @@ def test_to_python():
     mat2[2, 3] = 5
     assert mat2[2, 3] == 5
 
+    if env.GRAALPY:
+        pytest.skip("ConstructorStats is incompatible with GraalPy.")
+
     cstats = ConstructorStats.get(m.Matrix)
     assert cstats.alive() == 1
     del mat

Maybe it's possible to split this in half?

I recommend against doing that in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done (except a couple of methods where the refcount checks were in the middle of the method)

Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks!

@rwgk rwgk merged commit c4a05f9 into pybind:master Oct 7, 2024
80 checks passed
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs changelog Possibly needs a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants