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.

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

ci: Selective triggering #775

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

ko3n1g
Copy link
Contributor

@ko3n1g ko3n1g commented Apr 30, 2024

tl;dr: #255

@ko3n1g ko3n1g force-pushed the ko3n1g/ci/selective-triggering branch 3 times, most recently from a1182b4 to 828a226 Compare April 30, 2024 12:30
@ko3n1g ko3n1g linked an issue Apr 30, 2024 that may be closed by this pull request
@ko3n1g ko3n1g force-pushed the ko3n1g/ci/selective-triggering branch 2 times, most recently from aebab6e to 20161c0 Compare April 30, 2024 13:15
@ko3n1g ko3n1g marked this pull request as ready for review April 30, 2024 13:18
@ko3n1g ko3n1g force-pushed the ko3n1g/ci/selective-triggering branch from 20161c0 to 04b10df Compare April 30, 2024 13:24
@ko3n1g ko3n1g requested review from yhtang and DwarKapex April 30, 2024 13:43
.github/workflows/_ci.yaml Outdated Show resolved Hide resolved
@ko3n1g ko3n1g force-pushed the ko3n1g/ci/selective-triggering branch 10 times, most recently from 4f117b6 to a1b346c Compare May 6, 2024 18:08
@ko3n1g ko3n1g mentioned this pull request May 7, 2024
@ko3n1g
Copy link
Contributor Author

ko3n1g commented May 15, 2024

/ci

---

Watchdog 🤖: presubmit CI was automatically triggered. Click here to navigate to the workflow run.

@ko3n1g ko3n1g force-pushed the ko3n1g/ci/selective-triggering branch 2 times, most recently from b03dba0 to f6f012b Compare May 15, 2024 09:05
@ko3n1g
Copy link
Contributor Author

ko3n1g commented May 15, 2024

Hi everyone! (@terrykong and @DwarKapex and @yhtang),
I've updated the watchdog to respond to subsets. You can see some examples (and also test it yourself) in my own fork.

@ko3n1g ko3n1g requested review from terrykong and yhtang May 15, 2024 09:07
@terrykong terrykong marked this pull request as draft May 20, 2024 19:37
@ko3n1g ko3n1g force-pushed the ko3n1g/ci/selective-triggering branch from a715cbc to b2c45fd Compare May 21, 2024 08:45
@ko3n1g ko3n1g marked this pull request as ready for review May 21, 2024 08:45
@ko3n1g ko3n1g force-pushed the ko3n1g/ci/selective-triggering branch from b2c45fd to ede503b Compare May 31, 2024 15:49
@ko3n1g ko3n1g requested a review from terrykong May 31, 2024 15:49
-H "Accept: application/vnd.github.v3+json" \
"https://api.github.com/repos/${{ github.repository }}/actions/runs/${{ github.run_id }}/cancel"
while true; do sleep 1; done # blocks execution in case workflow cancellation takes time
# - name: Cancel workflow run if the trigger is a draft PR
Copy link
Contributor

@DwarKapex DwarKapex Jun 3, 2024

Choose a reason for hiding this comment

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

Remove dead code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

shouldn't be dead in the first place

Copy link
Contributor

Choose a reason for hiding this comment

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

You're going to have another PR for this step?

@@ -416,7 +440,7 @@ jobs:

finalize:
needs: [metadata, amd64, arm64, publish-containers]
if: "!cancelled()"
if: '!cancelled()'
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make it consistent:

if: ${{ !cancelled() }}

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are already a lot of consistencies, like https://github.com/NVIDIA/JAX-Toolbox/blob/main/.github/workflows/_ci.yaml#L11 vs https://github.com/NVIDIA/JAX-Toolbox/blob/main/.github/workflows/ci.yaml#L419

I think we should address it in a different PR to not bloat this one. Maybe we should think about some pre-commit checks for linting and formatting?

Copy link
Contributor

@DwarKapex DwarKapex Jun 5, 2024

Choose a reason for hiding this comment

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

Do you know any linter for Github Actions?

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'd need to test a few, I only use VSCode's markdown extension for formatting. I just heard of https://github.com/marketplace/actions/markdown-lint before but never tried it myself

@ko3n1g ko3n1g force-pushed the ko3n1g/ci/selective-triggering branch from ede503b to 9a8fe89 Compare June 5, 2024 13:57
@ko3n1g ko3n1g force-pushed the ko3n1g/ci/selective-triggering branch from 918c493 to eaf7c19 Compare June 5, 2024 16:03
@ko3n1g ko3n1g requested a review from DwarKapex June 5, 2024 16:05
@DwarKapex
Copy link
Contributor

Looks good to try =)

Signed-off-by: Oliver Koenig <[email protected]>
ashors1
ashors1 previously approved these changes Jun 6, 2024
Copy link
Contributor

@ashors1 ashors1 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks a lot Oliver!

@@ -34,6 +29,22 @@ on:
A comma-separated PACKAGE=URL#REF list to override sources used by build.
PACKAGE∊{JAX,XLA,Flax,transformer-engine,T5X,paxml,praxis,maxtext,levanter,haliax,grok,mujuco,mujuco-mpc,gemma,big-vision,common-loop-utils,flaxformer,panopticapi} (case-insensitive)
default: ''
TEST_SUBSET:
type: string
options:
Copy link
Contributor

Choose a reason for hiding this comment

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

I actually don't see these options from the web. I tried entering foobar and it didn't stop the run. Any ideas if that's expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch! Probably some dev/debugging artifact, but tested in my fork that switching type: choice doesn't break the workflow so changed it

terrykong
terrykong previously approved these changes Jun 7, 2024
Copy link
Contributor

@terrykong terrykong left a comment

Choose a reason for hiding this comment

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

Other than my one comment. LGTM

Signed-off-by: Oliver Koenig <[email protected]>
@ko3n1g ko3n1g dismissed stale reviews from terrykong and ashors1 via 3fc4716 June 14, 2024 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Triggering a subset of the pre-submit CI (ci.yaml)
5 participants