-
-
Notifications
You must be signed in to change notification settings - Fork 31.1k
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-128515: Add BOLT build to CI #128845
gh-128515: Add BOLT build to CI #128845
Conversation
29351fc
to
1d7ab1e
Compare
Copied from the JIT workflow
Interesting, edit: This occurs because |
I encountered a couple blockers for aarch64, a failed assertion in the instrumented binary
and (after hacking past that) a segfault in BOLT
I dropped aarch64 in 684ece4 — we can add it later. |
A few tests are failing after BOLT optimization. I'd appreciate some guidance on that.
|
The timing on this actually seems pretty reasonable at 13 minutes. We could expand this to perform other build optimizations, e.g., PGO, to verify they're working as intended? Right now it's just BOLT though. |
Two things
|
Can you expand on this comment?
Sounds good to me — should I open an issue to investigate why they fail too? Like is the profiler actually broken? |
Because we skip several tests with BOLTed binary, PGO + LTO can not check the regression issue where tests are skipped. Currently, PGO + LTO is the standard optimization policy of the CPython project.
Yeah, we should; maybe @pablogsal is interested in this issue. |
# Do not test BOLT with free-threading, to conserve resources | ||
- bolt: true | ||
free-threading: true | ||
# BOLT currently crashes during instrumentation on aarch64 | ||
- os: ubuntu-24.04-aarch64 | ||
bolt: true |
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.
I don't have strong feelings about this pattern (using exclude
instead of include
), but liked that I could document why we're not running the additional cases.
@@ -246,10 +250,17 @@ jobs: | |||
exclude: |
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.
Not a strong opinion, but I would prefer to have just 1, 2, or 3 jobs with bolt. unless it is absolutely needed.
We can move some very specific builds to buildbots, while maintatining the bare minimum in CI.
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.
This is just one job with BOLT — I think in the future we'd want a second job for aarch64 once that's unblocked. Are you suggesting I should frame this as an include
instead? ref #128845 (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.
Yes! Sorry for not being clear :)
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.
I'll wait to change it until others have a chance to weigh in, but I'm not opposed.
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.
I started to do this but found it awkward since I know we want aarch64 testing here eventually. Since @hugovk 👍 my comment at #128845 (comment) I think I'll leave it for now.
run: >- | ||
PROFILE_TASK='-m test --pgo --ignore test_unpickle_module_race' |
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.
Interesting, why doesn't it raise any issues while we build PGO build?
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.
I don't think there is a PGO build in CI
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.
(The read-only build file system looks specific to this CI setup)
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.
I don't think there is a PGO build in CI
FYI, At Github Action there is no CI for PGO and LTO.
But at build bot we run CI for the PGO and LTO.
https://buildbot.python.org/#/builders/378/builds/1554
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.
(The read-only build file system looks specific to this CI setup)
Let me take a look more detail. :)
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.
https://github.com/python/cpython/actions/runs/12776586124/job/35615597427
ERROR: test_unpickle_module_race (test.test_pickle.PyUnpicklerTests.test_unpickle_module_race)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/support/import_helper.py", line 48, in forget
unlink(source + 'c')
~~~~~~^^^^^^^^^^^^^^
File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/support/os_helper.py", line 345, in unlink
_unlink(filename)
~~~~~~~^^^^^^^^^^
OSError: [Errno 30] Read-only file system: '/home/runner/work/cpython/cpython-ro-srcdir/Lib/locker.pyc'
I think it's fine to remove the test from the optimization suite. It seems likely for there to be some problems here due to the read-only setup. It's known that the tests require a writeable source directory
cpython/.github/workflows/reusable-ubuntu.yml
Lines 105 to 110 in ae31df3
- name: Remount sources writable for tests | |
# some tests write to srcdir, lack of pyc files slows down testing | |
run: sudo mount "$CPYTHON_RO_SRCDIR" -oremount,rw | |
- name: Tests | |
working-directory: ${{ env.CPYTHON_BUILDDIR }} | |
run: xvfb-run make ci |
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.
#29904 added the read-only out of tree builds.
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.
(We could disable the read-only builds for the BOLT job but it seems more painful than it's worth)
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.
Ah okay let’s exclude the test with current way and let’s pile the issue about the test suite problem. Thank you for the investigation:)
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.
@hugovk Do we have any reason to mount file system as the read-only?
Zanie found it was added in #29904, which gives this reason:
The Ubuntu test runner now configures and compiles CPython out of tree.
The source directory is a read-only bind mount to ensure that the build
cannot create or modify any files in the source tree.
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.
Looks good!
run: >- | ||
PROFILE_TASK='-m test --pgo --ignore test_unpickle_module_race' |
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.
@hugovk Do we have any reason to mount file system as the read-only?
Zanie found it was added in #29904, which gives this reason:
The Ubuntu test runner now configures and compiles CPython out of tree.
The source directory is a read-only bind mount to ensure that the build
cannot create or modify any files in the source tree.
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.
lgtm! Thanks you for the work!
Adds BOLT test coverage to CI, which will allow us to prevent regressions and move towards stabilization of this feature.
Of note:
test_pickle
case from the profiling corpus because we use a RO file system for builds