fix(inkling): return 400 instead of 500 for unfetchable/undecodable multimodal input by hanahhh · Pull Request #40899 · sgl-project/sglang

hanahhh · 2026-09-23T10:45:06Z

Motivation

InklingMultimodalProcessor overrides process_mm_data_async() with its own media-resolution step (_resolve_media_item()) and its own image-decode step (_encode_image_bytes()). Neither goes through BaseMultimodalProcessor._load_single_item() / common.py::load_image(), so the CLIENT_MEDIA_EXCEPTIONS classification added there by #31417 (which every other VLM processor inherits) never applied to Inkling. As a result, an empty/unreachable image_url or an undecodable image raised an unclassified exception that the generic handler in serving_base.py mapped to HTTP 500 instead of HTTP 400.

Modifications

python/sglang/srt/multimodal/processors/inkling.py: _resolve_media_item() now rejects an empty image_url at the boundary, and wraps the data:/http(s):// branches in try/except CLIENT_MEDIA_EXCEPTIONS: raise ValueError(...) so requests.exceptions.RequestException from download_remote_media() is reclassified as a client error instead of propagating raw.
python/sglang/srt/multimodal/inkling/image_processing.py: _encode_image_bytes() wraps Image.open(...) in try/except OSError as e: raise ValueError(...), mirroring common.py::_load_image().
test/registered/unit/multimodal/test_inkling_media_errors.py (new): 11 cases covering both functions -- empty/unreachable image_url, invalid base64, non-URL/plain-path passthrough, undecodable/truncated image bytes, and a valid-image regression guard.

Accuracy Tests

N/A -- this only changes error handling for invalid client input; it does not touch model forward code or any path exercised by valid input.

Speed Tests and Profiling

N/A -- no hot-path/model-forward code touched.

Checklist

Format your code according to the Format code with pre-commit.
Add unit tests according to the Run and add unit tests.
Update documentation according to Write documentations. (N/A -- no doc under docs/ describes Inkling's multimodal error-handling contract)
Provide accuracy and speed benchmark results according to Test the accuracy and Benchmark the speed. (N/A, see above)
Follow the SGLang code style guidance.

CI States

Latest PR Test (Base): ❌ Run #37560765663
Latest PR Test (Extra): ❌ Run #37560765617
Latest PR Test (AMD ROCm 10): ❌ Run #37560765989

apex-mochen · 2026-09-28T11:10:09Z

Thanks for covering the whitespace, Rust decode, shared image loader, and audio paths. The remaining blocker is CI maintenance: #21065 currently requires MIN_BASE_SHA 3fc7a66, and this branch is diverged from it, so check-maintenance fails before base-a-test-cpu can run. Could you please rebase onto current main (including that base) and push? Once CI runs, the current direct regression coverage should exercise the intended paths.

apex-mochen · 2026-09-28T16:31:16Z

@hanahhh I prepared the reviewer-requested fix that narrows the two filesystem OSError handlers to expected bad-path exceptions and adds coverage ensuring unexpected server-side errors (for example EMFILE) still propagate. The patch is available as commit efc607d on https://github.com/apex-mochen/sglang/tree/codex/review-40899-narrow-oserror. Please cherry-pick it, then rebase the branch onto current main before pushing so the maintenance gate can run the CPU tests. I will follow up on the two review threads once the update is present on this PR.

…ultimodal input InklingMultimodalProcessor overrides process_mm_data_async() with its own media fetch (_resolve_media_item) and decode (_encode_image_bytes) steps, bypassing the CLIENT_MEDIA_EXCEPTIONS classification that sgl-project#31417 added to BaseMultimodalProcessor/common.py for every other VLM processor. As a result, an empty/unreachable image_url or an undecodable image raised an unclassified exception that the generic handler in serving_base.py mapped to HTTP 500 instead of 400. Apply the same classify-and-reraise-as-ValueError pattern used elsewhere: - reject an empty image_url at the boundary - reclassify requests.exceptions.RequestException from download_remote_media - reclassify OSError/UnidentifiedImageError from PIL's Image.open() Fixes sgl-project#40897

…ule docstring Minor cleanup, no behavior change.

…ication Address review feedback and audit findings on top of d170b24: - _resolve_media_item(): reject whitespace-only image_url, not just empty string (review feedback). - image_processing_rust.py::_pil_decode(): apply the same try/except OSError -> ValueError classification as _encode_image_bytes(). This is the actual default image decode path (SGLANG_INKLING_RS_MM_PREPROCESS defaults to True), so the earlier fix only covered the plain-Python fallback (review feedback). - image_processing.py::_load_image_bytes(): classify OSError from open() on a bad local path. Shared by both image decode paths and runs before either one, so a garbage image_url that isn't empty/data:/http(s):// was still reaching an unguarded open() and raising FileNotFoundError. - feature_extraction.py::_load_audio_bytes() / _decode_audio(): same class of gap, audio modality. _decode_audio wraps sf.read() to reclassify soundfile.LibsndfileError (a RuntimeError subclass), matching the pattern common.py::load_audio() already uses for the generic path. Adds regression tests for all of the above in test_inkling_media_errors.py.

hanahhh · 2026-09-29T08:21:48Z

@apex-mochen Thanks for the contribution. I reviewed and cherry-picked your commit as f7a8aca (attribution preserved). Branch is also rebased onto current main now

nvpohanh

[by Claude Code] Reviewed at f7a8aca. Inline comments below: 3 functional, 1 style, 1 nit.

- _pil_decode() / _encode_image_bytes(): catch SyntaxError alongside OSError. PIL raises SyntaxError (not OSError) for some structurally valid but corrupt images (e.g. a truncated IDAT chunk), which were still reaching the generic 500 handler. - _load_image_bytes() / _load_audio_bytes(): switch the narrowed except clause from an exception-type tuple to an errno check against {ENOENT, ENOTDIR, EISDIR, ENAMETOOLONG}. The previous tuple missed ENAMETOOLONG, so a raw base64 payload sent without a "data:" prefix (treated as an overlong "path") still returned 500. Also truncate the path in the error message, since it can hold an entire payload. - _resolve_media_item(): truncate the url embedded in the error message to 100 chars, matching the convention in BaseMultimodalProcessor._load_single_item(). A truncated multi-MB "data:" URL was otherwise echoed in full, both in logs and in the 400 response body. Also generalize the empty-URL message from "empty image_url" to "empty media URL", since this function resolves both image_data and audio_data. - test_inkling_media_errors.py: consolidated redundant docstrings and per-input __cause__ checks into the main assertions, added shared _png_bytes()/_broken_chunk_png_bytes() helpers, and added regression coverage for all of the above (SyntaxError, ENAMETOOLONG, message truncation, generalized empty-URL message). Addresses nvpohanh's review on PR sgl-project#40899.

nvpohanh · 2026-10-06T05:29:10Z

/rerun-test test/registered/unit/multimodal/test_inkling_media_errors.py test/registered/e2e/models/test_inkling.py

github-actions · 2026-10-06T05:29:36Z

⚠️ Rebase Required Before Re-run

A major update has landed on main. Your PR is diverged relative to required base commit fea3c088a141.

Re-run was not dispatched. What to do:

Rebase your branch onto the latest main and push again
Follow issue #21065 for context
CI-fix PRs may request the bypass-maintenance label to skip this check

nvpohanh · 2026-10-06T05:55:39Z

/rerun-test test/registered/unit/multimodal/test_inkling_media_errors.py test/registered/e2e/models/test_inkling.py

github-actions · 2026-10-06T05:56:05Z

🚀 ubuntu-latest (1 test): ✅ View workflow run

cd test/ && python3 registered/unit/multimodal/test_inkling_media_errors.py

🚀 1-gpu-h100 (1 test): ✅ View workflow run

cd test/ && python3 registered/e2e/models/test_inkling.py

apex-mochen · 2026-10-06T06:25:17Z

Hi @hanahhh, since my contribution in f7a8aca may be lost as a separate commit if this PR is squash-merged, could you please preserve it in the final commit with:

Co-authored-by: apex-mochen 2756823972@qq.com
Thanks!

apex-mochen · 2026-10-06T06:31:54Z

Hi @hanahhh, since my contribution in f7a8aca is an authored production-code commit and this PR may be squash-merged, could you please preserve my attribution in the final commit with:

Co-authored-by: apex-mochen 2756823972@qq.com
Thanks ！

Preserve attribution for apex-mochen's contribution in f7a8aca (fix(inkling): preserve server file descriptor errors) when this PR is squash-merged. Co-authored-by: apex-mochen <2756823972@qq.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

nvpohanh · 2026-10-07T02:11:35Z

[by Claude Code]

@apex-mochen Thanks for the contribution. I merged the latest main into this branch as 2b1adfe. That merge commit's message ends with the trailer:

Co-authored-by: apex-mochen <2756823972@qq.com>

This repository squash-merges PRs, and GitHub carries Co-authored-by trailers from a PR's commits, including merge commits, into the squash commit. Your trailer should therefore be in the final commit. Your original commit f7a8aca also still lists you as its author. @hanahhh, please don't drop the trailer if you edit the squash message at merge time.

nvpohanh · 2026-10-07T02:11:36Z

/rerun-test test/registered/unit/multimodal/test_inkling_media_errors.py test/registered/e2e/models/test_inkling.py
[by Claude Code]

github-actions · 2026-10-07T02:12:02Z

🚀 ubuntu-latest (1 test): ✅ View workflow run

cd test/ && python3 registered/unit/multimodal/test_inkling_media_errors.py

🚀 1-gpu-h100 (1 test): ✅ View workflow run

cd test/ && python3 registered/e2e/models/test_inkling.py

YouNeedCryDear · 2026-10-07T23:12:47Z

@TheDuyIT Could you take a look at the PR again and remove the "request change" if no other issues?

nvpohanh · 2026-10-08T07:03:44Z

@mickqian @yhyang201 Could you help to review this? Thanks!

hanahhh requested review from HaiShaw, JustinTong0323, mickqian, yctseng0211, yhyang201 and yuan-luo as code owners September 23, 2026 10:45

hanahhh marked this pull request as draft September 23, 2026 10:50

hanahhh marked this pull request as ready for review September 24, 2026 02:06

apex-mochen mentioned this pull request Sep 24, 2026

[Bugfix] Classify Inkling media errors as client errors (HTTP 400) instead of server faults (500) #40931

Closed

3 of 5 tasks

TheDuyIT suggested changes Sep 28, 2026

View reviewed changes

Comment thread python/sglang/srt/multimodal/processors/inkling.py Outdated

Comment thread python/sglang/srt/multimodal/inkling/image_processing.py

TheDuyIT suggested changes Sep 28, 2026

View reviewed changes

Comment thread python/sglang/srt/multimodal/inkling/image_processing.py

Comment thread python/sglang/srt/multimodal/inkling/feature_extraction.py

hanahhh and others added 4 commits September 29, 2026 07:25

test(inkling): drop redundant "no server/model loading" note from mod…

0313bf9

…ule docstring Minor cleanup, no behavior change.

fix(inkling): preserve server file descriptor errors

f7a8aca

hanahhh force-pushed the fix/inkling-multimodal-400-not-500 branch from 1402178 to f7a8aca Compare September 29, 2026 07:38

YouNeedCryDear approved these changes Oct 5, 2026

View reviewed changes

nvpohanh reviewed Oct 6, 2026

View reviewed changes

nvpohanh approved these changes Oct 6, 2026

View reviewed changes

Merge branch 'main' into fix/inkling-multimodal-400-not-500

bba926c

TheDuyIT approved these changes Oct 8, 2026

View reviewed changes

Conversation

hanahhh commented Sep 23, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Accuracy Tests

Speed Tests and Profiling

Checklist

CI States

Uh oh!

Uh oh!

Uh oh!

apex-mochen commented Sep 28, 2026

Uh oh!

Uh oh!

Uh oh!

apex-mochen commented Sep 28, 2026

Uh oh!

hanahhh commented Sep 29, 2026

Uh oh!

nvpohanh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nvpohanh commented Oct 6, 2026

Uh oh!

github-actions Bot commented Oct 6, 2026

⚠️ Rebase Required Before Re-run

Uh oh!

nvpohanh commented Oct 6, 2026

Uh oh!

github-actions Bot commented Oct 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apex-mochen commented Oct 6, 2026

Uh oh!

apex-mochen commented Oct 6, 2026

Uh oh!

nvpohanh commented Oct 7, 2026

Uh oh!

nvpohanh commented Oct 7, 2026

Uh oh!

github-actions Bot commented Oct 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

YouNeedCryDear commented Oct 7, 2026

Uh oh!

nvpohanh commented Oct 8, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hanahhh commented Sep 23, 2026 •

edited by github-actions Bot

Loading

github-actions Bot commented Oct 6, 2026 •

edited

Loading

github-actions Bot commented Oct 7, 2026 •

edited

Loading