-
Notifications
You must be signed in to change notification settings - Fork 763
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
Basic GraalPy Support #3247
Basic GraalPy Support #3247
Conversation
9e69f0e
to
9b9ddd6
Compare
I think the most important question to clarify here is the status of this integration in the CI. If we are planning on proper support we should include it both for checking PR and for full target checks for the merge queues. Is this possible and could you sketch an approach here? |
There are some changes for this that are only in graalpy master. Our releases go into pyenv, and our next release that will include the relevant changes will be out in September. So one option is to then setup the CI action to add graalpy to the matrix, install it using pyenv and simply run |
Ideally, this would use exactly the same CI jobs we use for CPython and PyPy or is there a technical limitation which makes this unreasonable?
I think this mainly depends on how urgent this is for you, if you'd like to have this in our |
We would first have to get graalpy into github actions.
We would of course prefer to have this in before our next release, so that we can start running the tests of actual packages that use PyO3 in our CI and fix any additional issues that occur before the September release. |
We are adding support for graalpy to the setup-python action, then running it in the PyO3 CI should need minimal changes |
The setup-python action maintainers had a question about GraalPy's licence, I hope they are happy with it soon and will merge, at which point I will rebase this PR and add GraalPy pytest to your CI (actions/setup-python#694) |
70deae9
to
b34fef4
Compare
This now passes all pytests on GraalPy 23.1 with the latest maturin release. Once PyO3/maturin#1773 and actions/setup-python#694 are merged, I can add GraalPy to the CI config. |
Great! Do you have a sense of how close the setup-python PR is to merging? I think we're going to push an 0.20 release imminently, so if this is extremely close we could wait for it. (Otherwise this could land in 0.20.1 later.) |
I couldn't say. We've tried to be very quick to do any requested changes, but I assume the github folks are busy and they end up not getting back to us for a week or two each time they ask for something. So don't wait for us. |
Ah okay, frustrating! |
b34fef4
to
6560915
Compare
@timfel I added |
c54d795
to
ee0d6ed
Compare
Ok, it's basically working now. The failure on python3.9 is a 500 server error downloading the manylinux image, so I suspect it's transient. For GraalPy, we need a new release of maturin with this PR: PyO3/maturin#1802. I ran the CI config locally and with an updated maturin it passes that one step that's not running. |
Great, I will try to give this a full review in the coming days! I just got back from EuroRust with a ton of threads to follow up before they go cold as well as some time due for my family, so please forgive me if it takes a little longer than usual 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Overall it looks very reasonable to me. Just a couple of style nits and a few questions to understand the future support and testing strategies for this.
@messense, when might you plan to do the next maturin
release?
Yes, the graalpy and implemented cpython version go in the tag. |
06e1b63
to
765ae0a
Compare
Co-authored-by: David Hewitt <[email protected]>
…is mostly missing
bca1c5a
to
1eb6b06
Compare
1eb6b06
to
c5ae4c7
Compare
I'm not entirely sure how the performance differences (both regressions and improvements) are caused by my changes. Maybe those benchmarks are so micro that there is jitter? |
Yep there's a few benchmarks which need to have volatility ignored after I re-enabled them in #3986 I cleared those now 👍 |
Is the failure in "CI / python3.12-x64 ubuntu-latest rust-nightly" something I could have done? It fails in pyo3-macros-backend, I haven't touched that, but I fear I don't understand enough to rule out some interaction with my changes. |
Nightly probably has improved dead code detection. I don't think you need to worry about it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My view here is that we're now in agreement that this is suitable to merge in the current form, and there will be follow ups to tidy up various pieces subject to future work in graalpy.
I think given the size of the effort put in here it makes sense to see this merge now and release in 0.21, both to unblock the ecosystem and so that we can start learning about user experiences with this.
Thanks so much @timfel and @adamreichold for all the time invested here 👍
With these changes, a GraalPy master build, and using maturin master, I can run the pytests and the examples. I have 3 failures in the pytests still. Please let me know if this is going in the right direction for you.