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

Test free-threaded build on all PRs #4631

Merged
merged 4 commits into from
Oct 19, 2024

Conversation

ngoldbaum
Copy link
Contributor

This makes the free-threaded CI fire on all PRs, which should hopefully allow us to spot flakey tests sooner. Also 3.13.0 is out, so this is now a build that's officially supported by upstream.

Unfortunately setup-python still doesn't support the free-threaded build. As an experiment, I'm using setup-uv to install a free-threaded build. I've been experimenting with setup-uv in the NumPy CI so I'm interested if the PyO3 tests find anything weird. I'll switch back to the deadsnakes build if the uv build proves problematic.

@ngoldbaum
Copy link
Contributor Author

Oh well, the python-build-standalone python doesn't quite work out of the box. See astral-sh/python-build-standalone#374.

Going back to deadsnakes...

@ngoldbaum ngoldbaum added the CI-skip-changelog Skip checking changelog entry label Oct 18, 2024
@ngoldbaum
Copy link
Contributor Author

It occurs to me that now might be a reasonable time to add a check to the nox test script for a free-threaded python and not run the abi3 tests in that case. Right now we're effectively running the tests twice since the free-threaded build just ignores the abi3 setting.

I know that you're not really supposed to check for a particular build of Python with nox, but if I understand how we're using nox in CI, adding a check in the noxfile would get the job done. I'm not sure how to do it "correctly" in nox.

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, adding the check to the noxfile.py seems fine to me. Yes, we just use it as a task runner rather than the tox-style multiple Pythons thing. 🙃

@@ -555,7 +555,7 @@ jobs:
- run: python3 -m nox -s test

test-free-threaded:
if: ${{ contains(github.event.pull_request.labels.*.name, 'CI-build-full') || github.event_name != 'pull_request' }}
if: github.ref != 'refs/heads/main'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we do want to run this on main, because that's necessary to populate the GitHub Actions cache (don't ask me why they made it this way, it's their compute that we have to waste 😂).

Suggested change
if: github.ref != 'refs/heads/main'

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(It's for security reasons: You don't want PRs to pull caches from each other, otherwise one evil PR can poison the cache of a good PR, but it's fine to trust the cache of hte base branch.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but I also believe that if the PR is merged it could be considered trustworthy and its cache contents could be imported into the base branch 😁

@ngoldbaum ngoldbaum force-pushed the free-threaded-ci-always-run branch from 57068ae to c94c2cf Compare October 18, 2024 21:11
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@davidhewitt davidhewitt added this pull request to the merge queue Oct 19, 2024
Merged via the queue into PyO3:main with commit 6e7a578 Oct 19, 2024
42 of 43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI-skip-changelog Skip checking changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants