Skip to content

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

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
Merged
brandtbucher merged 10 commits into from
May 21, 2024
Merged

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

brandtbucher merged 10 commits into from
May 21, 2024

Conversation

@savannahostrowski
Copy link
Member

@savannahostrowskisavannahostrowski commented May 21, 2024•
edited
Loading

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 😄

@bedevere-appbedevere-appbot mentioned this pull request May 21, 2024
Closed
@savannahostrowski
Copy link
MemberAuthor

savannahostrowski commented May 21, 2024•
edited
Loading

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?

hugovk
hugovk reviewed May 21, 2024
View reviewed changes
savannahostrowskiand others added 2 commits May 20, 2024 21:36
@savannahostrowski@hugovk
Update .github/workflows/jit.yml
6b760d3
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
@savannahostrowski@hugovk
Update .github/workflows/jit.yml
add9659
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
@savannahostrowski
Copy link
MemberAuthor

savannahostrowski commented May 21, 2024

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.

sobolevn
sobolevn reviewed May 21, 2024
View reviewed changes
.github/workflows/jit.yml
- 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

@sobolevnsobolevnMay 21, 2024

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

@brandtbucherbrandtbucherMay 21, 2024

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).

brandtbucher
brandtbucher approved these changes May 21, 2024
View reviewed changes
Copy link
Member

@brandtbucherbrandtbucher 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:

@savannahostrowski@brandtbucher
Update jit.yml
e50b538
Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>
@brandtbucherbrandtbucher merged commit c4722cd into python:mainMay 21, 2024
@brandtbucherbrandtbucher self-assigned this May 21, 2024
@brandtbucherbrandtbucher 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

miss-islington-appbot commented May 21, 2024

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
@savannahostrowski@hugovk@miss-islington
pythonGH-119292: Add job to JIT CI to build and test with --disable-g…
d3c5b6c
…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-appbot commented May 21, 2024

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

@bedevere-appbedevere-appbot 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
@savannahostrowski@hugovk@estyxx
pythonGH-119292: Add job to JIT CI to build and test with --disable-g…
646a445
…il (pythonGH-119293) Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
@savannahostrowskisavannahostrowski deleted the jit-no-gil-ci branch September 27, 2024 16:55
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hugovkhugovkhugovk left review comments

@sobolevnsobolevnsobolevn left review comments

@brandtbucherbrandtbucherbrandtbucher approved these changes

Assignees

@brandtbucherbrandtbucher

Labels

awaiting mergeskip newstestsTests in the Lib/test dirtopic-JIT

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@savannahostrowski@hugovk@sobolevn@brandtbucher@Eclips4