Skip to content

Make Github Run ID selection stricter, update docs - #170

Merged
jameslamb merged 4 commits into
mainfrom
gh-artifacts/stricter-run-selection
May 8, 2025
Merged

Make Github Run ID selection stricter, update docs#170
jameslamb merged 4 commits into
mainfrom
gh-artifacts/stricter-run-selection

Conversation

@jameslamb

@jameslamb jameslamb commented May 8, 2025

Copy link
Copy Markdown
Member

Contributes to https://github.com/rapidsai/build-infra/issues/237

Proposes the following changes to scripts involved in handling GitHub Actions artifacts:

  • in rapids-github-run-id, implements explicit sorting to get the latest run
    • see inline thread
  • removes --created from rapids-github-run-id code for nightly builds
    • see inline thread
  • expands documentation in rapids-github-run-id and rapids-prompt-local-repo-config
  • across multiple scripts, removes phrase "A utility script that..." from the beginning of docs
    • this does not add any information

Notes for Reviewers

How I tested this

Looked for a repo that doesn't get a lot of commits, so I could test that case where one query with gh run list was likely to return multiple runs for the same commit.

Chose this cugraph-gnn commit: rapidsai/cugraph-gnn@33bccfe

Sure enough, there were multiple runs for it.

RAPIDS_BUILD_TYPE="nightly" \
RAPIDS_REPOSITORY="rapidsai/cugraph-gnn" \
RAPIDS_REF_NAME="branch-25.06" \
RAPIDS_SHA="33bccfe413357deeb4938d8c54182edaca59b1f0" 
gh run list \
    --repo "${RAPIDS_REPOSITORY}" \
    --branch "${RAPIDS_REF_NAME}" \
    --commit "${RAPIDS_SHA}" \
    --workflow "build.yaml"

Output:

STATUS  TITLE  WORKFLOW  BRANCH        EVENT              ID           ELAPSED  AGE             
✓       build  build     branch-25.06  workflow_dispatch  14878118285  26m40s   about 1 day ago
✓       build  build     branch-25.06  workflow_dispatch  14855651986  34m43s   about 2 days ago
✓       build  build     branch-25.06  workflow_dispatch  14830840366  27m25s   about 3 days ago
✓       build  build     branch-25.06  workflow_dispatch  14818738942  26m11s   about 4 days ago
✓       build  build     branch-25.06  workflow_dispatch  14809159545  30m33s   about 5 days ago
✓       build  build     branch-25.06  workflow_dispatch  14791168607  26m35s   about 6 days ago
✓       build  build     branch-25.06  workflow_dispatch  14777188917  27m26s   about 7 days ago
X       build  build     branch-25.06  workflow_dispatch  14777127020  3m28s    about 7 days ago

In this case, run 14878118285 is the latest, and so the one we want.

Tested that that's what is returned:

RAPIDS_BUILD_TYPE="nightly" \
RAPIDS_REPOSITORY="rapidsai/cugraph-gnn" \
RAPIDS_REF_NAME="branch-25.06" \
RAPIDS_SHA="33bccfe413357deeb4938d8c54182edaca59b1f0" \
rapids-github-run-id

# 14878118285

I've also tested this in an rmm PR: rapidsai/rmm#1909

Look at the most recent run from there: https://github.com/rapidsai/rmm/actions/runs/14914562796/job/41897151092?pr=1909

The run ID (14914562796) in the conda-cpp-tests logs matches the one in the job URL... so I think it's finding the right thing, at least for PRs.

Comment on lines +31 to +32
--json 'createdAt,databaseId' \
--jq 'sort_by(.createdAt) | reverse | .[0] | .databaseId')}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

See "How I tested this" in the PR description.

In short, gh run list does not make any strong guarantees about sorting, so just saying "choose the first element in the returned list" is not safe.

docs on gh run list: https://cli.github.com/manual/gh_run_list

--commit "${RAPIDS_SHA}" \
--workflow "${RAPIDS_BUILD_WORKFLOW_NAME:-build.yaml}" \
--event "workflow_dispatch" \
--created "${RAPIDS_NIGHTLY_DATE}" \

@jameslamb jameslamb May 8, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Proposing here that we remove --created from the call for nightly runs.

