fix(frontend): preserve completion backend error status - #12706
Conversation
github-actions
Bot
added
fix
frontend
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
WalkthroughChangesCompletion backend error handling
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![dynamo-review-agent[bot]](https://avatars.githubusercontent.com/u/201626793?s=60&v=4){kind=small}
a0c7004 to
fa65616
Compare
|
/ok to test fa65616 |
|
🎯 Code Coverage (details) 🔗 Commit SHA: fa65616 | Docs | Datadog PR Page | Give us feedback! |
| where | ||
| S: futures::Stream<Item = Annotated<NvCreateCompletionResponse>> + Send + 'static, | ||
| { | ||
| futures::future::try_join_all( |
There was a problem hiding this comment.
Reviewed locally with Codex: try_join_all waits for every prompt stream to produce its first non-annotation frame before any buffered frame reaches select_all. If prompt A produces a token at 10 ms and prompt B at 500 ms, A's metrics aren't observed and http_queue_guard isn't dropped until roughly 500 ms, inflating batch TTFT and queue time to the slowest prompt. Could we preserve the typed error while consuming the merged stream instead of adding this barrier?
There was a problem hiding this comment.
If prompt A produces a token at 10 ms and prompt B at 500 ms, A's metrics aren't observed and http_queue_guard isn't dropped until roughly 500 ms, inflating batch TTFT and queue time to the slowest prompt.
If this is correct, we need to fix this @KrishnanPrash
| // Preserve typed backend errors before the completions aggregator turns | ||
| // them into strings. In particular, Python ValueError/TypeError arrives | ||
| // as Backend(InvalidArgument) and must remain an HTTP 400. | ||
| let stream = check_for_backend_error(stream, None) |
There was a problem hiding this comment.
Reviewed locally with Codex: This only checks the first non-annotation frame. If the backend emits a normal completion chunk and then Backend(InvalidArgument), the later error reaches the aggregator, is reduced to a string, and is returned through the generic 500 path. Non-streaming requests should preserve typed errors across the full aggregation.
| } | ||
|
|
||
| // Merge all streams | ||
| let all_streams: Vec<BoxedCompletionResponseStream> = if streaming { |
There was a problem hiding this comment.
Reviewed locally with Codex: This boxes every prompt stream even when streaming is true, adding one allocation per prompt and dynamic dispatch on every poll of the unchanged streaming path. Can we keep these streams concrete and only box the merged stream if type erasure is needed?
There was a problem hiding this comment.
This may impact perf at high concurrency, we should verify
| }; | ||
|
|
||
| let error_response = | ||
| match check_for_backend_error(stream::iter(vec![error_event]), None).await { |
There was a problem hiding this comment.
Reviewed locally with Codex: This test exercises the helper directly, so it still passes if the production call in completions_single is removed; the batch test has the same gap. Please cover the handler/HTTP path so the regression is actually protected.
| let stream = check_for_backend_error(stream, None) | ||
| .await | ||
| .map_err(|error_response| { | ||
| tracing::error!(request_id, "Backend error detected: {:?}", error_response); |
There was a problem hiding this comment.
Reviewed locally with Codex: This formats the response into the log message and logs an expected client-side 400 at error level. Could this use a structured error_response field and a lower level?
dyn-3691-extract-shared-target-pid-cuda-customstorage-operation-layer * 'main' of https://github.com/ai-dynamo/dynamo: (65 commits) fix(frontend): emit SGLang stream role once (#12741) docs(fern): promote v1.3.1 to current release (#12752) fix(docs): remove duplicate unscoped community-rail CSS rules (#12615) feat(operator): migrate CRD storage to v1beta1 (#11904) fix: synchronize self-benchmark capacity across DP ranks (#12021) chore(deps): bump dynamo-tokenizers to 1.8.0 (#12707) fix(frontend): preserve split UTF-8 characters (#12688) docs: align Kubernetes build selector with CLI (#12729) fix(frontend): preserve completion backend error status (#12706) fix(operator): replace snapshot pods after GMS restart (#11286) refactor(media): rename installer module, drop --packages per review fix(media): harden installer against three pre-redesign review findings fix(media): verify installs in a fresh interpreter; teach --pip-args= form test(serve): install test-time decoders at the validated bounds feat(media): explicit installer for additional media decoders docs(spica): correct kv_load_ratio support guidance (#12714) feat(operator): add experimental grove.forceScalingGroup for single-node components (#11772) fix(vllm): declare entry-stage engine_input_source in GLM-Image NIXL config (#12709) chore: bump trtllm to v1.3.0rc23 (#12532) perf: remove trtllm postprocessing workers from the args as post processing workers are not effective in dynamo (#12592) ... Signed-off-by: Hannah Zhang <hannahz@nvidia.com>
3 participants
Overview:
Fix /v1/completions returning HTTP 500 when the backend reports a typed client error such as Backend(InvalidArgument).
The non-streaming completions path now checks for backend errors before response aggregation, preserving the original HTTP 400 status and descriptive message. The existing timeout-aware error-checking helper is generalized to support completion and chat response types.
Adds regression coverage for the SGLang logprobs >= 1 case.
Details:
Where should the reviewer start?
Related Issues
🔗 This PR is linked to an issue:
🚫 This PR is NOT linked to an issue:
Summary by CodeRabbit