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

gh-119292: Add job to jit.yml to build and test with --disable-gil #119293

Merged
merged 10 commits into from
May 21, 2024

Conversation

savannahostrowski
Copy link
Member

@savannahostrowski savannahostrowski commented May 21, 2024

Alrighty, so I added a very basic job to test that we don't regress things as the JIT and free threading work evolves. I split the build and test into separate steps, so we have a little finer granularity on failures (it's just slightly prettier!). I also opted not to use strategy/matrix to pass in the LLVM version since we only support 18 today. I can add that in if folks feel strongly if we want to keep things very consistent with the other job, but I was going for simple 😄

@savannahostrowski
Copy link
Member Author

savannahostrowski commented May 21, 2024

I also think I messed up the issue tagging bot by adding my PR comment after opening the PR...not sure how to fix that? Copy paste saves the day?

.github/workflows/jit.yml Outdated Show resolved Hide resolved
.github/workflows/jit.yml Outdated Show resolved Hide resolved
savannahostrowski and others added 2 commits May 20, 2024 21:36
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
@savannahostrowski
Copy link
Member Author

I suppose this could also be implemented as a matrix entry using include. For now, I feel like a new job is probably simpler because we're only testing a single platform, but if we ever want to expand the tested platforms, we could consider moving in that direction to avoid a ton of duplication.

- name: Build with JIT enabled and GIL disabled
run: |
sudo bash -c "$(wget -O - https://apt.llvm.org/llvm.sh)" ./llvm.sh 18
export PATH="$(llvm-config-18 --bindir):$PATH"
Copy link
Member

Choose a reason for hiding this comment

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

Opinions: should we add --with-pydebug? Maybe test both modes?

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 it's fine to leave this as-is (just one --with-pydebug build).

Copy link
Member

@brandtbucher brandtbucher left a comment

Choose a reason for hiding this comment

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

Great idea.

One suggestion:

.github/workflows/jit.yml Show resolved Hide resolved
Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>
@brandtbucher brandtbucher merged commit c4722cd into python:main May 21, 2024
50 checks passed
@brandtbucher brandtbucher self-assigned this May 21, 2024
@brandtbucher brandtbucher added tests Tests in the Lib/test dir awaiting merge needs backport to 3.13 bugs and security fixes labels May 21, 2024
@miss-islington-app
Copy link

Thanks @savannahostrowski for the PR, and @brandtbucher for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 21, 2024
…il (pythonGH-119293)

(cherry picked from commit c4722cd)

Co-authored-by: Savannah Ostrowski <savannahostrowski@gmail.com>
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
@bedevere-app
Copy link

bedevere-app bot commented May 21, 2024

GH-119314 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label May 21, 2024
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
…il (pythonGH-119293)

Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
@savannahostrowski savannahostrowski deleted the jit-no-gil-ci branch September 27, 2024 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants