Repository navigation
Conversation
…>= 0.20 [MLOS-950] vLLM 0.20 replaced EngineCoreOutput.num_cached_tokens with prefill_stats, which is only attached to a request's first output. Reading the old attribute raised AttributeError inside the OutputProcessor.process_outputs wrapper, which shut down the vLLM engine. Read cached tokens from the legacy attribute, prefill_stats, or the value RequestState keeps across iterations, and guard the process_outputs wrapper so instrumentation errors are logged instead of crashing the engine. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Dependency direction analysis
|
Circular import analysis
|
Codeowners resolved asResolved from the full PR diff against |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
dubloom
reviewed
Oct 6, 2026
Comment on lines
+176
to
+181
| try: | ||
| model_name = get_model_name(instance) | ||
| spans_data = _capture_request_states(instance, engine_core_outputs) | ||
| except Exception: | ||
| logger.warning("Failed to capture vLLM request state for tracing", exc_info=True) | ||
| return func(*args, **kwargs) |
Contributor
There was a problem hiding this comment.
Do we really need the try catch there and below ? This seems like over protecting as it seems the fix is above
Collaborator
Author
There was a problem hiding this comment.
Thanks for the review! I'd like to keep both guards, for these reasons:
- The fix above only covers one attribute. The crash in MLOS-950 was raised from
_capture_request_states, which nothing guards. This is the second time a vLLM release has broken this integration by changing internal structures (MLOS-854 was theprocessor→input_processorrename). Thellmobs::vllmsuite isskip: trueand GPU-gated, so CI won't catch the next upstream change. - The failure mode is unusually severe. This wrapper runs inside vLLM's engine output loop (
AsyncLLM.output_handler). An exception there doesn't just fail one request; it shuts down the whole inference server, which is what the customer hit. Dropping spans with a warning seems much better than taking down production inference. - The second block isn't redundant.
BaseLLMIntegration.llmobs_set_tagsalready catches its own errors, but_create_finished_spansalso callscreate_span,extract_latency_metricsandset_latency_metrics, which aren't guarded. That's the same pattern other LLM integrations use (e.g.openai/_realtime.py).
Happy to narrow the scope (e.g. only guard _capture_request_states) if you feel strongly about it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes MLOS-950. With vLLM >= 0.20, the first inference request fails with HTTP 500 and the vLLM engine shuts down:
Root cause: vLLM 0.20.0 removed
EngineCoreOutput.num_cached_tokensand replaced it withprefill_stats: PrefillStats | None.PrefillStats.num_cached_tokensholds the value. The exception is raised in ourOutputProcessor.process_outputswrapper, which runs in vLLM's engine output loop, so it takes down the whole engine.prefill_statsis only attached to a request's first emitted output (Request.take_prefill_stats()is one-shot). Swapping the attribute access alone would therefore reportnum_cached_tokens=0for nearly every multi-token completion, because spans are built from the final output. vLLM itself copies the value ontoRequestState.num_cached_tokens(present since 0.10.2), so we fall back to that.Changes:
extractors.get_num_cached_tokens()resolves cached tokens in this order: legacyEngineCoreOutput.num_cached_tokens(< 0.20), thenprefill_stats.num_cached_tokens(first output, >= 0.20), thenRequestState.num_cached_tokens(later outputs).traced_output_processor_process_outputsnow catches and logs instrumentation errors before and after calling the wrapped function. A future upstream schema change then drops spans instead of crashing the customer's inference server.Testing
tests/contrib/vllm/test_extractors.py: legacy attribute,prefill_stats,RequestStatefallback, andextract_request_dataon a >= 0.20-shaped output.tests/contrib/vllm/test_vllm_patch.py: both wrapper helpers raising still return the original result.v0.19.0,v0.20.0andv0.26.0source (vllm/v1/engine/__init__.py,output_processor.py,core/sched/scheduler.py).llmobs::vllmsuite isskip: trueand GPU-gated intests/llmobs/suitespec.yml, which is why CI didn't catch this regression.Risks
Low. Behavior on vLLM < 0.20 is unchanged. The new try/except only affects the error path.
Additional Notes
Customer workaround until this ships:
DD_TRACE_VLLM_ENABLED=false, or pin vLLM <= 0.19.🤖 Generated with Claude Code