Skip to content

feat(sglang): define sidecar executable contract - #11797

Closed
connorcarpenter15 wants to merge 1 commit into
mainfrom
feat/sglang-sidecar-executable
Closed

connorcarpenter15 wants to merge 1 commit into
mainfrom
feat/sglang-sidecar-executable

Conversation

@connorcarpenter15

@connorcarpenter15 connorcarpenter15 commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Make dynamo-sglang-sidecar parse its process arguments explicitly before constructing SglangSidecarEngine.
  • Add SglangSidecarEngine::from_parsed_args so the executable has a clear parsed-CLI boundary.
  • Add unit and executable-level coverage for the SGLang-managed --sglang-endpoint contract.
  • Document the expected SGLang supervision, failure-propagation, and graceful-shutdown behavior.
  • Ensure changes under lib/sglang-sidecar trigger SGLang CI.

Motivation

SGLang should be able to supervise Dynamo as a real child executable rather than importing an in-process Python module. The executable remains a thin composition of the native SGLang adapter and dynamo_backend_common::run, while SGLang owns process startup and shutdown.

python3 -m sglang.launch_server \
    <args> \
    --grpc-port 30001 \
    --sidecar-executable dynamo-sglang-sidecar

Packaging or bundling the executable is intentionally deferred.

Scope

This extracts the Rust executable portion of #11636. It does not include the Python entry-point changes or move lib/sglang-sidecar into lib/backend; those are separate changes.

Validation

  • cargo fmt --all -- --check
  • cargo metadata --no-deps --format-version 1
  • git diff --check

The focused local Cargo test progressed through the sidecar crate and its dependencies but could not finish because the workspace filesystem ran out of space; draft PR CI will provide the full build and test result.

Summary by CodeRabbit

  • New Features

    • The SGLang sidecar now supports launching with the --sglang-endpoint command-line option.
    • Improved command-line help describes endpoint configuration and SGLang-managed operation.
  • Documentation

    • Added usage instructions covering build, startup, supervision, timeouts, and endpoint discovery.
    • Clarified configuration for prefilling deployments.
  • Tests

    • Added coverage verifying endpoint argument parsing and executable help output.
  • Chores

    • Updated CI routing so sidecar changes trigger the appropriate checks.

Signed-off-by: Connor Carpenter <connorc@nvidia.com>
@github-actions github-actions Bot added feat documentation Improvements or additions to documentation actions labels Jul 16, 2026
@github-actions

Copy link
Copy Markdown
Contributor

@datadog-official

datadog-official Bot commented Jul 16, 2026

Copy link
Copy Markdown

🎯 Code Coverage (details)
Patch Coverage: 100.00%
Overall Coverage: 41.10% (-7.57%)

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 386dc5c | Docs | Give us feedback!

@connorcarpenter15
connorcarpenter15 marked this pull request as ready for review July 16, 2026 21:36
@connorcarpenter15
connorcarpenter15 requested review from a team as code owners July 16, 2026 21:36

@devin-ai-integration devin-ai-integration Bot left a comment

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.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Open in Devin Review

@coderabbitai

coderabbitai Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

The SGLang sidecar now parses its endpoint from CLI arguments, constructs the engine from parsed arguments, documents launcher supervision requirements, exposes updated help text, validates the executable contract, and routes sidecar changes to SGLang CI jobs.

Changes

SGLang sidecar CLI integration

Layer / File(s) Summary
CLI contract and sidecar documentation
lib/sglang-sidecar/README.md, lib/sglang-sidecar/src/args.rs, lib/sglang-sidecar/Cargo.toml
The sidecar’s description, endpoint parsing test, CLI wording, build instructions, and SGLang-managed launcher contract are updated.
Parsed-argument engine bootstrap
lib/sglang-sidecar/src/engine.rs, lib/sglang-sidecar/src/main.rs
The engine adds from_parsed_args, while the binary parses Args and passes them into engine construction.
Executable validation and CI routing
lib/sglang-sidecar/tests/executable.rs, .github/filters.yaml
Executable help output is checked for the endpoint contract, and sidecar changes are included in SGLang CI filtering.

Estimated code review effort: 2 (Simple) | ~15 minutes

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description covers the change, but it does not follow the required template and omits the required Related Issues and reviewer-start sections. Rewrite it with the repository template: Overview, Details, Where should reviewer start?, and Related Issues with a linked issue or the no-issue checkbox.
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and accurately summarizes the main change: defining the SGLang sidecar executable contract.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

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.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@lib/sglang-sidecar/src/main.rs`:
- Around line 6-9: In the module documentation comment, rephrase the sentence
around from_parsed_args so it clearly states that it builds the WorkerConfig
that run needs synchronously, preserving the surrounding bootstrap and
runtime-harness description.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: fa9183e0-95a8-4771-97c4-462f578755f0

📥 Commits

Reviewing files that changed from the base of the PR and between 01d2cb2 and 386dc5c.

📒 Files selected for processing (7)
  • .github/filters.yaml
  • lib/sglang-sidecar/Cargo.toml
  • lib/sglang-sidecar/README.md
  • lib/sglang-sidecar/src/args.rs
  • lib/sglang-sidecar/src/engine.rs
  • lib/sglang-sidecar/src/main.rs
  • lib/sglang-sidecar/tests/executable.rs

Comment thread lib/sglang-sidecar/src/main.rs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

actions documentation Improvements or additions to documentation feat size/L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant