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

Fix MacOS Python wheel generation #262

Merged
merged 7 commits into from
Feb 23, 2024
Merged

Fix MacOS Python wheel generation #262

merged 7 commits into from
Feb 23, 2024

Conversation

cschwan
Copy link
Contributor

@cschwan cschwan commented Feb 23, 2024

No description provided.

@cschwan cschwan added the bug Something isn't working label Feb 23, 2024
@cschwan cschwan requested a review from alecandido February 23, 2024 07:56
@cschwan cschwan linked an issue Feb 23, 2024 that may be closed by this pull request
@alecandido
Copy link
Member

@cschwan did you just give up on fixing the Python range? Or is still an attempt to test it?

In case it's the latter: what are you testing?

@alecandido
Copy link
Member

alecandido commented Feb 23, 2024

Btw, since we're touching the version range, consider that also py3.7 is EOL by last June.

https://devguide.python.org/versions/

The current supported range is 3.8-3.12, but I saw many popular packages (including NumPy, that is relevant for us) dropping support for py3.8 already (it will be EOL by October, together with the release of py3.13).
https://pypi.org/project/numpy/#files

@cschwan
Copy link
Contributor Author

cschwan commented Feb 23, 2024

I don't think the Python range has any influence on the subsequent maturin action and, that's what I want to test, possible a harmful influence. In any case I'm just using what maturin generate-ci github produces and if that doesn't work it'll warrant and Issue on maturin's Github page.

@alecandido
Copy link
Member

I don't think the Python range has any influence on the subsequent maturin action and, that's what I want to test, possible a harmful influence.

Ok, so your guess is that the interpreter found by maturin on MacOS are all already present on the runner, isn't it?

For manylinux, this depends on the container, so we already know to be unrelated to the host. But for MacOS it would be a bit surprising (setup-python should install at least user-wide, but I assumed system-wide - so, I'd be puzzled by the --find-interpreter discovery mechanism).

@alecandido
Copy link
Member

Ah @cschwan, while scanning the workflow, I noticed:

sccache: 'true'

which is different from YAML's boolean:

    sccache: true

I would not touch in this PR, or not before having fixed the Python MacOS issue (to avoid noise).

@cschwan
Copy link
Contributor Author

cschwan commented Feb 23, 2024

Despite what I said it seems to make a difference; before commit 00140d5 maturin --find-interpreter finds:

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.18/x64/bin/python3.9,
      PyPy 3.10 at /Users/runner/hostedtoolcache/PyPy/3.10.13/x64/bin/python3.10,
      CPython 3.11 at /Library/Frameworks/Python.framework/Versions/3.11/bin/python3.11,
      CPython 3.12 at /usr/local/bin/python3.12,
      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.18/x64/bin/pypy3.9,
      PyPy 3.10 at /Users/runner/hostedtoolcache/PyPy/3.10.13/x64/bin/pypy3.10

after the commit it finds:

Found CPython 3.10 at /Users/runner/hostedtoolcache/Python/3.10.13/x64/bin/python3.10,
      CPython 3.11 at /usr/local/bin/python3.11,
      CPython 3.12 at /usr/local/bin/python3.12

@alecandido
Copy link
Member

That's incredibly weird, and the most incredible part is CPython 3.10: how come that is found only when installed in isolation?

@alecandido
Copy link
Member

I want to test whether PyPy is masking CPython in discovery

@cschwan
Copy link
Contributor Author

cschwan commented Feb 23, 2024

The last version set in python-version is the default version, so maybe something goes wrong when maturin is run with PyPy.

@cschwan
Copy link
Contributor Author

cschwan commented Feb 23, 2024

This looks much better now:

Found CPython 3.7m at /Users/runner/hostedtoolcache/Python/3.7.17/x64/bin/python3.7,
      CPython 3.8 at /Users/runner/hostedtoolcache/Python/3.8.18/x64/bin/python3.8,
      CPython 3.9 at /Users/runner/hostedtoolcache/Python/3.9.18/x64/bin/python3.9,
      CPython 3.10 at /Users/runner/hostedtoolcache/Python/3.10.13/x64/bin/python3.10,
      CPython 3.11 at /Library/Frameworks/Python.framework/Versions/3.11/bin/python3.11,
      CPython 3.12 at /Library/Frameworks/Python.framework/Versions/3.12/bin/python3.12

@alecandido
Copy link
Member

This looks much better now:

Found CPython 3.7m at /Users/runner/hostedtoolcache/Python/3.7.17/x64/bin/python3.7,
      CPython 3.8 at /Users/runner/hostedtoolcache/Python/3.8.18/x64/bin/python3.8,
      CPython 3.9 at /Users/runner/hostedtoolcache/Python/3.9.18/x64/bin/python3.9,
      CPython 3.10 at /Users/runner/hostedtoolcache/Python/3.10.13/x64/bin/python3.10,
      CPython 3.11 at /Library/Frameworks/Python.framework/Versions/3.11/bin/python3.11,
      CPython 3.12 at /Library/Frameworks/Python.framework/Versions/3.12/bin/python3.12

Where did you find this info in the logs?

@cschwan
Copy link
Contributor Author

cschwan commented Feb 23, 2024

This looks much better now:

Found CPython 3.7m at /Users/runner/hostedtoolcache/Python/3.7.17/x64/bin/python3.7,
      CPython 3.8 at /Users/runner/hostedtoolcache/Python/3.8.18/x64/bin/python3.8,
      CPython 3.9 at /Users/runner/hostedtoolcache/Python/3.9.18/x64/bin/python3.9,
      CPython 3.10 at /Users/runner/hostedtoolcache/Python/3.10.13/x64/bin/python3.10,
      CPython 3.11 at /Library/Frameworks/Python.framework/Versions/3.11/bin/python3.11,
      CPython 3.12 at /Library/Frameworks/Python.framework/Versions/3.12/bin/python3.12

Where did you find this info in the logs?

Here: https://github.com/NNPDF/pineappl/actions/runs/8016527785/job/21898622667#step:4:242. I added some newlines to improve the readibility.

@alecandido
Copy link
Member

Concerning maturin: I investigated the source, and there are a lot of non-trivial heuristics that takes into account the platform, the Python kind, and so on.

However, the essential mechanism is simple:

So, it truly depends on $PATH availability, plus complications to determine the Python specs, that could affect the validity of the executable - but I expect them not to be our issue.
However, if on Linux is working simultaneously for CPython and PyPy, the problem could be inside the complications related to the MacOS target.

@alecandido
Copy link
Member

So, does it truly depend on the installation order?

🐍 Found CPython 3.7m at /Users/runner/hostedtoolcache/Python/3.7.17/x64/bin/python3.7,
         CPython 3.8 at /Users/runner/hostedtoolcache/Python/3.8.18/x64/bin/python3.8,
         CPython 3.9 at /Users/runner/hostedtoolcache/Python/3.9.18/x64/bin/python3.9,
         CPython 3.10 at /Users/runner/hostedtoolcache/Python/3.10.13/x64/bin/python3.10,
         CPython 3.11 at /Library/Frameworks/Python.framework/Versions/3.11/bin/python3.11,
         CPython 3.12 at /Library/Frameworks/Python.framework/Versions/3.12/bin/python3.12,
         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.18/x64/bin/pypy3.9,
         PyPy 3.10 at /Users/runner/hostedtoolcache/PyPy/3.10.13/x64/bin/pypy3.10

@cschwan
Copy link
Contributor Author

cschwan commented Feb 23, 2024

It can't (?) depend on the installation order, but I believe the default Python version (with which maturin is started) does make a difference.

@cschwan
Copy link
Contributor Author

cschwan commented Feb 23, 2024

@alecandido with commit f00839e I've switched the wheel generation from universal wheels to two separate wheels, one for each of the two (aarch and x86) targets. They're potentially generated more quickly and the wheels themselves are now smaller. Do you think this is OK or should we go back to universal wheels?

@alecandido
Copy link
Member

@alecandido with commit f00839e I've switched the wheel generation from universal wheels to two separate wheels, one for each of the two (aarch and x86) targets. They're potentially generated more quickly and the wheels themselves are now smaller. Do you think this is OK or should we go back to universal wheels?

That's should be perfectly fine. It's weird that is faster than generating the universal one (I wonder what's the purpose of it at this point, unless you have GB of shared Python code and data...), but I expected to be more or less the same anyhow (it should compile twice, no way out: they are two different ISAs).

@alecandido
Copy link
Member

It can't (?) depend on the installation order, but I believe the default Python version (with which maturin is started) does make a difference.

It could be, but I'm not sure about that. We could test moving a single PyPy version at the bottom, but if now it's working is probably not worth.

@cschwan
Copy link
Contributor Author

cschwan commented Feb 23, 2024

It's weird that is faster than generating the universal one

What I meant but didn't say is that the two wheels can be run in parallel and thus are usually faster, unless of course no Mac runner is found (like yesterday).

@alecandido
Copy link
Member

unless of course no Mac runner is found (like yesterday).

Unfortunately, that's usually the case. There are many limitations on Mac runners. But we don't care too much about CI performances. As long as they are reasonable that's enough.

@cschwan cschwan merged commit af80b17 into master Feb 23, 2024
7 checks passed
@cschwan cschwan deleted the fix-macos-wheels branch February 23, 2024 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sort out Python wheel generation
2 participants