8000 Feat/build multiple wheels by Oisin-M · Pull Request #20 · ecmwf/reusable-workflows · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Feat/build multiple wheels #20

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 49 commits into
base: main
Choose a base branch
from
Open

Feat/build multiple wheels #20

wants to merge 49 commits into from

Conversation

Oisin-M
Copy link
@Oisin-M Oisin-M commented Apr 25, 2025 •
edited
Loading

Adds the ability to also build wheels for packages. This is needed e.g. for Rust-Python packages.

This PR:

  • maintains the previous functionality by default, but makes it explicit now to only python -m build --sdist instead of a generic python -m build. The checks on versions etc. are still done here
  • adds optional extra task that depends on this first task with the ability to build wheels for config-specified platforms and Python versions

An example successful workflow building wheels for a rust-python package is https://github.com/ecmwf/earthkit-hydro/actions/runs/14662296289/job/41149252856, which is largely driven via the pyproject.toml. I have a version on https://github.com/ecmwf/earthkit-hydro/tree/develop that's working

Oisin-M and others added 23 commits April 24, 2025 13:56
@Oisin-M
use cibuildwheel to build multiple wheels
267ad4a
@Oisin-M
switch to always upload to pypi or pypi test
768a048
@Oisin-M
update default py versions for cd-pypi.yml
36a17ce
@Oisin-M
fix json parsing
45f586d
@Oisin-M
bugfix str to list
66f35b2
@Oisin-M
attempt bugfix
d52882f
@Oisin-M
remove dot in pyversion def args
a224d14
@Oisin-M
pip install in CIBW_BEFORE_BUILD
bc188c9
@Oisin-M
pip install cibuildwheel before running wheel build
bef32fa
@Oisin-M
install rust
d423719
@Oisin-M
fix artifact upload wheels
79db8d2
@Oisin-M
attempt bugfix artifact
feba855
@Oisin-M
bugfix typo
a13af8c
@Oisin-M
attempt bugfix rust install
1c3df9b 
l
@Oisin-M
bugfix rust install
974ef0d
@Oisin-M
improve rust install
b62e0e7
@Oisin-M
debug rust install
3ecae0e
@Oisin-M
debug rust install v2
61b2926
@Oisin-M
simplify cd
d6e00f9
@Oisin-M
remove skip in cd action
3f45df5
@Oisin-M
upload directly to pypi without artifact
74358a7
@Oisin-M
allow running on windows
5c77591
@Oisin-M
maintain previous functionality
8a5bfe5
@Oisin-M Oisin-M requested a review from iainrussell April 25, 2025 10:03
@iainrussell
Copy link
Member

Hi Oisin, this looks nice, but it does not seem to work for other packages. I made a test release of findlibs, and it does not like the empty os matrix:
https://github.com/ecmwf/findlibs/actions/runs/14665097091

Error when evaluating 'strategy' for job 'deploy_wheels'. ecmwf/reusable-workflows/.github/workflows/cd-pypi.yml@feat/build_multiple_wheels (Line: 118, Col: 13): Matrix vector 'os' does not contain any values,ecmwf/reusable-workflows/.github/workflows/cd-pypi.yml@feat/build_multiple_wheels (Line: 119, Col: 17): Matrix vector 'python' does not contain any values

@Oisin-M
Copy link
Author
Oisin-M commented Apr 25, 2025 •
edited
Loading

Just documenting after further investigation and discussion.

Two current issues:

  1. Sometimes the pyversion and platforms default values (namely []) cause an Error when evaluating 'strategy' but not always.
  2. The previous default functionality was python -m build which also may build a wheel. Changing now to --sdist will no longer build the wheel which is undesirable and we should avoid. However, actually the previous python -m build also caused issues in some cases with rejections from PyPI e.g.

Binary wheel
'earthkit_hydro-0.1.4.dev58-cp313-cp313-linux_x86_64.whl' has
an unsupported platform tag 'linux_x86_64'.

Will investigate further, but it seems like the easiest solution would be

  1. Make an explicit if check to see if pyversion or platforms are empty to avoid allowing it to even get to the strategy
  2. Add a boolean sdist_only with default False to switch between python -m build and python -m build --sdist. This retains the old functionality by default but makes it customisable.

tmi
tmi reviewed Apr 28, 2025
View reviewed changes
tmi
tmi reviewed Apr 28, 2025
View reviewed changes
tmi
tmi reviewed Apr 28, 2025
View reviewed changes
@iainrussell
Copy link
Member

Thanks @Oisin-M and @tmi - I tested it with findlibs and it's working as it used to :) I'm happy to merge this in if you're also happy for me to do it

@Oisin-M
Copy link
Author
Oisin-M commented Apr 30, 2025

Agreed to separate the wheels out from the cd for pure python packages. Plan is to have separate

cd-pypi.yml
cd-pypi-binwheel.yml
cd-pypi-cppwrapperwheel.yml

@Oisin-M
Copy link
Author
Oisin-M commented May 13, 2025

I have now split the action for binary wheels out from the original cd-pypi action.

Overview of changes are as follows

  • cd-pypi.yml:
    • can now give buildargs i.e. python -m build {buildargs}
    • can now set env variables from input env_vars
  • cd-pypi-binwheel.yml
    • can also set env variables from input env_vars
    • can specify platforms and python versions to build for

I've now got a way using these actions to build both purepython wheels as well as binary wheels in earthkit-hydro.

iainrussell
iainrussell reviewed May 13, 2025
View reviewed changes
iainrussell
iainrussell reviewed May 13, 2025
View reviewed changes
@Oisin-M
Copy link
Author
Oisin-M commented May 14, 2025 •
edited
Loading

Just to flag (this is not related to changes here, but it's part of the cd-pypi action checks we already have) - if we have prints in the setup.py, then the checks we do

version=$(python setup.py --version)
release=${{ github.ref_name }}
if [ "${{ inputs.testpypi }}" != "true" ] ; then test "$release" == "$version"; fi

are problematic because $version gets all of the printed output, not just the version.

Is there a better way to get the version to avoid this?

@tmi
Copy link
Contributor
tmi commented May 19, 2025

@Oisin-M I don't like the setup.py --version because in addition to what you say, it doesn't work for when you have pyproject only. I was looking for a python -m build-based way of obtaining just version, but failed to.

One way forward is to agree, ecmwf-wide, on means of obtaining the version, and hardcode that here. I can imagine
a/ always respect VERSION file (I actually started doing that in all packages that derive from c/c++ code directly, to ensure version equality)
b/ define get_version in setup.py (which still isn't great because some setup.pys aren't easily importible / side-effect free)
but sadly neither works in the case of setuptools_scm + pyproject :( But then, this check is opportunistic -- we can keep it optional, and check again after the package is built, from the wheel metadata

@FussyDuck
Copy link
FussyDuck commented May 19, 2025 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@Oisin-M
Copy link
Author
Oisin-M commented Jun 2, 2025 •
edited
Loading

@iainrussell are you happy to merge this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tmi tmi tmi left review comments

@iainrussell iainrussell iainrussell left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Oisin-M @iainrussell @tmi @FussyDuck
0