Only run test job after basic checks are green to reduce wasted CI work. #249

adamreichold · 2022-01-08T09:51:24Z

While it makes CI times a bit slower, it will avoid wasting CPU time and thereby energy on the test matrix when there are more fundamental issues with a PR.

Also adds PyPy on Linux to our test targets.

Fixes #219

adamreichold · 2022-01-08T12:18:07Z

PyPy 3.8 and NumPy 1.21.1 works locally for me using the official PyPy binaries so my guess is that there is some dependency missing in the GitHub images which I have installed locally.

This requires removing the Poetry lockfiles as it appears impossible to have a single lock spanning such a diverse set of Python interpreters.

adamreichold · 2022-01-08T14:21:44Z

@kngwyu @davidhewitt So what started as simply trying to avoid a few wasted CPU cycles sprawled into wasting quite the amount of CPU cycles by triyng to add PyPy to our CI... :-\

It does seem to work now, but I had to remove the poetry.lock files as a single set of files created using CPython triggered build failures when installed under PyPy.

Using Poetry also seems pretty slow as it will resolve NumPy versions which have to be built from source instead of downloading a pre-built wheel. I tried to avoid that by manually setting up a venv and installing our dependencies without version specifications but ran into trouble on macOS and had issues manually handling the difference between Windows and POSIX environments.

In addition building NumPy for PyPy 3.8 failed for unclear reasons on Windows and both Windows and macOS were considerably slower than Linux. Hence I decided to only add PyPy on Linux as CI targets.

davidhewitt

👍 looks good to me, yeah I had a lot of similar fun when I first added PyPy support for setuptools-rust and PyO3 in CI!

Strange that numpy doesn't have prebuilt wheels for PyPy on Windows and macOS?

R.e. poetry issues, do you think it's worth trying tox? Has worked nicely in PyO3 for its examples.

davidhewitt · 2022-01-08T16:07:21Z

.github/workflows/ci.yml

+      fail-fast: false
      matrix:
-        python-version: [3.6, 3.7, 3.8, 3.9]
+        python-version: [3.7, 3.8, 3.9]


Consider adding 3.10, and 3.11-dev (if numpy supports them?)

I did, but it seems that this does not work without a Git dependency on PyO3 until 0.16 is released.

adamreichold · 2022-01-08T16:23:43Z

Strange that numpy doesn't have prebuilt wheels for PyPy on Windows and macOS?

It is older NumPy versions missing pre-built wheels for newer PyPy versions. But Poetry's version resolution seems to always select these older versions based on our specified Python version range (not based on the actual Python version in use).

This is why just installing NumPy into a virtualenv using pip is much faster: It will select the newest stable version which usually also has a prebuilt wheel available.

R.e. poetry issues, do you think it's worth trying tox? Has worked nicely in PyO3 for its examples.

Have not used it so far, but will give it a try...

adamreichold · 2022-01-08T17:44:37Z

R.e. poetry issues, do you think it's worth trying tox? Has worked nicely in PyO3 for its examples.

Have not used it so far, but will give it a try...

This has certainly simplified things but NumPy really does not seem to provide pre-built wheels for PyPy on macOS and Windows and hence I left them disabled.

I also switched our linalg example use to dynamic linking to remove a few minutes from the initial CI stage.

kngwyu

Thanks, looks nicely shaped.

About PyPy: I think only linux is OK for now, to avoid compiling numpy.

kngwyu · 2022-01-09T04:40:06Z

.github/workflows/ci.yml

-          cargo_toml["dependencies"]["numpy"]["path"] = "../rust-numpy"
          cargo_toml["dependencies"]["ndarray"] = "0.13.1"
          cargo_toml["dependencies"]["num-complex"] = "0.2.4"
+          cargo_toml["workspace"] = {}


I like this 👍🏼

Me too, but it was almost a necessity due to caching: The caching action would fail if its working directory was not present when run, hence building within an existing directory significantly simplified things compared to re-ordering steps so that the copy was created first, and then the caching action triggered.

kngwyu · 2022-01-09T04:43:47Z

.github/workflows/ci.yml

-          cargo update -p $(cargo pkgid -p ndarray 2>&1 >/dev/null | grep 0.15 | sed -e 's/^[ \t]*//') --precise 0.13.1
-        working-directory: ../simple-extension-msrv
-      - name: Test Example
+          import toml
+          import subprocess
+          cargo_lock = toml.load("Cargo.lock")
+          for pkg in cargo_lock["package"]:
+            if pkg["name"] == "ndarray" and pkg["version"] != "0.13.1":
+              pkg_id = pkg["name"] + ":" + pkg["version"]
+              subprocess.run(["cargo", "update", "--package", pkg_id, "--precise", "0.13.1"], check=True)


I personally like the previous shell hack, but this version looks also OK

I have to admit that I am not very fond of shell oneliners. Especially in this case where a human-readable error message has to be ignored and the current ndarray version 0.15 was hard-coded. And since we have Python at hand in any case, it felt nicer to uniformly handle all the Cargo manipulations using the toml package.

kngwyu · 2022-01-09T04:50:50Z

.github/workflows/ci.yml

    runs-on: ${{ matrix.platform.os }}
+    needs: [lint, check-msrv, linalg-example]
    strategy:
-      max-parallel: 16


Are jobs still parallelized without this? I haven't used actions recently so sorry if I'm asking something outdated

Yes, they still run in parallel. But I would not have known, I had to try.

adamreichold added 2 commits January 8, 2022 12:28

Only run test job after basic checks are green to reduce wasted CI work.

6696698

Enable caching on the prerequiste CI jobs to improve time-to-feedback.

9d33e3b

Include PyPy in the test matrix.

8b7ce5c

This requires removing the Poetry lockfiles as it appears impossible to have a single lock spanning such a diverse set of Python interpreters.

davidhewitt approved these changes Jan 8, 2022

View reviewed changes

adamreichold added 2 commits January 8, 2022 18:36

Port example from peotry to tox to simplify dependency management

18e6863

Dynamically link OpenBLAS in linalg example to reduce CI time.

660ba20

kngwyu approved these changes Jan 9, 2022

View reviewed changes

adamreichold merged commit 3fcd194 into PyO3:main Jan 9, 2022

adamreichold deleted the delay-test-job branch January 9, 2022 06:30

Only run test job after basic checks are green to reduce wasted CI work. #249

Only run test job after basic checks are green to reduce wasted CI work. #249

Uh oh!

Conversation

adamreichold commented Jan 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamreichold commented Jan 8, 2022

Uh oh!

adamreichold commented Jan 8, 2022

Uh oh!

davidhewitt left a comment

Choose a reason for hiding this comment

Uh oh!

davidhewitt Jan 8, 2022

Choose a reason for hiding this comment

Uh oh!

adamreichold Jan 8, 2022

Choose a reason for hiding this comment

Uh oh!

adamreichold commented Jan 8, 2022

Uh oh!

adamreichold commented Jan 8, 2022

Uh oh!

kngwyu left a comment

Choose a reason for hiding this comment

Uh oh!

kngwyu Jan 9, 2022

Choose a reason for hiding this comment

Uh oh!

adamreichold Jan 9, 2022

Choose a reason for hiding this comment

Uh oh!

kngwyu Jan 9, 2022

Choose a reason for hiding this comment

Uh oh!

adamreichold Jan 9, 2022 • 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!

kngwyu Jan 9, 2022

Choose a reason for hiding this comment

Uh oh!

adamreichold Jan 9, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

adamreichold commented Jan 8, 2022 •

edited

Loading

adamreichold Jan 9, 2022 •

edited

Loading