Skip to content

Conversation

ScottTodd
Copy link
Member

@ScottTodd ScottTodd commented Sep 29, 2025

Motivation

Progress on #1559. This gets us presubmit coverage for building the Python packages and running sanity check tests on them (fully implemented on Windows, almost finished on Linux).

Technical Details

As mentioned on #1596, the new build_*_python_packages.yml workflows do not yet upload the packages they build anywhere and only run sanity checks on the current (CPU build) machine. Once the workflows upload we can chain another test_python_packages.yml workflow similar to how test_artifacts.yml is already connected.

The plumbing here exposed a few issues that I'm ignoring in this PR:

  1. artifact_run_id is overloaded now, working with use_prebuilt_artifacts and standard fully pipelined build/test/release runs. See the comments I added in the files for some ideas there.
  2. package_version should be computed at the start of the pipeline and passed through to all child jobs. We can lift that code from the release workflow into somewhere shared between CI and CD. For now I'm just hardcoding to a version that functionally works, but we should fix that version before uploading these packages anywhere.
  3. amdgpu_family: ${{ inputs.amdgpu_families }} is changing from plural to singular, but other workflows use plural and then only operate on singular anyways. Might correct that in a follow-up:
    strategy:
    fail-fast: false
    matrix:
    families: ${{ fromJSON(needs.setup.outputs.linux_amdgpu_families) }}
    uses: ./.github/workflows/ci_linux.yml
    secrets: inherit
    with:
    amdgpu_families: ${{ matrix.families.family }}

Test Plan

Watch CI results. I also triggered a run with use_prebuilt_artifacts on a prior commit at https://github.com/ROCm/TheRock/actions/runs/18112810485.

@ScottTodd ScottTodd marked this pull request as ready for review September 30, 2025 19:56
artifact_run_id: ${{ inputs.artifact_run_id }}

build_portable_linux_python_packages:
needs: [build_portable_linux_artifacts]
Copy link
Member

Choose a reason for hiding this comment

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

I am aware that our CI is still flaky, but is it desired to change this to

Suggested change
needs: [build_portable_linux_artifacts]
needs: [build_portable_linux_artifacts, test_linux_artifacts]

in the future to only proceed if tests passed? Might okay to build those in parallel to get a signal earlier though. If it should be a requirement please add a comment, otherwise ignore this comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Artifact testing can take multiple hours, when I'd like early signal for package construction and sanity checks. When we upload these Python packages to an index (especially as part of release pipelines), we'll want a similar setup to what we have for the torch packages where we first upload to a staging directory/index and later promote once enough tests have passed.

@ScottTodd ScottTodd merged commit c93d078 into main Oct 1, 2025
55 of 62 checks passed
@ScottTodd ScottTodd deleted the users/scotttodd/ci-python-packages branch October 1, 2025 15:22
@github-project-automation github-project-automation bot moved this from TODO to Done in TheRock Triage Oct 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants