-
Notifications
You must be signed in to change notification settings - Fork 58
[AIROCMLIR-375] Run tests in parallel across multiple GPUs #2419
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
Open
bogdan-petkovic
wants to merge
22
commits into
develop
Choose a base branch
from
users/bpetkovi/parallel-tests-multi-gpu
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
f427ee2
[AIROCMLIR-375] Run tests in parallel across multiple GPUs
bogdan-petkovic fd1bc54
[AIROCMLIR-375] Fix multi-GPU E2E driver hitting Jenkins activity tim…
bogdan-petkovic 0dfda17
Merge branch 'develop' into users/bpetkovi/parallel-tests-multi-gpu
bogdan-petkovic c3ac08b
fix yapf format checks
bogdan-petkovic 8aad30c
Merge branch 'develop' into users/bpetkovi/parallel-tests-multi-gpu
bogdan-petkovic f4cdd78
[AIROCMLIR-375] Pass per-GPU lit worker count to sharded E2E driver
bogdan-petkovic 426fb79
Merge branch 'develop' into users/bpetkovi/parallel-tests-multi-gpu
bogdan-petkovic 8074d5f
Merge branch 'develop' into users/bpetkovi/parallel-tests-multi-gpu
bogdan-petkovic 5a54e9e
Merge branch 'develop' into users/bpetkovi/parallel-tests-multi-gpu
bogdan-petkovic 331bee4
[AIROCMLIR-375] Address review: install gpu_topology, validate GPU id…
bogdan-petkovic 34cf444
Merge branch 'develop' of github.com:ROCm/rocMLIR into users/bpetkovi…
bogdan-petkovic 343fecf
Merge branch 'develop' of github.com:ROCm/rocMLIR into users/bpetkovi…
bogdan-petkovic 3eb674f
Merge branch 'develop' into users/bpetkovi/parallel-tests-multi-gpu
bogdan-petkovic b605c51
Merge branch 'develop' into users/bpetkovi/parallel-tests-multi-gpu
bogdan-petkovic 909f187
Merge branch 'develop' into users/bpetkovi/parallel-tests-multi-gpu
bogdan-petkovic 953f801
Merge branch 'develop' into users/bpetkovi/parallel-tests-multi-gpu
bogdan-petkovic 39be80c
Merge branch 'develop' into users/bpetkovi/parallel-tests-multi-gpu
bogdan-petkovic f244dd0
[AIROCMLIR-375] Move multi-GPU E2E driver into the performance scripts
bogdan-petkovic 837250c
Merge branch 'develop' into users/bpetkovi/parallel-tests-multi-gpu
bogdan-petkovic c9e14cd
[AIROCMLIR-375] Make GPU pinning reach lit tests and cap host concurr…
bogdan-petkovic 566b730
Merge remote-tracking branch 'origin/develop' into users/bpetkovi/par…
bogdan-petkovic f1ffcb1
[AIROCMLIR-375] Stream shard output and isolate the driver from GPU f…
bogdan-petkovic File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
Oops, something went wrong.
Oops, something went wrong.
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.
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.
If we are passing
limit_lit_workersas thejobsPerGpuvalue, then doesn't it mean that concurrency is much higher than we want? E.g.,setLitWorkerCount()returns 64 for gfx942, and if one of those systems has 8 GPUs then that is 8 × 64 = 512?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.
Per-GPU it's unchanged so each GPU gets the same setLitWorkerCount() cap it had when everything ran on GPU 0 which is why I figured running the same count on each GPU was safe. But you have a point that host-side it's now 8× what that box has ever seen. My inclination would be to keep the per-GPU cap but also clamp the total, something like min(jobsPerGpu, hostBudget / numGpus), so the GPU-side limit still holds on small nodes while big nodes don't oversubscribe the host. What do you think is the best approach here?
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.
The problems that we saw with high concurrency in the past were always with running on the GPU, so as long as those caps remain in place it does make sense to bump the CPU side. The only thing that should be checked is if the CPU (host) actually supports the amount of parallelism that we are requesting. I.e., should we do a check of
nprocto see how many CPU cores are actually available?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.
Done, the driver now caps at min(jobsPerGpu, hostCPUs / numShards), using sched_getaffinity (what nproc reports, and it respects the container's cpuset). Per-GPU caps are untouched. On an 8-GPU gfx942 node with 128 cores that's 8×16 instead of 8×64