Repository navigation
[OMNIML-6181] [11/11] Share one Megatron worker pool across the gpu_megatron modules - #2855
Open
shengliangxu wants to merge 3 commits into
Open
shengliangxu wants to merge 3 commits into
shengliangxu wants to merge 3 commits into
Conversation
shengliangxu
added this pull request to stack #2856
October 17, 2026 01:41
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Contributor
This was referenced Oct 17, 2026
[OMNIML-6179] [5/11] Make the ONNX tests cheaper: CPU-only ones to the unit lane, small models
#2849
Open
Collaborator
Author
|
/ok to test 3c4611f |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## shengliangx/ci-10-pool-dead-worker #2855 +/- ##
===================================================================
Coverage 78.94% 78.94%
===================================================================
Files 658 658
Lines 74023 74023
===================================================================
+ Hits 58435 58436 +1
+ Misses 15588 15587 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Collaborator
Author
|
/ok to test 9f2fca6 |
Collaborator
Author
|
/ok to test ca6126d |
shengliangxu
removed this pull request from stack #2856
October 17, 2026 06:58
shengliangxu
force-pushed
the
shengliangx/ci-11-megatron-session-pool
branch
from
October 17, 2026 07:02
ca6126d to
dee192a
Compare
shengliangxu
added this pull request to stack #2859
October 17, 2026 07:02
Collaborator
Author
|
/ok to test dee192a |
shengliangxu
force-pushed
the
shengliangx/ci-11-megatron-session-pool
branch
from
October 18, 2026 23:15
dee192a to
adfbd48
Compare
shengliangxu
marked this pull request as ready for review
October 19, 2026 03:45
shengliangxu
force-pushed
the
shengliangx/ci-11-megatron-session-pool
branch
from
October 19, 2026 21:34
adfbd48 to
1812f62
Compare
Each of the 15 gpu_megatron modules that use `dist_workers` started its own
worker pool, about 19 of the lane's 36 minutes. Fourteen logged module starts
took 75-77 s each (the GDN module 179 s: 76 s pool plus 103 s Triton JIT), plus
3.6-10 s of shutdown, and the start has a p90 of 94-98 s.
The 76 s is not distributed init (an in-process init takes 0.1 s): the spawned
child unpickles megatron_worker_teardown by reference, which imports the heavy
tests/gpu_megatron/conftest.py (transformers, megatron.core, all of modelopt and
its plugins, TE) before it reaches init_process_group. The same import chain
takes pytest itself 89 s to start here against 17 s in the gpu lane, whose
session-scoped pool, with a light teardown function, starts in 1.7 s. Module scope
was chosen when a spawn cost about 10 s ("avoids cross-test state contamination");
the git history shows no contamination incident, and the cost has grown 7.6x.
_pool_cache is now session-scoped, so the pools are keyed by world size and
shared by every module. The dist_workers fixtures stay function-scoped and get
their pool from get_live_pool, which replaces a pool that a hang killed. What
isolation the module scope gave is kept explicitly:
- megatron_worker_teardown, after every job: synchronizes CUDA first (an
asynchronous CUDA error then surfaces in the test that caused it), destroys
Megatron's model-parallel state as before, and now also the process groups it
leaves behind (about 20 NCCL groups per dispatch, each with a communicator and a
watchdog thread; test_megatron.py alone dispatches about 110 times). The loop is
the one open PR #2836 adds for the same leak, comment included: whichever PR
lands second sees an identical hunk. A pool that lives through the whole session
needs it more than a per-module pool did;
- _reset_shared_pools, an autouse module-scoped fixture: when a module starts, it
resets the process-global torch settings that helpers like set_seed change
(cudnn and TF32 flags, deterministic algorithms, RNG) to what a fresh worker
starts with (reset_worker_state, which the workers prepare by recording those
settings right after they start), in the pools that already exist. It is not part
of _get_pool, which runs once per test and would add a job to each;
- get_live_pool and the pool's abort handling: a job that times out, is interrupted
or loses a worker kills the pool and the next test gets a fresh one, for the cost
of one pool start. That also covers CUDA poisoning after the known Blackwell
illegal-memory-access flake.
Expected saving: about 17 min of the gpu_megatron job (36 min of tests to about
19), and one pool start instead of 15 means fewer chances to hit the ready
timeout.
Testing: reset_worker_state is covered by a gloo test in
tests/gpu/torch/utils/test_distributed_worker_pool.py (it runs on CPU): flags and
seed changed in a job are back at their spawn-time values afterwards. The Megatron
lane needs GPUs, so this PR's gpu_megatron job is the check. In an earlier CI run of
the whole series (with the Triton cache of another change), the lane's pytest step
took 862 s on cold caches and 336 s on warm ones, against 2,022 s for a baseline
run.
Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
… erroring a module The session-wide pool of the previous commit was reused after a job that failed on every rank and left its workers' CUDA context poisoned. In the first CI run of this PR, test_lora_then_quantize hit the known Blackwell "illegal memory access" in an NCCL broadcast. A CUDA error of that kind is sticky, so the next jobs in the same workers failed too, and because _reset_shared_pools ran a job in the poisoned pool at the start of every module, every test of every later module errored, including the ones that use no pool (3 failed, 180 errors). With a pool per module such a flake cost one module. - DistributedWorkerPool.run retires the pool after any failed job: the workers are killed (without the stack dump, which only a hang needs) and the pool is marked broken, so get_live_pool hands the next test a new one. Until now only a hang, an interrupt or a dead worker did that, and a job that failed on every rank left the pool in use. - _reset_shared_pools no longer lets a failed reset error the module. The pool is retired by run(), and the next test that needs one gets a new pool. Open PR #2836 makes the same change to run() for its own reasons (a failed rank can be stuck in its teardown); the two are compatible. Testing: tests/gpu/torch/utils/test_distributed_worker_pool.py (gloo, runs on CPU) has a case where rank 0 fails and rank 1 returns at once, so every rank replies: the pool is broken, its workers are dead, and another run fails at once. It fails without the change. The Megatron lane needs GPUs, so this PR's gpu_megatron job is the check. Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
…ron teardown The first two CI runs of this PR each hit "an illegal memory access was encountered" in test_megatron_peft (test_lora_then_quantize, then test_forward_with_one_lora[lora_config1]), after about 230 jobs in the shared worker. The lane almost never fails that way on other PRs, and it passed in the three runs of the draft this change was split from, whose teardown collected garbage right after destroying the groups; the version in this PR did not. The cause is not established. The groups are destroyed explicitly, but the finished test's Megatron and TE modules can still reference them, and without a collection their finalizers run at an arbitrary point of the next job, while its NCCL kernels may be in flight. Collecting right after the groups are destroyed, with nothing running, is the safe place for that. The cost is one collection per job, about 0.1-0.3 s. Testing: the Megatron lane needs GPUs, so this PR's gpu_megatron job is the check. The failure was intermittent, so the job has to pass repeatedly: it is re-run on this commit. Signed-off-by: Shengliang Xu <shengliangx@nvidia.com>
shengliangxu
force-pushed
the
shengliangx/ci-11-megatron-session-pool
branch
from
October 20, 2026 01:16
1812f62 to
37ef132
Compare
This branch has not been deployed
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
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.
What does this PR do?
Type of change: CI (test runtime); changes
tests/gpu_megatronandtests/_test_utilsonlyEach of the 15 gpu_megatron modules that use
dist_workersstarted its own worker pool: about 19 of the lane's 36 minutes. Fourteen logged module starts took 75-77 s each (the GDN module 179 s: 76 s pool plus 103 s Triton JIT), plus 3.6-10 s of shutdown, and the start has a p90 of 94-98 s.The 76 s is not distributed init (an in-process init takes 0.1 s): the spawned child unpickles
megatron_worker_teardownby reference, which imports the heavytests/gpu_megatron/conftest.py(transformers, megatron.core, all of modelopt and its plugins, TE) before it reachesinit_process_group. The same import chain takes pytest itself 89 s to start here against 17 s in the gpu lane, whose session-scoped pool, with a light teardown function, starts in 1.7 s. Module scope was chosen when a spawn cost about 10 s ("avoids cross-test state contamination"); the git history shows no contamination incident, and the cost has grown 7.6x._pool_cacheis now session-scoped, so the pools are keyed by world size and shared by every module. Thedist_workersfixtures stay function-scoped and get their pool fromget_live_pool, which replaces a pool that a hang killed. What isolation the module scope gave is kept explicitly:megatron_worker_teardown, after every job: synchronizes CUDA first (an asynchronous CUDA error then surfaces in the test that caused it), destroys Megatron's model-parallel state as before, and now also the process groups it leaves behind (about 20 NCCL groups per dispatch, each with a communicator and a watchdog thread;test_megatron.pyalone dispatches about 110 times). The loop is the one open PR CI follow-up: cache source-built wheels and extensions, bump CI containers, trim slow GPU tests #2836 adds for the same leak, comment included, so whichever PR lands second sees an identical hunk. A pool that lives through the whole session needs it more than a per-module pool did._reset_shared_pools, an autouse module-scoped fixture: when a module starts, it resets the process-global torch settings that helpers likeset_seedchange (cudnn and TF32 flags, deterministic algorithms, RNG) to what a fresh worker starts with (reset_worker_state; the workers record those settings right after they start), in the pools that already exist. It is not part of_get_pool, which runs once per test and would add a job to each.get_live_pooland the pool's abort handling: a job that times out, is interrupted, loses a worker or fails on any rank retires the pool, and the next test gets a fresh one, for the cost of one pool start.DistributedWorkerPool.runretires a pool after a plain failed job because a CUDA error such as the known Blackwell illegal memory access is sticky: the workers stay poisoned, and with a session-wide pool every later test would fail with it._reset_shared_poolsalso no longer lets a failed reset error the module's tests (they may not use a pool at all).Measured saving on top of #2846's sysmon core: 216 s of the gpu_megatron lane (1,038 s to 822 s; see the time section). Without sysmon a pool start costs 76 s, and the saving would be about 17 min (an estimate; the series was not run that way). One pool start instead of 15 also means fewer chances to hit the ready timeout.
Time improvement (measured in CI)
gpu_megatron lane, pytest time:
This PR on its own: 1,038 s -> 822 s, -216 s (-21%), against the two earlier runs (#2852: 1,029 s, #2854: 1,038 s). This PR's run was on a slightly slower runner (the heavy
callphases of the tests it does not change took 6% longer than in #2854), so the gain is not runner luck. The setups of 12 s or more, which are the pool starts and the module-level compiles, fall from 16 (489 s) to 3 (246 s): the pool starts once instead of in each of 14 modules, and what remains is the extension build (124 s), the GDN module's Triton JIT (105 s) and one 18 s pool start.With sysmon (#2846) a pool start takes 18-27 s instead of 75-77 s, so each avoided start saves less than the 76 s this PR was designed around. Without #2846 the saving would be 14 starts of 76 s plus the 3.6-10 s shutdowns, about 17 min; that is an estimate, because the series was never run without sysmon on this PR.
This is one CI run of the final head: the two runs before it failed on the sticky CUDA error and on the missing
gc.collect()described below.Method: every PR of the series has its own CI run, so "before" is the run of the PR(s) below that lack the change and "after" the runs that carry it. Times come from the job logs (
--durations, or the gap between consecutive result lines). Where runner speed matters, the tests that no PR of the series touches scale each run, and the raw and the scaled medians are both shown. One test varies by 15-30% from run to run on its own, so smaller differences are not evidence.Usage
Nothing user-facing (test fixtures).
Testing
reset_worker_statehas a gloo test next to the other pool tests (it runs on CPU): flags and seed changed in a job are back at their spawn-time values afterwards; it fails when the helper forgets one of the settings.test_lora_then_quantizeand poisoned the shared pool, and every later module errored (3 failed, 180 errors), because the pool was reused after a failed job and_reset_shared_poolsran a job in it at each module start. The second commit retires a pool after any failed job (a gloo test on CPU covers it and fails without the change) and makes a failed reset harmless; with it the lane ended with 1 failure instead of 183, intest_forward_with_one_lora[lora_config1](the same CUDA error). Re-running the job on that commit failed at the same test again, so it was not a random flake: the teardown of this PR, unlike the draft's, did not collect garbage after destroying the groups, and the draft passed the lane three times. The third commit adds thatgc.collect(); the lane then passes. The cause is not established (the finished test's Megatron and TE modules can still reference the destroyed groups, and their finalizers then run at an arbitrary point of the next job). Open CI follow-up: cache source-built wheels and extensions, bump CI containers, trim slow GPU tests #2836 makes the same change torun()for its own reasons; the two are compatible, but its teardown loop has no collection, which a session-wide pool needs.Before your PR is "Ready for review"
Make sure you read and follow Contributor guidelines and your commits are signed (
git commit -s -S).Make sure you read and follow the Security Best Practices (e.g. avoiding hardcoded
trust_remote_code=True,torch.load(..., weights_only=False),pickle, etc.).CONTRIBUTING.md: no new dependencytests/gpu/torch/utils/test_distributed_worker_pool.py(gloo, CPU) for the new helper; the session pool itself is exercised by the whole gpu_megatron laneAdditional Information
Slice 11 of 11 of the GPU-CI speed-up work, split out of the reference draft #2730 (not for merge; it holds the whole series with its measurements). Merge in order; each PR stands on its own and passes CI without the later ones.
Summary by CodeRabbit