FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-128515: Add BOLT build to CI by zanieb · Pull Request #128845 · python/cpython · GitHub

/ cpython Public

gh-128515: Add BOLT build to CI #128845

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

Merged
corona10 merged 8 commits into from
Jan 18, 2025
Conversation 34 Commits 8 Checks 0 Files changed
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 .py  (1) .yml  (2) All 2 file types selected
Only manifest files
Viewed files
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Unified
Split
Hide whitespace
Diff view
Unified
Split
Hide whitespace
11 changes: 11 additions & 0 deletions .github/workflows/build.yml
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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:

sobolevn Jan 15, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

zanieb Jan 15, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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)

sobolevn Jan 15, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yes! Sorry for not being clear :)

zanieb Jan 16, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I'll wait to change it until others have a chance to weigh in, but I'm not opposed.

zanieb Jan 16, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

zanieb Jan 15, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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'

corona10 Jan 16, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Interesting, why doesn't it raise any issues while we build PGO build?

zanieb Jan 16, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I don't think there is a PGO build in CI

zanieb Jan 16, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

(The read-only build file system looks specific to this CI setup)

corona10 Jan 16, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

corona10 Jan 16, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

(The read-only build file system looks specific to this CI setup)

Let me take a look more detail. :)

zanieb Jan 16, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

zanieb Jan 16, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

#29904 added the read-only out of tree builds.

zanieb Jan 16, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

(We could disable the read-only builds for the BOLT job but it seems more painful than it's worth)

corona10 Jan 16, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

hugovk Jan 16, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@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
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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
Back | FazBrowse Home | New Git URL