Repository navigation
fix(servicer): never fail a served request on a media identity it cannot build - #2652
Conversation
…not build build_media_identity ran after the engine had answered and could raise on shapes it did not anticipate (a missing prompt_token_ids key, mixed grid dtypes, a non-tensor element), turning a served request into a client error. The identity is an optimisation with a fallback, decode reprocessing the media, so the build is now guarded: a failure logs one warning with the request id and the leg answers without it. Signed-off-by: Alex McC <319643551+hello-alexmcc@users.noreply.github.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: 2 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 6 reviews per hour. 📝 SummarySummary by CodeRabbit
WalkthroughGenerate now catches exceptions during prefill media identity construction, logs a warning, and continues without a media identity. A regression test checks that the request completes and the warning includes the failure details. ChangesPrefill media identity handling
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The reported fallback lets affected requests complete when media identity construction fails. No actionable merge-blocking risk is identified; normal checks remain appropriate. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
|
👋 The PR description doesn't fully follow
Please update the PR description so reviewers have the context they need. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| logger.warning( | ||
| "Request %s: media identity not built (%s); the decode leg " | ||
| "will reprocess the media", | ||
| request_id, | ||
| e, | ||
| ) |
There was a problem hiding this comment.
🟡 Nit: This warning logs only str(e), which drops the exception type and traceback. For a bare KeyError — one of the failure modes this PR anticipates (prompt["prompt_token_ids"]) — the log reads media identity not built ('prompt_token_ids'), with no hint of what went wrong or where. Since this catch exists precisely for unanticipated shapes, the traceback is the diagnostic you'll want. This file already does that for the analogous best-effort fallback at _notify_kv_rejected (line 502-507): keep the message as-is and add exc_info=True (the test's getMessage() assertions are unaffected).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the full diff. The fallback is well-scoped: except Exception doesn't swallow cancellation, build_media_identity never mutates the prompt it reads, and the new test verifies the request completes, the identity is absent, and the warning names the cause. One 🟡 nit inline about logging the traceback (exc_info=True) to match this file's existing convention for unexpected-failure fallbacks.
2 participants
Description
Problem
From the review of #2627: on a PD prefill leg the servicer calls
build_media_identity(prompt)afterself._mm_processor.process(...)has already succeeded, and the engine has already answered.build_media_identitycan raise on shapes it did not anticipate:prompt["prompt_token_ids"]evaluated before its early return,torch.stackover grids of mixed dtypes (only shapes are checked),prompt_token_ids[r.offset + first],torch.as_tensoron a non-tensor element. Any of those turned a request the engine had served into an error to the client. The identity is an optimisation with a working fallback: the router warns and the decode leg reprocesses the media.Solution
The build is wrapped: a failure logs
Request %s: media identity not built (%s); the decode leg will reprocess the media, with the request id and the exception, and the leg answers without an identity.Test Plan
test_vllm_pd_media_identity.py::test_an_identity_that_cannot_be_built_does_not_fail_the_request(vLLM lane, like the rest of that file):build_media_identityraises, the request completes with its output ids and no identity, and the warning names the cause.pytest grpc_servicer/tests: 406 passed, 10 skipped (vLLM-gated). Ruff clean.Checklist
cargo +nightly fmtpasses (no Rust change)cargo clippy --all-targets -- -D warningspasses (no Rust change)