From the docs in gh run list --help:

FLAGS
  ...
      --created date      Filter runs by the date it was created
  ...

Using this flag creates an awkward association between the time when a workflow is scheduled and the nightly date it is "for".

Imagine this sequence:

  1. rmm nightly build job starts at 2025-05-10T05:00:00Z
  2. ... builds for all dependencies between rmm and cuml run...
  3. cuml nightly build job starts at 2025-05-10T23:59:59Z (almost 19 hours later)
  4. cuml nightly test job starts at 2025-05-11T01:00:00Z (the next day)

I think in that case, the test run would fail to download artifacts here:

https://github.com/rapidsai/cuml/blob/41e397d22e9985ec57f9a6ced44395efb86f2e0f/ci/test_python_common.sh#L9-L10

Because RAPIDS_NIGHTLY_DATE on the test run would be 2025-05-11, but the latest nightly artifacts were generated by a workflow run that started on 2025-05-10.

Is this likely? I don't know.

On the most recent run, it took about 3 hours from when rmm-build began to when cuml-tests began

Situations where we could encounter this problem:

  • builds take much longer than that run (e.g. because of a high rate of sccache misses)
  • we change the nightly pipeline to be triggered later than 5am UTC (code link)
  • build parallelism is worsened by some change in the dependency graph (e.g., cugraph adding a build-time dependency on cuml)

I cannot think of any situation where "get the artifacts from the latest nightly run for this specific branch + commit" would produce the wrong behavior, so even if we think the scenario I described above is unlikely, I still think we should remove --created here because it's possible.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good writeup -- I agree.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks for reading through it!

@jameslamb jameslamb changed the title WIP: Make Github Run ID selection stricter, update docs Make Github Run ID selection stricter, update docs May 8, 2025
@jameslamb
jameslamb requested review from bdice and gforsyth May 8, 2025 20:14
@jameslamb
jameslamb marked this pull request as ready for review May 8, 2025 20:14
@jameslamb
jameslamb requested a review from a team as a code owner May 8, 2025 20:14
--commit "${RAPIDS_SHA}" \
--workflow "${RAPIDS_BUILD_WORKFLOW_NAME:-build.yaml}" \
--event "workflow_dispatch" \
--created "${RAPIDS_NIGHTLY_DATE}" \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good writeup -- I agree.

@jameslamb jameslamb added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels May 8, 2025
@jameslamb
jameslamb merged commit 2544ac3 into main May 8, 2025
@jameslamb
jameslamb deleted the gh-artifacts/stricter-run-selection branch May 8, 2025 21:48
rapids-bot Bot pushed a commit that referenced this pull request Mar 10, 2026
I've been using `rapids-get-pr-artifact` a lot recently, for rapidsai/build-planning#257

Today, one of my PRs for that briefly caused `cudf` to briefly exceed its GitHub API limits: NVIDIA/cudf#21671 (comment)

This proposes some fixes to `rapids-get-pr-artifact` in the future:

* error out of a unique GitHub Actions run ID can't be determined from the inputs
  - *(instead of passing an empty string to `gh run download`, which causes it to page over **all CI runs GitHub's history retains**)*
* limit initial search for runs to just `pr.yaml` workflow
  - *(optionally overridable by env variable `RAPIDS_BUILD_WORKFLOW_NAME`)*

These should make `rapids-get-pr-artifact` faster and, more importantly, use many fewer GitHub API calls.

## Notes for Reviewers

### Haven't I seen this before?

Yep! Similar changes were made in `rapids-github-run-id`:

* #193
* #170

`rapids-get-pr-artifact` doesn't use `rapids-github-run-id` because its behavior of "use environment variable `GITHUB_RUN_ID` if it's available" is undesirable when you're trying to workflow runs in a repo other than the one `rapids-get-pr-artifact` is currently running in.

### How I tested this

Tested on NVIDIA/cudf#21671, with NVIDIA/cudf@c058c43

Authors:
  - James Lamb (https://github.com/jameslamb)

Approvers:
  - Gil Forsyth (https://github.com/gforsyth)

URL: #249
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

improvement Improves an existing functionality non-breaking Introduces a non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants