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 GraalPy to manylinux images #1509

Closed
wants to merge 3 commits into from
Closed

Add GraalPy to manylinux images #1509

wants to merge 3 commits into from

Conversation

timfel
Copy link

@timfel timfel commented Jul 18, 2023

This makes the open source GraalPy implementation available in manylinux images.

@timfel
Copy link
Author

timfel commented Jul 18, 2023

This was requested when adding GraalPy to cibuildwheels: pypa/cibuildwheel#1538 (comment)

@auvipy
Copy link
Contributor

auvipy commented Jul 20, 2023

thanks lets wait for knowing the consensus

@mayeut
Copy link
Member

mayeut commented Jul 23, 2023

This seems like a huge bump in size for a single version (around 200MB compressed so that's roughly between +25% and +50% increase for compressed image size depending on images). This is similar uncompressed (+30% on manylinux2014_aarch64).
Unless there's a similar huge demand for GraalPy, merging this would add a penalty on every other user.
IMHO, a starting middle ground would be to add a helper script in the image to install GraalPy properly (i.e. as it would be installed with this PR).

@timfel
Copy link
Author

timfel commented Jul 24, 2023

This seems like a huge bump in size for a single version (around 200MB compressed so that's roughly between +25% and +50% increase for compressed image size depending on images). This is similar uncompressed (+30% on manylinux2014_aarch64). Unless there's a similar huge demand for GraalPy, merging this would add a penalty on every other user. IMHO, a starting middle ground would be to add a helper script in the image to install GraalPy properly (i.e. as it would be installed with this PR).

The manylinux build scripts are not left in the image. Are you suggesting I change it so we leave them, and I add a script to run the commands that the image build runs? If we're not using the same build scripts, I'm worried we'll end up diverging with how we set up GraalPy from how we set up CPython.

Below are the steps that currently run during the build that are relevant for graalpy. Both the install and finalize scripts source helper scripts that are in /build_scripts; should I change the build to not delete those build scripts so we can execute them at runtime?

 RUN manylinux-entrypoint /build_scripts/install-graalpy.sh 3.10 graal 23.0.0 graalpython
 RUN manylinux-entrypoint /build_scripts/finalize.sh && rm -rf /build_scripts
 /opt/python/graalpy310-graalpy230_310_native/bin/python /tests/manylinux-check.py manylinux_2_28 x86_64

@timfel
Copy link
Author

timfel commented Jul 25, 2023

Hi @mayeut - I've updated the PR. I tried for a bit to bend the system so that I could create something in the image at runtime that is equivalent to what the build process produces currently, but due to the multi-stage nature of the current process it was a bit messy and fragile.

I settled instead for another approach: at the end of the build process in the finalize script, I delete most of the graalpy files except for the site-packages and launchers that were created in the build process (so the installed requirements stay in the image). I leave a script /usr/local/bin/install-graalpy${PY_VER} in the image that was generated at build time to ensure the exact same version (same sha256) of graalpy is later re-installed into the image. This way, the only footprint overhead is from the packages and stdlib that are installed for all python's the same way, so they weigh in about the same.

@timfel
Copy link
Author

timfel commented Jul 26, 2023

The CI failure seems unrelated to my PR, it is in a job that doesn't install graalpy

Didn't appear again after the rebase.

@timfel
Copy link
Author

timfel commented Aug 7, 2023

Friendly ping :) Anything I can help with or clarify?

@timfel
Copy link
Author

timfel commented Aug 8, 2023

The failure is again unrelated to this PR (it's on s390x, this failed before on this PR and then just passed after rebasing and rerunning)

@mayeut mayeut mentioned this pull request Aug 13, 2023
@mayeut
Copy link
Member

mayeut commented Aug 13, 2023

thanks for the follow-up @timfel.
I finally had a bit of time so took another approach in #1518

@mayeut mayeut mentioned this pull request Aug 19, 2023
@mayeut
Copy link
Member

mayeut commented Oct 7, 2023

superseded by #1520

@mayeut mayeut closed this Oct 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants