Skip to content

feat: parse diffusion worker args natively from sglang's diffusion ServerArgs - #13376

Closed
marckarp wants to merge 6 commits into
ai-dynamo:jain-ria/sgl-to-0.5.17-and-cleanupsfrom
marckarp:feat/sglang-diffusion-native-args
Closed

feat: parse diffusion worker args natively from sglang's diffusion ServerArgs#13376
marckarp wants to merge 6 commits into
ai-dynamo:jain-ria/sgl-to-0.5.17-and-cleanupsfrom
marckarp:feat/sglang-diffusion-native-args

Conversation

@marckarp

Copy link
Copy Markdown
Contributor

Overview

Stacked on #13343 — do not merge first. This PR's base is that branch; GitHub will retarget to main when it merges. Design direction (build on the native diffusion parser, on top of the 0.5.17 bump) was agreed in team review.

Image/video diffusion workers currently parse CLI args with the LLM ServerArgs parser, hand-copy 13 fields into a SimpleNamespace stub, and call DiffGenerator.from_pretrained() with 5 hardcoded kwargs. Consequences:

  • Nearly every diffusion engine option (torch compile, quantization, batching, warmup, attention backend, …) is silently stuck at its default.
  • Unknown or LLM-only flags are silently absorbed — a typo'd flag and a working flag are indistinguishable.
  • --enable-torch-compile collides 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.
  • The stub itself drops flags it claims to carry: --trust-remote-code never reached the engine, and dist_timeout was forced to None, 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 + DiffusionWorkerArgs adapter. 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 any server_args are pinned to inert values. Explicitly-provided flags are passed through the parser's _sglang_explicit_arg_names side 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 to DiffGenerator.from_server_args(), replacing the 5 hardcoded kwargs.
  • Tests — 9 hermetic unit tests (fake engine parser injected via sys.modules; no GPU, no model).

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-init to an --image-diffusion-worker must 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

  • Parser against real sglang 0.5.17 (upstream 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.
  • Unit tests: 9/9.
  • Live image e2e (FLUX.1-schnell, 1×B200): worker boots through the native path; --warmup-mode server — previously unreachable — confirmed active in the resolved engine config and engine logs; /v1/images/generations returns HTTP 200; warm request 0.97 s at 1024².
  • Live video e2e (Wan2.1-T2V-1.3B, 1×B200): worker boots through the native path with --video-generation-worker; full pipeline runs and /v1/videos returns a playable file.
  • Resolved-config diff, stub vs native (107 fields): zero unintended changes. The only diffs were the explicitly passed warmup flags, the two stub bugs now fixed (trust_remote_code, dist_timeout), and a per-boot random port.
  • Assembled check: sglang v0.5.17 image + current dynamo wheel + this branch's files boots a frontend+worker pair and serves image generation end-to-end (HTTP 200). The official container build of this combination lands via CI — which also covers the image_diffusion.sh launch-script run that chore(sglang): bump to 0.5.17 #13343's validation had to skip for lack of model access.

Notes for reviewers

Part of DYN-3974

🤖 Generated with Claude Code

Signed-off-by: jain-ria <riajain@NVIDIA.com>
Signed-off-by: jain-ria <riajain@NVIDIA.com>
Signed-off-by: jain-ria <riajain@NVIDIA.com>
Signed-off-by: jain-ria <riajain@NVIDIA.com>
@marckarp
marckarp temporarily deployed to external_collaborator August 17, 2026 18:07 — with GitHub Actions Inactive
@marckarp
marckarp had a problem deploying to external_collaborator August 17, 2026 18:07 — with GitHub Actions Failure
@marckarp marckarp closed this Aug 17, 2026
@marckarp marckarp reopened this Aug 17, 2026
@marckarp
marckarp temporarily deployed to external_collaborator August 17, 2026 18:14 — with GitHub Actions Inactive
@github-actions github-actions Bot added the feat label Aug 17, 2026
@copy-pr-bot

copy-pr-bot Bot commented Aug 17, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@marckarp marckarp closed this Aug 17, 2026
@marckarp marckarp reopened this Aug 17, 2026
@marckarp
marckarp temporarily deployed to external_collaborator August 17, 2026 18:39 — with GitHub Actions Inactive
@github-actions github-actions Bot added documentation Improvements or additions to documentation backend::sglang Relates to the sglang backend container labels Aug 17, 2026
Signed-off-by: jain-ria <riajain@NVIDIA.com>
@jain-ria
jain-ria force-pushed the jain-ria/sgl-to-0.5.17-and-cleanups branch from 6a32da6 to 08c85e6 Compare August 17, 2026 18:48
…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>
@marckarp
marckarp force-pushed the feat/sglang-diffusion-native-args branch from 327995a to 010ba75 Compare August 17, 2026 18:53
@marckarp
marckarp temporarily deployed to external_collaborator August 17, 2026 18:54 — with GitHub Actions Inactive
@jain-ria
jain-ria force-pushed the jain-ria/sgl-to-0.5.17-and-cleanups branch from 08c85e6 to 8c6f5b6 Compare August 17, 2026 20:22
@jain-ria
jain-ria deleted the branch ai-dynamo:jain-ria/sgl-to-0.5.17-and-cleanups August 21, 2026 00:05
@jain-ria jain-ria closed this Aug 21, 2026
explicit_names |= set(parsed._sglang_explicit_arg_names)
parsed._sglang_explicit_arg_names = tuple(sorted(explicit_names))

engine_args = DiffusionServerArgs.from_cli_args(parsed, remaining)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correctly rejects unsupported arguments, but both checked-in diffusion launchers still pass in the LLM-only --skip-tokenizer-init flag (image, video). Can we update + double check both scripts?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in #13667: --skip-tokenizer-init removed from both launch scripts, and the video launcher now uses the canonical --tp-size. Also swept the repo for other callers passing LLM-only flags to diffusion workers (docs, tests, configs) — these two scripts were the only ones. Both scripts were executed end to end against sglang 0.5.17 and serve generation requests; the old argument vector is rejected with unrecognized arguments: --skip-tokenizer-init.

# caller that passes an argument list (tests, embedding). Communicate the
# exact flags we parsed through the side channel the engine parser
# supports, so resolution never depends on process argv.
explicit_names = {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parse_known_args() can accept an abbreviated or aliased option and store it under a different name, but this records the raw spelling, so then SGLang then may not recognize the corresponding field as explicitly provided. For example, the existing video launcher’ passes --tp 2, accepting it as an abbreviation for --tp-size, but this code records the raw name tp. Because SGLang does not map tp back to tp_size, it treats tp_size as unspecified and falls back to 1.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch — fixed in #13667. The side channel now resolves each provided flag to its parser destination (exact match or unique prefix, mirroring argparse's own abbreviation rules) instead of recording the raw spelling. Unit-tested: --tp 2 lands as tp_size=2 in both the parsed config and the explicit-args set. Flags not registered on the parser (e.g. dynamic --<component>-path) still fall back to the normalized raw name so the engine's own resolution handles them.

# server_args is a DiffusionWorkerArgs adapter; engine_args is the
# natively parsed sglang diffusion ServerArgs, handed over wholesale so
# every engine option the user set on the CLI takes effect.
generator = DiffGenerator.from_server_args(server_args.engine_args)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This update removed the previous num_gpus = tp_size * dp_size calculation, defaulting to 1. Any call that runs with TP or DP > 1 without also passing --num-gpus will have an inconsistent configuration.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in #13667 — the engine defaults num_gpus to 1 and does not derive it from parallelism degrees, so this was a real regression. Restored num_gpus = tp_size * dp_size when --num-gpus is not explicitly provided (an explicit --num-gpus wins). Covered by two unit tests.

@marckarp

Copy link
Copy Markdown
Contributor Author

GitHub does not allow reopening this PR after the base-branch deletion, so it continues in #13667 — the same change rebased onto main, with all three review comments addressed there (each has a threaded reply above with the fix and test evidence). Thanks @jain-ria — all three were real issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend::sglang Relates to the sglang backend container documentation Improvements or additions to documentation feat size/XL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants