Skip to content

Fix SGLang sidecar launch and endpoint integration - #362

Merged
ishandhanani merged 4 commits into
NVIDIA:mainfrom
weireweire:fix/sidecar-dump-config-argument
Sep 3, 2026
Merged

ishandhanani merged 4 commits into
NVIDIA:mainfrom
weireweire:fix/sidecar-dump-config-argument

Conversation

@weireweire

@weireweire weireweire commented Aug 28, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • do not pass Dynamo-only --dump-config-to to native sglang.launch_server in sidecar mode
  • derive the sidecar startup deadline from health_check.max_attempts * health_check.interval_seconds
  • pass the same deadline to Slurm and direct sidecar launch paths
  • use native SGLang HTTP ports for sidecar metrics and control endpoints
  • add regression coverage for the launchers and disaggregated endpoint selection

Why

Sidecar mode introduced in #354 launches native SGLang and connects it to Dynamo through the standalone sidecar. Three integration details prevented existing workloads from completing cleanly:

  1. The worker launcher appended --dump-config-to, which is accepted by dynamo.sglang but rejected by native sglang.launch_server.
  2. The sidecar used its built-in 300-second gRPC readiness deadline even when srt-slurm was configured to wait longer for a large model to initialize.
  3. Logical worker endpoints still used Dynamo system ports. In sidecar mode, native SGLang exposes its metrics and control APIs on the leader HTTP ports, so benchmark metric collection and lifecycle controls were pointed at the wrong service.

The sidecar now receives --health-deadline-secs from the same health-check budget used by srt-slurm and exits early as soon as native gRPC becomes ready. In-process Dynamo keeps using system ports, while SGLang sidecars use their native leader HTTP ports.

Validation

  • 230 configuration, direct-plan, and direct-runner tests passed for the launcher changes
  • 13 custom benchmark endpoint tests passed, including disaggregated SGLang sidecar coverage
  • changed source passed Ruff checks
  • diffs passed whitespace validation

Root cause: sidecar mode launches native sglang.launch_server, but the worker command still appended Dynamo's --dump-config-to argument. Native SGLang rejects that argument during startup.\n\nFix: gate the dump argument on the existing native-SGLang selection so only dynamo.sglang receives it. Add a sidecar command regression assertion.\n\nValidation: 8 sidecar-focused tests passed; Ruff check and format passed for the changed files.

Signed-off-by: weireweire <20922698+weireweire@users.noreply.github.com>
Root cause: the Dynamo SGLang sidecar defaults its native gRPC readiness deadline to 300 seconds, while srt-slurm permits much longer model startup through health_check. Large models can still be loading when the sidecar exits and tears down the engine.

Fix: derive one timeout from health_check.max_attempts and health_check.interval_seconds, pass it to the sidecar as --health-deadline-secs in both Slurm and direct launch paths, and reuse the same value for direct health polling.

Validation: 230 configuration, direct-plan, and direct-runner tests passed; Ruff source checks and format checks passed.
Signed-off-by: weireweire <20922698+weireweire@users.noreply.github.com>
Root cause: SGLang sidecar mode was still classified as an in-process Dynamo worker when selecting logical metrics and control endpoints. This advertised Dynamo system ports, which do not expose the native SGLang metrics or control APIs.

Fix: keep system ports for in-process Dynamo, but select each leader's native SGLang HTTP port for sidecar mode. Add coverage for disaggregated prefill/decode endpoint and metrics URL generation.

Validation: uv run pytest tests/test_benchmarks.py::TestCustomBenchmarkRunner -q
@weireweire weireweire changed the title Fix config dump argument for SGLang sidecars Fix SGLang sidecar launch and endpoint integration Sep 1, 2026
@codecov-commenter

codecov-commenter commented Sep 2, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (main@e32fa22). Learn more about missing BASE report.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #362   +/-   ##
=======================================
  Coverage        ?   72.64%           
=======================================
  Files           ?       99           
  Lines           ?    13656           
  Branches        ?        0           
=======================================
  Hits            ?     9921           
  Misses          ?     3735           
  Partials        ?        0           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Main's sidecar rearchitecture (NVIDIA#363) superseded this branch's original fixes
(mixin-level _wrap_sglang_sidecar/uses_sidecar/engine_mode), so most of this
merge drops that code in favor of main's backends/sidecar.py + sglang.py
implementation.

Ported forward the one still-relevant fix: _logical_worker_endpoints was
still advertising Dynamo system ports for sidecar-mode workers instead of
native SGLang HTTP ports, which don't expose Dynamo's metrics/control APIs.
Adapted to main's config.dynamo.sidecar field, with regression test.
@ishandhanani
ishandhanani force-pushed the fix/sidecar-dump-config-argument branch from ca78d25 to c70cd34 Compare September 2, 2026 19:00
@ishandhanani
ishandhanani merged commit ba37b7c into NVIDIA:main Sep 3, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants