feat(dynamo): add managed Slurm generation with W&B telemetry - #3391
Conversation
|
Auto-sync is disabled for ready for review pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
/ok to test 9fc9146 |
|
/ok to test b3a726120d635750185175db9ae90be59fab92e8 |
|
/ok to test b3a7261 |
|
/ok to test 9d4f807 |
|
/ok to test 878db7b |
|
/ok to test 3afa5c7 |
|
/ok to test c4b2b5c |
|
/ok to test 7224c0d |
7224c0d to
321c9b5
Compare
|
/ok to test 321c9b5 |
terrykong
left a comment
There was a problem hiding this comment.
Thorough review of the managed Dynamo backend — 46 inline comments. Thanks for a genuinely well-built integration: the _ArgvBuilder conflict detection, the git apply --check gate on the vLLM #44814 backport, the pre-flight dynamo.vllm argv validation in the engine's own venv, and FixedDynamoWorkerPool.validate() checking GPU-reservation liveness before every refit are all careful work that most PRs this size don't do.
Most comments open with an explicit action count so you can triage. A few themes:
Correctness — verified, not speculative
- TRT-LLM silently lost its NeMo-Gym spin-up. The new
elif backend == "dynamo":was inserted immediately before theif enable_nemo_gym:block that ended the trtllm branch, so it now belongs to dynamo. The diff renders it as context because the lines are unchanged.grpo.py:1300. - No CI shard collects the five new test files (37 tests). Proven by a genuinely failing assert in
test_swe_dynamo_r2_config.py:68sitting green today. NEMO_RL_PY_EXECUTABLES_SYSTEM=1in both launchers routesMegatronPolicyWorkerto an interpreter without megatron-core. That flag's only valid home is the NGC PyTorch image, whose venv is built--system-site-packages; the Dynamo image derives fromdocker/Dockerfile, which isn't.VLLM_PORTis never forwarded, leaving vLLM's TP rendezvous port kernel-ephemeral — the exact failure #2380 was written to prevent.- AREAL-style cache invalidation silently degrades to Magistral-style, because
invalidate_kv_cache()runs on the unpickled collector copy where_managed_runtimeisNone. expert_parallel_sizeis accepted and ignored; the vLLM backend rejects the same configuration.
Design — worth settling before this lands
The refit is wired as isinstance(policy_generation, DynamoGeneration) branches in the legacy path rather than through WeightSynchronizer, which #2444, #2608 and #2971 all landed through. DynamoGeneration already implements every method CollectiveWeightSynchronizer calls, with matching signatures, so the port is smaller than it looks — and it's the kind of shape that gets much more expensive to change after merge. Related: two state clusters in DynamoGeneration (metrics sampling, refit) have closed state and independent lifecycles, and extracting them would take __setstate__ from 19 attributes to 4.
Feature gaps that fail quietly
Speculative decoding, FP8/MXFP8 and NVFP4 all work on the vLLM backend and none work here — but only one of them errors, and on a misleading message. Guards requested; implementing the features is not.
Config and ports
oc.env usage, port bands above the ephemeral floor, engine_world_size being user-supplied where every sibling derives it, and dynamo_python requiring an absolute container path that no other interpreter in the repo does.
Non-blocking follow-ups are tracked in #3459 with stable LE-n ids so comments can point at a specific one; items we're asking for in this PR have been struck there so the two don't disagree.
Happy to be wrong on any of these — several comments are questions rather than findings, and where I couldn't construct a failing case I've said so.
321c9b5 to
b80245d
Compare
|
/ok to test b80245d |
b80245d to
37a24cc
Compare
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Co-authored-by: Terry Kong <terrycurtiskong@gmail.com> Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
49e7f97 to
e4c8b79
Compare
|
/ok to test e4c8b79 |
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
|
/ok to test f270519 |
yuki-97
left a comment
There was a problem hiding this comment.
LGTM. CI is fully green on f270519, including L1_Functional_Tests_Dynamo.
Two non-blocking things I hit while reading the telemetry path are tracked in #3459 rather than held against this PR (summary):
- LE-20 — no arm64 / GB200 coverage:
L1_Functional_Tests_Dynamois filtered out of the GB200 functional plan and the GB200 image never getsBUILD_DYNAMO=1, thoughdocker/dynamo/install.shhandlesarm64itself. - LE-21 —
generation_metrics/*logs each aliased Dynamo gauge next to its surviving vLLM twin, and one alias source went unreachable ine4c8b79b6. Plot noise only.
Rebuilds the MXFP8 refit-prequantize path on main's CollectiveSenderSpec model (NVIDIA-NeMo#3391): the generation side now owns refit buffer geometry, so the PR's consumer-side buffer_size_bytes plumbing and _resolve_target_packed_tensor_size are dropped in favor of main's interfaces, with the producer-side buffer_size_bytes > 0 guard re-added. Narrows main's grouped-MoE MXFP8 refit guard (NVIDIA-NeMo#3477) to reject only on-the-fly quantization, letting trainer-prequantized E4M3 slabs pass through. Rebuilds the refit trigger on main's unconditional prepare_for_generation structure (NVIDIA-NeMo#2884) while keeping the MegatronGeneration reshard branch. Adds a sender/receiver MXFP8 bit-equivalence test. Signed-off-by: seonjinn <sna@nvidia.com>
Summary
This PR is independent of #2990 and targets current
main. It excludes Kubernetes, DGD, external Dynamo deployments, and the NeMo-Gym submodule update.Container
ai-dynamo[vllm]==1.3.0.post1as the inference backend in isolated Python 3.12/opt/dynamo_venvValidation
/opt/dynamo_venv: ai-dynamo 1.3.0.post1, vLLM 0.23.0, no SGLang/TRT-LLM, and the #44814 patch present in installed sourceL1_Functional_Tests_Dynamogreen on f270519, assertingmax(train/token_mult_prob_error) < 1.05train/token_mult_prob_errormedian 1.0376 / final 1.0477, mean generation KL 0.003654, four refits and four cache invalidations, no leaked managed processes — full report in feat(dynamo): add managed Slurm generation with W&B telemetry #3391 (comment)