Fix python version problems #214

cschwan · 2023-03-06T09:02:48Z

Here we'll fix the problems discovered in this comment: #211 (comment).

alecandido · 2023-03-06T09:16:06Z

.github/workflows/wheels.yml

@@ -13,7 +11,7 @@ jobs:
      - uses: actions/checkout@v3
      - uses: actions/setup-python@v4
        with:
-          python-version: '3.10'
+          python-version: ['3.7', '3.8', '3.9', '3.11', 'pypy3.7', 'pypy3.8', 'pypy3.9', '3.10']


This should be irrelevant, since the wheel is built in any case inside a manylinux container.

Not sure how to implement it with sed...
I know something algorithmic is more convenient (even with some changes to the base, sed would keep working), but if it is too complex I'd rather go for a patch, or just commit the final result manually edited.

About the irrelevancy I'm not so sure anymore, it seems setup-python may also have an effect there: https://github.com/actions/setup-python/blob/main/docs/advanced-usage.md#using-setup-python-with-a-self-hosted-runner. We should figure out the effect and do it properly as you said.

A self-hosted runner is not a container, but a different machine you provide on your own to run the workflow, instead of using those of GitHub (i.e. Azure...).

Instead, maturin-action is running a container in a GitHub's virtual machine, that is relevant for the host system (we need a Linux machine to run a Linux container, otherwise we would need a Linux virtual machine inside the virtual machine), but it is not relevant Python-wise, since the manylinux container comes pre-built with its versions of Python.

This is true only for Linux, while for Windows and MacOS the setup-python is a crucial step.

alecandido · 2023-03-06T09:16:52Z

.github/workflows/wheels.yml

+  #release:
+  #  name: Release
+  #  runs-on: ubuntu-latest
+  #  if: "startsWith(github.ref, 'refs/tags/')"


Since this is only running on releases, you could keep this uncommented (just to reduce the diff)

alecandido · 2023-03-06T09:17:38Z

.github/workflows/wheels.yml

-on:
-  release:
-    types: [published]
+on: push


You can also add an event, without removing the previous one>

Suggested change

on: push

on:

push:

release:

types: [published]

That's going in the right direction; I think I want to run every job except release in wheels.yml whenever I want to test them (in branches like this one).

This is also what Stefano C is doing in quantum repos (even if they are just pure Python... here I believe it's more relevant).

You can filter the jobs and individual steps with if: key, as you are doing, and the condition can contain an expression, that can depend on variables in contexts (thus including events and outputs of previous steps).
https://docs.github.com/en/actions/learn-github-actions/expressions
https://docs.github.com/en/actions/learn-github-actions/contexts
https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idif
https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idstepsif

cschwan · 2023-03-06T09:39:20Z

It seems to work, except that on Windows no PyPy (3.7, 3.8, 3.9) wheels are created.

alecandido · 2023-03-06T09:40:18Z

It seems to work, except that on Windows no PyPy (3.7, 3.8, 3.9) wheels are created.

That is not a problem :)

alecandido · 2023-03-06T09:42:50Z

I still don't see MacOS wheels for CPython 3.7, 3.8, and 3.9 in https://github.com/NNPDF/pineappl/suites/11369716006/artifacts/584873397
(i.e. the wheels archive from https://github.com/NNPDF/pineappl/actions/runs/4341762408)

cschwan · 2023-03-06T09:56:22Z

You're right: instead the corresponding PyPy wheels are created (and also found) twice. See this line from the action:

Found PyPy 3.7 at /Users/runner/hostedtoolcache/PyPy/3.7.13/x64/bin/python3.7, PyPy 3.8 at /Users/runner/hostedtoolcache/PyPy/3.8.16/x64/bin/python3.8, PyPy 3.9 at /Users/runner/hostedtoolcache/PyPy/3.9.16/x64/bin/python3.9, CPython 3.10 at /Users/runner/hostedtoolcache/Python/3.10.10/x64/bin/python3.10, CPython 3.11 at /Library/Frameworks/Python.framework/Versions/3.11/bin/python3.11, PyPy 3.7 at /Users/runner/hostedtoolcache/PyPy/3.7.13/x64/bin/pypy3.7, PyPy 3.8 at /Users/runner/hostedtoolcache/PyPy/3.8.16/x64/bin/pypy3.8, PyPy 3.9 at /Users/runner/hostedtoolcache/PyPy/3.9.16/x64/bin/pypy3.9

Note how it finds PyPy (?) 3.7

once as /Users/runner/hostedtoolcache/PyPy/3.7.13/x64/bin/python3.7 and
then as /Users/runner/hostedtoolcache/PyPy/3.7.13/x64/bin/pypy3.7.

Both claim to have

Built wheel for PyPy 3.7 to dist/pineappl-0.6.0a2-pp37-pypy37_pp73-macosx_10_9_x86_64.macosx_11_0_arm64.macosx_10_9_universal2.whl

cschwan · 2023-03-06T09:59:09Z

It seems to work, except that on Windows no PyPy (3.7, 3.8, 3.9) wheels are created.

That is not a problem :)

