Add cibuildwheel workflow for building pip wheels, and pip tests #583

lanctot · 2021-05-10T13:26:20Z

No description provided.

.github/workflows/wheels.yml

henryiii · 2021-07-06T13:54:41Z

.github/workflows/wheels.yml

+        run: python -m cibuildwheel --output-dir wheelhouse
+
+      - name: Install and test bdist_wheel
+        run: ./open_spiel/scripts/test_wheel.sh


FYI, you can tell cibuildwheel to do the testing via variables too; if you do that, you can remove most of the install step.

Thanks for the suggestions!

Do you mean it can do the testing from within the same virtual environments that have built the wheels? That would be ideal, yes! Do you have an example you can point me to on how to run arbitrary commands within them?

https://cibuildwheel.readthedocs.io/en/stable/options/#test-command - multiple commands should be joined with && on all platforms.

That's awesome, thanks for the pointer! I will take a look once I figure out how to fix the current build issue.

FYI, cibuildwheel 2.0 will add a config mode which is really nice for adding things like this (see scikit-build/cmake-python-distributions@bc80ddb for a great example of it). 2.0.0a4 is fully usable as long as you don't activate the new "build" backend instead of pip (ironically broken due to a pypa/pip bug, not a pypa/build bug).

Ok, thanks again for all the great feedback.

I have now run a basic test suite within the Docker image by the CIBW_TEST_COMMAND and larger tests externally on just the two platforms.

Our release has been delayed for some time waiting only on this bdist wheel, and there are a few other reasons that
I need to push it forward. Unfortunately, I won't have the time to address everything since I have quite limited time to spend on this. So some of these points will have to be deferred (most notably a move to the TOML -- I am unable to invest the time into it right now). I appreciate the help nonetheless, and will look into it for future releases.

Co-authored-by: Henry Schreiner <[email protected]>

henryiii · 2021-07-06T17:51:30Z

Ohh, curious, looks like the new Path caster introduced in pybind/pybind11#2730 is breaking support for anything older than macOS 10.15? Guessing you are using master pybind11?

henryiii · 2021-07-06T18:18:38Z

Yep, bug in master pybind11. Either revert to using an older commit, or wait till a fix is made (hopefully soon).

lanctot · 2021-07-06T18:34:42Z

Actually in OpenSpiel we are using the smart_holder branch, would it still explain it? Edit: tagging @rwgk who we've been working with. This is not urgent so will wait, thanks for your help!

henryiii · 2021-07-06T18:38:12Z

Pretty sure @rwgk's team keeps smart_holder branch up to date.

rwgk · 2021-07-06T18:39:06Z

Actually in OpenSpiel we are using the smart_holder branch, would it still explain it? Edit: tagging @rwgk who we've been working with. This is not urgent so will wait, thanks for your help!

Yes, I'm generally merging PRs against master very quickly into the smart_holder branch (I did so this time).

henryiii · 2021-08-03T17:40:09Z

.github/workflows/wheels.yml

+          source ./open_spiel/scripts/python_extra_deps.sh
+          python -m pip install --upgrade $OPEN_SPIEL_PYTHON_JAX_DEPS $OPEN_SPIEL_PYTHON_PYTORCH_DEPS $OPEN_SPIEL_PYTHON_TENSORFLOW_DEPS $OPEN_SPIEL_PYTHON_MISC_DEPS
+          python -m pip install twine build
+          python -m pip install cibuildwheel==1.12.0


Why not 2.0.1? Also, you can use the action if you'd like instead of manually installing it - that's easier to keep up to date (say with dependabot). Or you can use pipx, but the syntax for pinning is (currently) a little ugly.

If you use version 2, you can specify your settings (most of the CIBW_* variables) in your pyproject.toml, which also makes it much easier to run locally.

Why not 2.0.1?

Because I copied this mostly from examples I found and have not kept up with the versions/releases :) Fixed now, though: I bumped it up to 2.0.1. Thanks!

Also, you can use the action if you'd like instead of manually installing it - that's easier to keep up to date (say with dependabot). Or you can use pipx, but the syntax for pinning is (currently) a little ugly.
If you use version 2, you can specify your settings (most of the CIBW_* variables) in your pyproject.toml, which also makes it much easier to run locally.

Ok good to know. I'll look into it for the next release....... or solicit a contribution from an enthusiatic third party (nudge nudge wink wink, I'll say no more! :))

.github/workflows/wheels.yml

henryiii · 2021-08-03T17:46:06Z

.github/workflows/wheels.yml

+      CIBW_MANYLINUX_X86_64_IMAGE: manylinux2014
+      CIBW_BUILD: ${{ matrix.CIBW_BUILD }}
+      CIBW_SKIP: cp27-* pp*
+      CIBW_BEFORE_BUILD: python -m pip install --upgrade cmake


These should be specified in pyproject.toml normally, then everyone who builds this from an SDist will benefit. Though I see you are running .install.sh and ./open_spiel/scripts/python_extra_deps.sh - at least for Python, the environment is isolated, you will not be using things installed via pip inside the wheel build process. It's not even running on the same version of Python that you are running cibuildwheel with.

Now it might be picking up build produces from the environment, but those must be done inside CIBW_BEFORE_ALL or tool.cibuildwheel.before-all (TOML). For example, on Linux, these are built on the host rather than in the docker container if you run them in a step.

