feat: parse diffusion worker args natively from sglang's diffusion ServerArgs - #13667
Draft
marckarp wants to merge 2 commits into
Draft
feat: parse diffusion worker args natively from sglang's diffusion ServerArgs#13667marckarp wants to merge 2 commits into
marckarp wants to merge 2 commits into
Conversation
…rverArgs Image/video diffusion workers previously parsed CLI args with the LLM ServerArgs parser, hand-copied 13 fields into a SimpleNamespace stub, and called DiffGenerator.from_pretrained() with 5 hardcoded kwargs. Every other diffusion engine option (torch compile, quantization, batching, warmup, attention backend, ...) was silently stuck at its default, and unknown or LLM-only flags were silently absorbed. Build the worker CLI from sglang's native diffusion ServerArgs instead, mirroring how the LLM path builds its CLI from the LLM ServerArgs: - diffusion_args.py: native parser + DiffusionWorkerArgs adapter. Engine fields delegate to the parsed diffusion ServerArgs; Dynamo-side settings (--served-model-name, --enable-metrics) are registered only when the engine parser does not define them natively, so future native flags win automatically. Fields Dynamo's shared worker code probes are pinned inert. Explicitly-provided flags are communicated through the parser's side channel so resolution never depends on process argv. - args.py: diffusion/video workers branch to the native parser before any LLM-specific processing; the stub is removed. - init_diffusion.py: hand the full parsed config to DiffGenerator.from_server_args() for both image and video workers. BREAKING: LLM-only flags on diffusion worker command lines (previously silently ignored) now fail at startup with "unrecognized arguments". Remove such flags from existing deployment configs. Verification: - 126 engine fields reachable on sglang 0.5.17 (previously 5); invalid values and unknown flags now rejected with actionable errors - unit tests 9/9; live image and video deployments on B200 generate successfully with --warmup-mode server (previously unreachable) confirmed active in the resolved engine config - resolved-config diff vs the stub path: no unintended changes; the stub was also silently dropping --trust-remote-code and nulling dist_timeout, both now honored - assembled check against sglang v0.5.17 + current dynamo wheel: worker boots and serves /v1/images/generations end-to-end Signed-off-by: Marc Karp <mkarp@nvidia.com> Co-Authored-By: Claude <noreply@anthropic.com>
…chers Address review findings on the native diffusion arg parsing: - Resolve each explicitly provided flag to its parser destination instead of recording its raw spelling. argparse accepts abbreviations and aliases (e.g. --tp for --tp-size); recording the raw text made the engine treat the real field as unspecified and fall back to defaults. - Restore the num_gpus = tp_size * dp_size derivation when --num-gpus is not explicitly set. The engine defaults num_gpus to 1 and does not derive it from parallelism degrees, so tp/dp > 1 without --num-gpus under-allocated. - Remove the LLM-only --skip-tokenizer-init flag from both checked-in diffusion launch scripts (previously silently ignored, now rejected), and use the canonical --tp-size in the video launcher. Also route --help through the diffusion parser when a diffusion worker flag is present: the Dynamo parser's help printed LLM engine options, which a diffusion worker now rejects; help and parser must agree. Verified: 13/13 unit tests; both launchers' argument vectors parse against real sglang 0.5.17 (old vectors rejected naming the offending flag); both launch scripts executed end to end against sglang 0.5.17 serve generation requests (image via /v1/images/generations, video via /v1/videos). Signed-off-by: Marc Karp <mkarp@nvidia.com> Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
|
👋 Hi marckarp! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
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.
Supersedes #13376
#13376 was auto-closed when its base branch (#13343's) was deleted on merge, and GitHub does not allow reopening it. This PR is the same change rebased onto
main(which now contains #13343), plus fixes for all three review comments from #13376:--skip-tokenizer-initremoved from both checked-in diffusion launch scripts, and the video launcher now uses the canonical--tp-size; the repo was swept for other callers passing LLM-only flags to diffusion workers (these two scripts were the only ones)--tp 2correctly lands astp_size=2), unit-testednum_gpusderivation removed — restorednum_gpus = tp_size * dp_sizewhen--num-gpusis not explicitly set; an explicit--num-gpuswinsAdditionally,
--helpon a diffusion worker now prints the diffusion engine's options (plus Dynamo's) instead of the LLM engine options a diffusion worker would reject.Both launch scripts were executed end to end against sglang 0.5.17: the image launcher serves
/v1/images/generationsand the video launcher serves/v1/videos(HTTP 200 with generated content in both cases). As a bonus demonstration of the strict parser: an invocation typo (--modelinstead of--model-path) during validation was rejected withambiguous option: --model could match --model-path, --model-subfolder, --model-variant, --model-id— previously it would have been silently ignored and the default model loaded.Overview
Design direction (build on the native diffusion parser, on top of the 0.5.17 bump, now merged via #13343) was agreed in team review.
Image/video diffusion workers currently parse CLI args with the LLM ServerArgs parser, hand-copy 13 fields into a
SimpleNamespacestub, and callDiffGenerator.from_pretrained()with 5 hardcoded kwargs. Consequences:--enable-torch-compilecollides with the same-named LLM flag: it parses into the LLM ServerArgs and never reaches the diffusion config; passing it a value crashes the worker.--trust-remote-codenever reached the engine, anddist_timeoutwas forced toNone, disabling the engine's default distributed-init timeout.What this PR does
Build the worker CLI from sglang's native diffusion ServerArgs, mirroring how the LLM path builds its CLI from the LLM ServerArgs:
diffusion_args.py(new) — native parser +DiffusionWorkerArgsadapter. Engine fields delegate to the natively parsed diffusion ServerArgs; Dynamo-side settings (--served-model-name,--enable-metrics) are registered only when the engine parser does not define them natively, so when sglang ships its own (already on sglang main) it wins automatically. Fields Dynamo's shared worker code probes on anyserver_argsare pinned to inert values. Explicitly-provided flags are passed through the parser's_sglang_explicit_arg_namesside channel so resolution never depends on process argv (keeps the module testable and embeddable). Version-tolerant import for the server_args module/package layout change.args.py— diffusion/video workers branch to the native parser before any LLM-specific processing; the stub is deleted.init_diffusion.py— both image and video inits hand the full parsed config toDiffGenerator.from_server_args(), replacing the 5 hardcoded kwargs.sys.modules; no GPU, no model): parsing, adapter delegation, dynamo-side flag splitting, abbreviation resolution, num_gpus derivation, help routing, loud rejection.Breaking change / migration
LLM-only flags on diffusion worker command lines — previously silently ignored — now fail at startup with
unrecognized arguments. Example: a deployment passing--skip-tokenizer-initto an--image-diffusion-workermust drop that flag. This is the intended contract (same as the LLM path): loud rejection is what makes the newly reachable flags trustworthy. If reviewers prefer a softer landing (warn-and-ignore for one release), that's a small follow-up.Verification
lmsysorg/sglang:v0.5.17-cu130-runtime): 126 engine fields reachable (previously 5); invalid values rejected with actionable messages (--performance-mode: invalid choice: ... choose from 'manual', 'auto', 'speed', 'memory'); unknown flags rejected loudly; Dynamo-side flags kept out of the engine config.image_diffusion.shserves/v1/images/generationsandtext-to-video-diffusion.shserves/v1/videos(HTTP 200, generated content verified in both).--warmup-mode server— previously unreachable — confirmed active in the resolved engine config and engine logs;/v1/images/generationsreturns HTTP 200; warm request 0.97 s at 1024².--video-generation-worker; full pipeline runs and/v1/videosreturns a playable file.trust_remote_code,dist_timeout), and a per-boot random port.image_diffusion.shlaunch-script run that chore(sglang): bump to 0.5.17 #13343's validation had to skip for lack of model access.Notes for reviewers
dllm_algorithm=False, which madediffusion_worker = server_args.dllm_algorithm is not NoneaccidentallyTruefor image/video workers; it was never load-bearing becausemain.pydispatches image/video workers first. The new path sets itFalseexplicitly (dllm = text-diffusion LLMs, unrelated to image/video generation).num_gpusderivation (tp_size * dp_size) moves from Dynamo code into sglang's own ServerArgs resolution, and--num-gpusis now directly settable like any other native flag.nparameter in the SGLang image diffusion handler #12970 and fix: surface image generation errors instead of returning empty HTTP 200 #13000.Part of DYN-3974
🤖 Generated with Claude Code