I agree, but what worries me is that I don't understand what's going on here. ~~I specifically requested these versions, why are they not present?~~

They are installed, but it seems maturin doesn't find them:

Found CPython 3.11 at C:\hostedtoolcache\windows\Python\3.11.2\x64\python.exe, CPython 3.10 at C:\hostedtoolcache\windows\Python\3.10.10\x64\python.exe, CPython 3.9 at C:\hostedtoolcache\windows\Python\3.9.13\x64\python.exe, CPython 3.8 at C:\hostedtoolcache\windows\Python\3.8.10\x64\python.exe, CPython 3.7 at C:\hostedtoolcache\windows\Python\3.7.9\x64\python.exe

alecandido · 2023-03-06T10:19:44Z

They are installed, but it seems maturin doesn't find them:

Then there are two main options:

try specifying everything explicitly, and giving up with --find-interpreter (at least in this case)
file an issue on maturin repo (messense is usually replying fast and very helpful)

Note how it finds PyPy (?) 3.7

once as /Users/runner/hostedtoolcache/PyPy/3.7.13/x64/bin/python3.7 and

then as /Users/runner/hostedtoolcache/PyPy/3.7.13/x64/bin/pypy3.7.

This I believe to be normal, and most likely the first is a link to the second, in such a way that PyPy can work both side-by-side and as a drop-in replacement of CPython.

cschwan · 2023-03-06T10:22:56Z

Note how it finds PyPy (?) 3.7

once as /Users/runner/hostedtoolcache/PyPy/3.7.13/x64/bin/python3.7 and

then as /Users/runner/hostedtoolcache/PyPy/3.7.13/x64/bin/pypy3.7.

This I believe to be normal, and most likely the first is a link to the second, in such a way that PyPy can work both side-by-side and as a drop-in replacement of CPython.

Is this OK? Because the wheel is twice created for PyPy, can CPython work with it?

alecandido · 2023-03-06T10:29:00Z

Is this OK? Because the wheel is twice created for PyPy, can CPython work with it?

No, definitely not. The compiled wheel needs ABI compatibility with the interpreter.
In order to guarantee this, wheels adopted a consistent naming scheme, that is the one you observed in the generated objects.

It might be that on MacOS, for whatever reason, PyPy is actually the python3.x x \in [7, 8, 9] executable found by maturin, so it is regenerating twice the wheel for PyPy and overwriting it, while those for CPython are never generated.

Since for PyPy Linux is definitely far more than enough, maybe just try to remove it from all the lists of python-version (for Linux it will be built anyhow, since PyPy will be inside the container).

This reverts commit 466ec41.

This reverts commit 0884b58.

This reverts commit e7688d6.

alecandido · 2023-03-06T13:51:22Z

@cschwan I have the feeling that 80433c0 is in the wrong branch, but I might be wrong...

EDIT: ah sorry, I just got the notification missed the series of "Revert" commits; we will squash anyhow, so please ignore my message

alecandido · 2023-03-06T15:04:58Z

@cschwan in https://github.com/NNPDF/pineappl/actions/runs/4344435880 we actually obtained all the wheels we wanted:

we have all the full Python versions range ([3.7, 3.11]) for CPython for all the three platforms
we have PyPy wheels for Linux

