Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
Enable native AArch64 Ubuntu CI jobs for the JIT#127332
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
diegorusso commented Nov 27, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Enable OpenSSL builds on AArch64 Move AArch64 JIT builds from emulated to native.
diegorusso commented Nov 27, 2024
This is a continuation of #125786 |
hugovk commented Nov 27, 2024
Exciting! Could we add something like |
Uh oh!
There was an error while loading. Please reload this page.
diegorusso commented Nov 27, 2024
Thanks @hugovk for the help on Discord DM :) I think we have now what you were asking. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
Remove openssl and asan jobs on AArch64
brandtbucher commented Nov 27, 2024
This is probably going to conflict with GH-127212. I think maybe we should merge that one first (resolving the conflict shouldn't be too bad, since you're just removing the conflicting lines anyways). |
brandtbucher left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy with the JIT changes, thanks!
savannahostrowski commented Nov 28, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Update: the ownership of the hardware here was ambiguous. Looks like this is a non-concern as we are using GH managed runners. |
diegorusso commented Nov 29, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Hello, I reworked the PR "completely". @savannahostrowski@brandtbucher please have a look. I've dismissed your approval because now the PR is very different from before. |
Please review again. The PR has changed a lot from the approval
hugovk left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
savannahostrowski left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One nit but, otherwise, this LGTM! Thanks for fighting with this, Diego!
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
savannahostrowski left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Diego! This looks great to me!
savannahostrowski commented Dec 3, 2024
@brandtbucher If you're okay with these changes, I'd be ready to merge this. |
brandtbucher commented Dec 3, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
So, it looks like it works correctly (thanks for figuring it out). I'm just bummed because I find the new structure quite a bit harder to follow and work with than before. This seems like a lot of churn just to turn off two jobs on forks. My understanding is that the restructuring is necessary because we can't exclude jobs from a matrix that's built up of includes, since excludes runs before includes. I'd like to explore the options a bit further before landing this, since GHA has lots of different, uh, "features" for this sort of logic. For instance: could we turn this line into |
brandtbucher commented Dec 3, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
See GH-127581 for an alternative approach. The two AArch64 Linux jobs run as expected on CPython org CI, and complete immediately on the fork's CI. When the runners (presumably) become free for all, we can just remove the |
diegorusso commented Dec 3, 2024
We tried at some point to put the As you can see from the screenshot below, the aarch64 jobs will be pending forever because the matrix includes them but they will never be allocated on forks. The only way possible is to exclude them from the matrix altogether with ![]() The only way possible is to exclude them when building the matrix. Unfortunately as the When reading the fields in the include, I realized that:
Hence I decided to do the refactoring. As further improvement, this could lead to an additional refactoring by moving the build logic in the I'm open to other solutions (if there are) |
diegorusso commented Dec 3, 2024
After messaging with @brandtbucher on Discord, I decided to close this PR as leave it for history. Moreover:
|
diegorusso commented Dec 3, 2024
This is the new PR: #127584 |

Enable native AArch64 Ubuntu CI jobs for the JIT. In order to achieve that I had to refactor the jit.yml workflow:
mainor3.*like we have inbuild.yamlThe refactoring was needed because these jobs used different fields from the earlier unified matrix. By splitting it, we can better tailor what fields to use for every OS.
Also this allows to exclude AArch64 Ubuntu jobs on forks. Earlier this wasn't possible because the workflow was using in the matrix definition an
includethat is always executed after anexclude. There was no way to exclude AArch64 Linux runners for forks.As a consequence, the job names are not re
porting anymore the target/compiler but more generically:
OS (arch or runner, Debug/Release)like the image below: