Skip to content

fix(sidecar): remove duplicate SGLang media fields - #17981

Closed
karen-sy wants to merge 1 commit into
mainfrom
karenc/fix-sglang-sidecar-duplicate-media-fields
Closed

karen-sy wants to merge 1 commit into
mainfrom
karenc/fix-sglang-sidecar-duplicate-media-fields

Conversation

@karen-sy

@karen-sy karen-sy commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Overview

main doesn't compile: dynamo-sglang-sidecar fails with E0062 because image_data, video_data and audio_data are set twice when building the SGLang generate request. #17653 added them as empty placeholders and #16203 added them with the request's media inputs; each passed CI alone, and they landed two minutes apart. This keeps the forwarded inputs and removes the placeholders.

Key Changes

Validation

Case Before Now
Build the sidecar crates (dynamo-sglang-sidecar and the rest of the sidecar set) E0062: fields specified more than once Builds; clippy clean with -D warnings

Issues and Ongoing Work

Pull Request Checklist
  • I used Dynamo's /pr-checklist skill.

Summary by CodeRabbit

  • Bug Fixes
    • Generation requests now include validated image, video, and audio content in the order it was provided. Previously, these media fields were sent as empty lists, so attached media was not carried through. This improves handling of requests containing media, while requests without media continue to be supported.

@karen-sy
karen-sy requested review from a team as code owners November 3, 2026 21:52
#17653 and #16203 each set image_data, video_data and audio_data on the
SGLang pb::GenerateRequest, one as empty placeholders and one with the
request's inputs. They landed two minutes apart, and together the SGLang sidecar no
longer compiles (E0062). Keep the forwarded inputs from #16203.

Signed-off-by: Karen Chung <karenc@nvidia.com>
@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 sets image_data, video_data, and audio_data from media collected from the request instead of initializing these fields as empty lists.

Changes

Request media propagation

Layer / File(s) Summary
Populate generate request media fields
lib/sidecar/sglang/src/protocol.rs
GenerateRequest receives the image, video, and audio data collected from the request.

Estimated code review effort: 1 (Trivial) | ~3 minutes

Merge Risk: Minimal · up to a946b

No actionable issue remains from the supplied review evidence; the change is ready for normal build checks.

Pre-merge checks: Passed 5
Check name Status Explanation
Title check Passed The title names the main change: removing duplicate SGLang media fields in the sidecar. The raw summary says build_generate_request no longer initializes image_data, video_data, and audio_data to empt…
Description check Passed The description follows the repository template. It has Overview, Key Changes, Validation, and Issues and Ongoing Work sections, plus the checklist. It explains the E0062 build break, names the file c…
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 names the main change: removing duplicate SGLang media fields in the sidecar. The raw summary says build_generate_request no longer initializes image_data, video_data, and audio_data to empty lists, and the objectives describe the same fix. The title is specific, short, and matches the changeset.

Full details: Description check

Explanation

The description follows the repository template. It has Overview, Key Changes, Validation, and Issues and Ongoing Work sections, plus the checklist. It explains the E0062 build break, names the file changed, gives a validation table, and links the related issues #16203 and #17653. The 'Closes #XXXX' line from the template is replaced by 'Fixes the build break from #16203 and #17653', with no issue number. The checklist item is still unchecked. These gaps are minor.

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 a946b3e.

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.

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.

2 participants