ci: override -vm-size to run the tests on GPU machines #3431
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
benbjohnson
approved these changes
Apr 5, 2024
dangra
reviewed
Apr 6, 2024
test/preflight/testlib/test_env.go
Outdated
| @@ -180,6 +181,10 @@ type testingTWrapper interface { | |||
|
|
|||
| // Fly runs a flyctl the result | |||
| func (f *FlyctlTestEnv) Fly(flyctlCmd string, vals ...interface{}) *FlyctlResult { | |||
| if strings.HasPrefix(flyctlCmd, "machine run ") || strings.HasPrefix(flyctlCmd, "launch ") { | |||
| flyctlCmd += " --vm-gpu-kind a100-sxm4-80gb --vm-cpu-kind performance --vm-memory 8192 " | |||
There was a problem hiding this comment.
A few notes:
a100-sxm4-80gbare scarsea100-pcie-40gborl40savailability are more common but only oniadandordregions--vm-size a100-40gbor--vm-size l40sare useful shortcuts
kzys
force-pushed
the
gpu-ad-hoc-tests
branch
4 times, most recently
from
779566e
to
582c3e2
Compare
kzys
changed the title
GPU machines have some quirks (e.g. use Cloud Hypervisor instead of Firecracker). While most of the tests don't need GPU, running these tests would increase our coverage.
It is helpful to test specific regions, including GPU testing which we need to avoid some regions.
These tests don't work on GPU machines. While we will investigate the issue further, we want to run other tests on GPU machines to prevent regressions.
dangra
reviewed
Apr 25, 2024
| if f.VMSize != "" { | ||
| t.Skip() | ||
| } | ||
|
|
There was a problem hiding this comment.
IMHO all these skips are a big red flag this PR is going in the wrong direction
What if instead of this matrix based on vmsize that requires mangling the flyctl command and skipping things like this, we do:
- Add a few more integration tests that use GPU machines
- Add a process group with a
[[vm]]section to currentfly launchexamples that also uses GPU machines - And longer but worth it, add another Fly organization only for cloudhypervisor testing, note I am not saying GPU testing here, but CH so the same set of tests don't require GPUs to run.
…ne size has a GPU.
…rs was a good idea. By which I mean no thanks. No thanks at all.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Change Summary
What and Why:
GPU machines have some quirks (e.g. use Cloud Hypervisor instead of Firecracker). While most of the tests don't need GPU, running these tests would increase our coverage.
How:
Related to:
Documentation