.github/workflows/wheels.yml

henryiii · 2021-08-03T17:49:27Z

.github/workflows/wheels.yml

+          pwd
+          uname -a
+          which python
+          which g++
+          g++ --version
+          python --version
+          chmod +x install.sh
+          ./install.sh
+          python -m pip install --upgrade pip
+          python -m pip install --upgrade setuptools
+          python -m pip install --upgrade -r requirements.txt -q
+          source ./open_spiel/scripts/python_extra_deps.sh
+          python -m pip install --upgrade $OPEN_SPIEL_PYTHON_JAX_DEPS $OPEN_SPIEL_PYTHON_PYTORCH_DEPS $OPEN_SPIEL_PYTHON_TENSORFLOW_DEPS $OPEN_SPIEL_PYTHON_MISC_DEPS


None of this should affect the cibuildwheel run. If it does, that's probably a bug - you don't want stuff built with this version of GCC, etc, inside manylinux2014, that has it's own compiler, etc. If you need it, put it in the before build variable. Join with && in environment variable mode or use a list in TOML for multiple commands.

I have added comments to explain why these are still needed. Short version: I have two set of tests now: basic and full.

The 'basic' tests are run within the docker image via CIBW_TEST_COMMAND as per your recommendation. These have to be a subset of all the tests because unfortunately we need >= 3.7 for some of the machine learning library dependencies (but I want the basic OpenSpiel wheels to still be 3.6-compliant).

The 'full' tests are then run afterward (outside the docker images), by installing the wheel and running the full suite of tests (including all the Tensorflow, PyTorch, and JAX tests). So these full tests require the extra installs.

Add cibuildwheel workflow and pip bdist_wheel tests

5fb0d4a

google-cla bot added the cla: yes label May 10, 2021

lanctot added 6 commits May 10, 2021 11:01

Fix typo and missing env. var

294686c

Fix typo in travis_script

a313ad4

Fix typo

b979374

Use pip3 directly when under virtualenv (travis)

f4c464e

Install extra deps in wheel env

9dc12b7

Add missing __init__.py for python/jax

8035a71

lanctot mentioned this pull request Jun 2, 2021

Building with CentOS causes libtorch linker errors #619

Closed

lanctot and others added 8 commits June 6, 2021 17:00

Merge branch 'master' into lanctot_cibuildwheel

1df4bf4

Update python CMakeLists for full tests names

3aff252

Merge branch 'master' into lanctot_cibuildwheel

5f6ebf8

Make workflow manually triggered

09d71da

Update wheels.yml

b98f232

Update wheels.yml

6f47e18

Add min MacOS version compile flag

ed59146

Add MacOS flag in CMakeLists

dfe55ea

henryiii reviewed Jul 6, 2021

View reviewed changes

.github/workflows/wheels.yml Outdated Show resolved Hide resolved

henryiii reviewed Jul 6, 2021

View reviewed changes

Upgrade cibuildwheel version.

6874da2

Co-authored-by: Henry Schreiner <[email protected]>

lanctot added 5 commits July 12, 2021 09:44

Remove MacOS 10.15 requirement, after pybind11 PR #3077

7b14ff7

Try testing via CIBW_TEST_COMMAND

761a317

Comment out line to test syntax issue

444143d

Move line up one

90d4a8a

Try escaping the curly braces

ad2b603

lanctot and others added 8 commits August 3, 2021 09:39

Merge branch 'master' into lanctot_cibuildwheel

2f5e001

Try test command with regular braces

e1a5f2b

Add bash prefix to command

11b97c9

Add missing link to directory containing requirements.txt

83d2bb4

Add two modes for testing wheels (full vs. basic)

e5a460c

Fix typo for full mode

d23880e

Specify builds for only x86_64

df4083c

Rename wheel workflow items

8a02f55

henryiii reviewed Aug 3, 2021

View reviewed changes

lanctot added 4 commits August 3, 2021 20:36

Move to 2.0.1 + other fixes from @henryiii comments

b6a7212

Add comments to wheels.yml

6f98102

Change wheel filename for install

5a77be2

Applying changes from internal review by @jblespiau

f871136

lanctot added the imported This PR has been imported and awaiting internal review. Please avoid any more local changes, thanks! label Aug 9, 2021

OpenSpiel merged commit f63bd34 into master Aug 9, 2021

lanctot mentioned this pull request Aug 9, 2021

Next release planning #555

Closed

Add cibuildwheel workflow for building pip wheels, and pip tests #583

Add cibuildwheel workflow for building pip wheels, and pip tests #583

Uh oh!

Conversation

lanctot commented May 10, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henryiii commented Jul 6, 2021

Uh oh!

henryiii commented Jul 6, 2021

Uh oh!

lanctot commented Jul 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

henryiii commented Jul 6, 2021

Uh oh!

rwgk commented Jul 6, 2021

Uh oh!

henryiii Aug 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lanctot Aug 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

henryiii Aug 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lanctot commented Jul 6, 2021 •

edited

Loading

henryiii Aug 3, 2021 •

edited

Loading

lanctot Aug 3, 2021 •

edited

Loading

henryiii Aug 3, 2021 •

edited

Loading