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

Jump to bottom

ci: Selective triggering #775

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

ci: Selective triggering #775

wants to merge 6 commits into from

Conversation

ko3n1g
Copy link
Contributor

@ko3n1g ko3n1g commented Apr 30, 2024 •
edited
Loading

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
Triggering a subset of the pre-submit CI (ci.yaml) #255
Open
@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
yhtang
yhtang requested changes Apr 30, 2024
View reviewed changes
.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
Merged
@ko3n1g
Copy link
Contributor Author

ko3n1g commented May 15, 2024 •
edited by github-actions bot
Loading

/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
DwarKapex
DwarKapex reviewed Jun 3, 2024
View reviewed changes
.github/workflows/ci.yaml Outdated
-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 •
edited
Loading

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?

DwarKapex
DwarKapex reviewed Jun 3, 2024
View reviewed changes
.github/workflows/ci.yaml Outdated
@@ -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 •
edited
Loading

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

@ko3n1g
ci: Fix global state of env var
a2e834f 
Signed-off-by: Oliver Koenig <[email protected]>
ashors1
ashors1 previously approved these changes Jun 6, 2024
View reviewed changes
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!

terrykong
terrykong reviewed Jun 7, 2024
View reviewed changes
.github/workflows/ci.yaml
@@ -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
View reviewed changes
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

@ko3n1g
fix choice input
3fc4716 
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
Reviewers

@ashors1 ashors1 ashors1 left review comments

@terrykong terrykong terrykong left review comments

@yhtang yhtang Awaiting requested review from yhtang

@DwarKapex DwarKapex Awaiting requested review from DwarKapex

@olupton olupton Awaiting requested review from olupton

@nouiz nouiz Awaiting requested review from nouiz

@kocchop kocchop Awaiting requested review from kocchop

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Triggering a subset of the pre-submit CI (ci.yaml)
5 participants
@ko3n1g @DwarKapex @yhtang @terrykong @ashors1