Moreover, the workflow is now running by default on push:, but without uploading the wheels (that should only happen on releases, but of course this I couldn't test).
Everything is documented in update-wheels.sh.

Should we agree on the current result, or do we have still some desiderata left behind?

alecandido · 2023-03-06T15:08:24Z

Curious fact: Windows has the smallest wheels, while MacOS the biggest ones

Win: 676 KB
Linux: 764 KB
MacOS: 1.3 MB

The size of the wheel does not depend on the Python version or the interpreter (wheels for Linux-PyPy are the same size of Linux-CPython). This makes me believe that they are actually very similar, and only the platform makes an actual significant difference...

cschwan · 2023-03-06T15:15:16Z

Should we agree on the current result, or do we have still some desiderata left behind?

As far as it concerns me, that's perfect! Go ahead and merge it.

cschwan · 2023-03-06T15:16:12Z

Curious fact: Windows has the smallest wheels, while MacOS the biggest ones
* Win: 676 KB

* Linux: 764 KB

* MacOS: 1.3 MB
The size of the wheel does not depend on the Python version or the interpreter (wheels for Linux-PyPy are the same size of Linux-CPython). This makes me believe that they are actually very similar, and only the platform makes an actual significant difference...

These numbers make sense, because for MacOS the universal wheels are basically two wheels in one, one for x86_64 and the other for aarch64.

cschwan added 3 commits March 6, 2023 09:59

Enable more Python versions in wheels CI

d83bc7c

Enable wheels CI on push

febb1ef

Try array syntax for field python-version

9569b88

cschwan mentioned this pull request Mar 6, 2023

Problem with Python deployment #211

Closed

Try yet another syntax

466ec41

alecandido reviewed Mar 6, 2023

View reviewed changes

cschwan added 4 commits March 6, 2023 14:21

Improve bin limits handling of Grid::merge

e7688d6

Revert "Try yet another syntax"

0884b58

This reverts commit 466ec41.

Revert "Revert "Try yet another syntax""

c693bc3

This reverts commit 0884b58.

Revert "Improve bin limits handling of Grid::merge"

80433c0

This reverts commit e7688d6.

alecandido added 3 commits March 6, 2023 15:08

Drop PyPy from setup-python action, update event and event handling

9b35981

Mimic former branching condition syntax

dc02ef5

Add workflow name

0c5fa12

alecandido closed this Mar 6, 2023

alecandido reopened this Mar 6, 2023

alecandido merged commit 6276d0b into master Mar 6, 2023

alecandido deleted the fix-python-version-problems branch March 6, 2023 15:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix python version problems #214

Fix python version problems #214

cschwan commented Mar 6, 2023

alecandido Mar 6, 2023

cschwan Mar 6, 2023

alecandido Mar 6, 2023

alecandido Mar 6, 2023

alecandido Mar 6, 2023

cschwan Mar 6, 2023

alecandido Mar 6, 2023

cschwan commented Mar 6, 2023

alecandido commented Mar 6, 2023

alecandido commented Mar 6, 2023 •

edited

Loading

cschwan commented Mar 6, 2023

cschwan commented Mar 6, 2023 •

edited

Loading

alecandido commented Mar 6, 2023

cschwan commented Mar 6, 2023

alecandido commented Mar 6, 2023

alecandido commented Mar 6, 2023 •

edited

Loading

alecandido commented Mar 6, 2023

alecandido commented Mar 6, 2023

cschwan commented Mar 6, 2023

cschwan commented Mar 6, 2023 •

edited

Loading

Fix python version problems #214

Fix python version problems #214

Conversation

cschwan commented Mar 6, 2023

alecandido Mar 6, 2023

Choose a reason for hiding this comment

cschwan Mar 6, 2023

Choose a reason for hiding this comment

alecandido Mar 6, 2023

Choose a reason for hiding this comment

alecandido Mar 6, 2023

Choose a reason for hiding this comment

alecandido Mar 6, 2023

Choose a reason for hiding this comment

cschwan Mar 6, 2023

Choose a reason for hiding this comment

alecandido Mar 6, 2023

Choose a reason for hiding this comment

cschwan commented Mar 6, 2023

alecandido commented Mar 6, 2023

alecandido commented Mar 6, 2023 • edited Loading

cschwan commented Mar 6, 2023

cschwan commented Mar 6, 2023 • edited Loading

alecandido commented Mar 6, 2023

cschwan commented Mar 6, 2023

alecandido commented Mar 6, 2023

alecandido commented Mar 6, 2023 • edited Loading

alecandido commented Mar 6, 2023

alecandido commented Mar 6, 2023

cschwan commented Mar 6, 2023

cschwan commented Mar 6, 2023 • edited Loading

alecandido commented Mar 6, 2023 •

edited

Loading

cschwan commented Mar 6, 2023 •

edited

Loading

alecandido commented Mar 6, 2023 •

edited

Loading

cschwan commented Mar 6, 2023 •

edited

Loading