-
Notifications
You must be signed in to change notification settings - Fork 64
[ci] Rework test_runs_on plumbing for release workflows. #1100
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
Changes from 5 commits
caf9445
661b390
f5f51e9
e0d25c8
58f28cc
5c29800
7cee3eb
f668ede
7106d5b
3aeb303
eb7a048
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,10 +26,18 @@ on: | |
workflow_dispatch: | ||
inputs: | ||
amdgpu_family: | ||
required: true | ||
type: string | ||
type: choice | ||
options: | ||
- gfx110X-dgpu | ||
- gfx1151 | ||
- gfx120X-all | ||
- gfx94X-dcgpu | ||
- gfx950-dcgpu | ||
default: gfx94X-dcgpu | ||
test_runs_on: | ||
description: The runner label to use for tests. Leave this empty to skip testing. | ||
type: string | ||
default: linux-mi325-1gpu-ossci-rocm | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am a bit concerned that having a default will result in building for one family from the choices but missing to set the correct label here and testing on the wrong family afterwards. Not having a default would mean to know / look up one more label. Though I am not blocking on this even though still think it would be nice to not know the labels at all. If testing can be skipped here, how does this colludes with #1072? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, I can try restoring the script we had and fixing it. We're still not running pytorch tests on Windows until this lands though, since gfx1151 is not triggering test jobs :/ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pushed some changes. I'll test them then re-request review once they are ready. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! |
||
release_type: | ||
description: The type of release to build ("nightly", or "dev") | ||
type: string | ||
|
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also
string
fieldI think limiting to only families that we have here makes sense though:
TheRock/build_tools/third_party/s3_management/manage.py
Lines 38 to 44 in 231dd86
(but we will want to expand that list, and now we have multiple locations to update)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, needing to update multiple locations is unfortunate but I agree that it makes sense to limit it here instead of having a freeform string.