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.

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

gh-128515: Add BOLT build to CI #128845

Merged
merged 8 commits into from
Jan 18, 2025
Merged

gh-128515: Add BOLT build to CI #128845

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8699afc
Add BOLT support to reusable Ubuntu workflow
zanieb Jan 14, 2025
254b553
Add BOLT to build matrix
zanieb Jan 14, 2025
9b179cb
Install Clang
zanieb Jan 14, 2025
684ece4
Skip BOLT CI on aarch64
zanieb Jan 14, 2025
03be932
Ignore `test_unpickle_module_race` during BOLT profiling
zanieb Jan 14, 2025
01cb8d8
Skip perf trampoline tests when BOLT instrumented
zanieb Jan 15, 2025
5f9c410
Move `bolt` flags into alphabetical order
zanieb Jan 16, 2025
6aa007c
Merge branch 'main' into zb/bolt
corona10 Jan 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions .github/workflows/build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,10 +231,14 @@ jobs:
name: >-
Ubuntu
${{ fromJSON(matrix.free-threading) && '(free-threading)' || '' }}
${{ fromJSON(matrix.bolt) && '(bolt)' || '' }}
needs: check_source
if: needs.check_source.outputs.run_tests == 'true'
strategy:
matrix:
bolt:
- false
- true
free-threading:
- false
- true
Expand All @@ -246,9 +250,16 @@ jobs:
exclude:
Copy link
Member

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.

Copy link
Contributor Author

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)

Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

- os: ubuntu-24.04-aarch64
is-fork: true
# 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
Comment on lines +253 to +258
Copy link
Contributor Author

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.

uses: ./.github/workflows/reusable-ubuntu.yml
with:
config_hash: ${{ needs.check_source.outputs.config_hash }}
bolt-optimizations: ${{ matrix.bolt }}
free-threading: ${{ matrix.free-threading }}
os: ${{ matrix.os }}

Expand Down
15 changes: 15 additions & 0 deletions .github/workflows/reusable-ubuntu.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,11 @@ on:
config_hash:
required: true
type: string
bolt-optimizations:
description: Whether to enable BOLT optimizations
required: false
type: boolean
default: false
free-threading:
description: Whether to use free-threaded mode
required: false
Expand Down Expand Up @@ -34,6 +39,12 @@ jobs:
run: echo "::add-matcher::.github/problem-matchers/gcc.json"
- name: Install dependencies
run: sudo ./.github/workflows/posix-deps-apt.sh
- name: Install Clang and BOLT
if: ${{ fromJSON(inputs.bolt-optimizations) }}
run: |
sudo bash -c "$(wget -O - https://apt.llvm.org/llvm.sh)" ./llvm.sh 19
sudo apt-get install bolt-19
echo PATH="$(llvm-config-19 --bindir):$PATH" >> $GITHUB_ENV
- name: Configure OpenSSL env vars
run: |
echo "MULTISSL_DIR=${GITHUB_WORKSPACE}/multissl" >> "$GITHUB_ENV"
Expand Down Expand Up @@ -73,14 +84,18 @@ jobs:
key: ${{ github.job }}-${{ runner.os }}-${{ env.IMAGE_VERSION }}-${{ inputs.config_hash }}
- name: Configure CPython out-of-tree
working-directory: ${{ env.CPYTHON_BUILDDIR }}
# `test_unpickle_module_race` writes to the source directory, which is
# read-only during builds — so we exclude it from profiling with BOLT.
run: >-
PROFILE_TASK='-m test --pgo --ignore test_unpickle_module_race'
Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Contributor Author

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)

Copy link
Member

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

Copy link
Member

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

Copy link
Contributor Author

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

Copy link
Contributor Author

@zanieb zanieb Jan 16, 2025 •
edited
Loading

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.

Copy link
Contributor Author

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)

Copy link
Member

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

Copy link
Member

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.

../cpython-ro-srcdir/configure
--config-cache
--with-pydebug
--enable-slower-safety
--enable-safety
--with-openssl="$OPENSSL_DIR"
${{ fromJSON(inputs.free-threading) && '--disable-gil' || '' }}
${{ fromJSON(inputs.bolt-optimizations) && '--enable-bolt' || '' }}
- name: Build CPython out-of-tree
if: ${{ inputs.free-threading }}
working-directory: ${{ env.CPYTHON_BUILDDIR }}
Expand Down
3 changes: 3 additions & 0 deletions Lib/test/test_perf_profiler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ def tearDown(self) -> None:
for file in files_to_delete:
file.unlink()

@unittest.skipIf(support.check_bolt_optimized, "fails on BOLT instrumented binaries")
def test_trampoline_works(self):
code = """if 1:
def foo():
Expand Down Expand Up @@ -100,6 +101,7 @@ def baz():
"Address should contain only hex characters",
)

@unittest.skipIf(support.check_bolt_optimized, "fails on BOLT instrumented binaries")
def test_trampoline_works_with_forks(self):
code = """if 1:
import os, sys
Expand Down Expand Up @@ -160,6 +162,7 @@ def baz():
self.assertIn(f"py::bar_fork:{script}", child_perf_file_contents)
self.assertIn(f"py::baz_fork:{script}", child_perf_file_contents)

@unittest.skipIf(support.check_bolt_optimized, "fails on BOLT instrumented binaries")
def test_sys_api(self):
code = """if 1:
import sys
Expand Down
Loading