search

NNPDF / pineappl

PineAPPL is not an extension of APPLgrid
https://nnpdf.github.io/pineappl/
GNU General Public License v3.0
12 stars 3 forks source link

Move wheels release to maturin action #212

Closed alecandido closed 1 year ago

alecandido commented 1 year ago

The maturin generator definitely added to many architectures to the workflow. I believe we don't care about all of them.

@cschwan @scarlehoff can you have a look and comment if we only care about x86_64 (and equivalents) or we also want to retain some of others? Concerning MacOS, I decided to release universal wheels (don't care too much if it is suboptimal or what on M1/M2, for as long as it works).

cschwan commented 1 year ago

I'm not sure whether https://github.com/NNPDF/pineappl/actions/runs/4302246364/jobs/7500486764 tells us anything, but this has the same error again:

💥 maturin failed
  Caused by: Failed to parse Cargo.toml at /home/runner/work/pineappl/pineappl/Cargo.toml
  Caused by: missing field `package`

which is usually the sign of maturin being too old (<maturin-0.13.4).

This, https://github.com/NNPDF/pineappl/actions/runs/4302246364/jobs/7500486918, looks like it's trying to run an x86-image on an amd64 platform.

alecandido commented 1 year ago

This is the version we are using https://github.com/PyO3/maturin-action/releases/tag/v1

Bump version to 1.38.1

So it is https://github.com/PyO3/maturin-action/releases/tag/v1.38.1

https://github.com/NNPDF/pineappl/actions/runs/4302246364/jobs/7500486764#step:4:182

cschwan commented 1 year ago

The version is indeed fine, but it seems that working-directory (or --manifest-path when calling maturin generate-ci) is missing. I found this Issue :smile:: https://github.com/PyO3/maturin-action/issues/11.

cschwan commented 1 year ago

--manifest-path is set in $FLAGS as an environment variable, but is it possible that it isn't properly substituted? The log says:

Caused by: Failed to parse Cargo.toml at /home/runner/work/pineappl/pineappl/Cargo.toml

while it should be the pineappl_py subfolder. Assuming that /home/runner/work/pineappl/ is the repository root I don't understand why it goes into the Rust crate subfolder.

cschwan commented 1 year ago

It seems that $FLAGS wasn't expanded, the last commit doesn't use it and work well (so far).

alecandido commented 1 year ago

Ok, fine for $FLAGS, I will find a better way to do it, but we definitely can't use --find-interpreter on Linux, because of

Note that the actions/setup-python action won't affect manylinux build since it's containerized, so if you want to build for certain Python version for Linux, use -i pythonX.Y in the args option in PyO3/maturin-action instead, for example

https://github.com/PyO3/maturin-action#manylinux-docker-container

For the other two, we can just add Python versions to the matrix.

cschwan commented 1 year ago

On Linux I see the following messages:

📦 Built wheel for CPython 3.7m to dist/pineappl-0.6.0a1-cp37-cp37m-manylinux_2_17_x86_64.manylinux2014_x86_64.whl
📦 Built wheel for CPython 3.8 to dist/pineappl-0.6.0a1-cp38-cp38-manylinux_2_17_x86_64.manylinux2014_x86_64.whl
📦 Built wheel for CPython 3.9 to dist/pineappl-0.6.0a1-cp39-cp39-manylinux_2_17_x86_64.manylinux2014_x86_64.whl
📦 Built wheel for CPython 3.10 to dist/pineappl-0.6.0a1-cp310-cp310-manylinux_2_17_x86_64.manylinux2014_x86_64.whl
📦 Built wheel for CPython 3.11 to dist/pineappl-0.6.0a1-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl
📦 Built wheel for PyPy 3.7 to dist/pineappl-0.6.0a1-pp37-pypy37_pp73-manylinux_2_17_x86_64.manylinux2014_x86_64.whl
📦 Built wheel for PyPy 3.8 to dist/pineappl-0.6.0a1-pp38-pypy38_pp73-manylinux_2_17_x86_64.manylinux2014_x86_64.whl
📦 Built wheel for PyPy 3.9 to dist/pineappl-0.6.0a1-pp39-pypy39_pp73-manylinux_2_17_x86_64.manylinux2014_x86_64.whl

which should be more than we need, should it not?

The documentation of PyO3 that you quoted I read as: if we wanted one specific version of Python, say only 3.11, then we'd have to use the option. Or do I misread it?

cschwan commented 1 year ago

@AleCandido I think this finally ready to merge. I'd like to squash and merge it since most changes are the typical trial-and-error Github action commits. If you're OK with it, I'd merge as soon as the CI has finished.

alecandido commented 1 year ago

I'm fine with squashing, I also dislike trial-and-errors commits required to debug workflows. I still hope that one day they will either give a debug shell or officially support act.

Give me just the time to have a final look, and then we will merge.