Skip to content

fix(sidecar): set the SGLang media fields once - #17980

Merged
dagil-nvidia merged 1 commit into
mainfrom
jdarve/sglang-duplicate-media-fields
Nov 3, 2026
Merged

dagil-nvidia merged 1 commit into
mainfrom
jdarve/sglang-duplicate-media-fields

Conversation

@JulienDarve

@JulienDarve JulienDarve commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Summary

dynamo-sglang-sidecar does not compile on main, because GenerateRequest sets image_data, video_data and audio_data twice in lib/sidecar/sglang/src/protocol.rs.

The two edits sit on different lines, so git merged them without a conflict, but Rust rejects a struct literal that names a field twice. This PR removes #17653's three empty placeholders and keeps #16203's forwarded media.

Validation

Related Issues

Generated with Claude Code

#17653 (SGLang 0.5.23) set GenerateRequest's image_data, video_data and
audio_data to empty, and #16203 set the same fields to the forwarded media.
Both merged into main, so the struct named each field twice and the SGLang
sidecar stopped compiling. Keep the forwarded media.

Signed-off-by: Julien Darve <jdarve@NVIDIA.com>
@JulienDarve
JulienDarve requested review from a team as code owners November 3, 2026 21:52
@coderabbitai

coderabbitai Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Walkthrough

build_generate_request now populates image, video, and audio data from the processed request. Media extraction rejects unsupported types and sources, and rejects media I/O options when media is present.

Changes

SGLang media request construction

Layer / File(s) Summary
Populate generation request media
lib/sidecar/sglang/src/protocol.rs
build_generate_request passes collected image, video, and audio data into the request. Extraction rejects unsupported media types and sources, and media I/O options when media is present.

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

Merge Risk: Minimal · up to 28cbd

No specific issue blocking merge is established by the supplied evidence; complete the normal build and test checks before merging.

Pre-merge checks: Passed 5
Check name Status Explanation
Title check Passed The title "fix(sidecar): set the SGLang media fields once" names the main change. The PR removes the duplicate empty image_data, video_data, and audio_data placeholders from GenerateRequest in lib/sid…
Description check Passed The description covers the overview (a compile break on main caused by duplicate struct field assignments from two merged PRs), the key change (removing the three empty placeholders and keeping the fo…
Docstring Coverage Passed No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
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.
Full details: Title check

Explanation

The title "fix(sidecar): set the SGLang media fields once" names the main change. The PR removes the duplicate empty image_data, video_data, and audio_data placeholders from GenerateRequest in lib/sidecar/sglang/src/protocol.rs, so the struct sets each media field once and the crate compiles. The title is specific, short, and in the repository's conventional-commit style.

Full details: Description check

Explanation

The description covers the overview (a compile break on main caused by duplicate struct field assignments from two merged PRs), the key change (removing the three empty placeholders and keeping the forwarded media), and validation (cargo check results and testkit runs). It also has a Related Issues section. It does not follow the template headings exactly: it uses Summary, Validation, and Related Issues rather than Overview, Key Changes, Validation, and Issues and Ongoing Work. It also lacks the Closes #XXXX issue link and the Pull Request Checklist item (I used Dynamo's /pr-checklist skill). The content is substantive, so the check passes, but the author should align the headings and add the missing checklist and issue-link items.

Full details: Docstring Coverage

Explanation

No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 0 files.

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

CodeRabbit Verify

  • Run CodeRabbit Verify to test the changes in this pull request.
Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between b0a6bd1 and 28cbd63.

Files selected for processing: 1
  • lib/sidecar/sglang/src/protocol.rs
Files with no reviewable changes: 1
  • lib/sidecar/sglang/src/protocol.rs

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.

@JulienDarve
JulienDarve enabled auto-merge (squash) November 3, 2026 21:54
@KrishnanPrash

Copy link
Copy Markdown
Contributor

Thank you for the fixes!

@dagil-nvidia
dagil-nvidia disabled auto-merge November 3, 2026 21:58
@dagil-nvidia
dagil-nvidia merged commit a0bf0fd into main Nov 3, 2026
40 checks passed
@dagil-nvidia
dagil-nvidia deleted the jdarve/sglang-duplicate-media-fields branch November 3, 2026 21:59
karen-sy added a commit that referenced this pull request Nov 3, 2026
Bring in main's fix for the SGLang sidecar build (#17980).

Signed-off-by: Karen Chung <karenc@nvidia.com>
@github-actions github-actions Bot added the fix label Nov 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants