Skip to content

docs: account for vLLM reasoning field migration in plan 343#377

Merged
nabinchha merged 3 commits intomainfrom
nmulepati/chore/address-reasoning-field-in-plan343
Mar 6, 2026
Merged

docs: account for vLLM reasoning field migration in plan 343#377
nabinchha merged 3 commits intomainfrom
nmulepati/chore/address-reasoning-field-in-plan343

Conversation

@nabinchha
Copy link
Contributor

@nabinchha nabinchha commented Mar 6, 2026

📋 Summary

Updates the model facade overhaul plan (plans/343) to explicitly account for the vLLM message.reasoningmessage.reasoning_content field migration identified in #374. This ensures the PR-3 OpenAI-compatible adapter won't silently drop reasoning traces after LiteLLM removal.

🔄 Changes

📚 Docs

  • plans/343/model-facade-overhaul-plan-step-1.md — four targeted additions:
    • OpenAI adapter scope (item 5): expanded to specify dual-field reasoning extraction (reasoning_content + reasoning), shared helper approach, and backlink to Handle vLLM/OpenAI-compatible reasoning field migration before LiteLLM removal #374
    • OpenAI response parsing table (item 3): updated to document both field names with precedence rule
    • Adapter unit tests (new item 8): added three specific test cases for reasoning field migration coverage
    • Risks section (new entry): added "silent reasoning trace loss after LiteLLM removal" with mitigation strategy and issue reference

🤖 Generated with AI

Closes #374

@nabinchha nabinchha requested a review from a team as a code owner March 6, 2026 16:51
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 6, 2026

Greptile Summary

This PR updates the model-facade overhaul planning document (plans/343/model-facade-overhaul-plan-step-1.md) to explicitly account for the vLLM message.reasoningmessage.reasoning_content field migration identified in issue #374, ensuring the upcoming OpenAI-compatible adapter won't silently drop reasoning traces after LiteLLM is removed.

Key updates and verification:

  • Item 5 (adapter scope): Expanded to require dual-field extraction via a shared helper in parsing.py, with message.reasoning taking precedence as the vLLM >= 0.16.0 canonical field and message.reasoning_content as the legacy/LiteLLM-normalized fallback. This precedence is correctly applied throughout.
  • Response parsing table (item 3): Updated to document both field names with reasoning taking precedence, consistent with item 5 and the Risks section.
  • Adapter unit tests (item 8): Three test cases specified covering the "only reasoning", "only reasoning_content", and "both present" scenarios.
  • Risks section: New entry correctly describes the risk ("silent reasoning trace loss after LiteLLM removal") and mitigation strategy ("checks reasoning first, falling back to reasoning_content"), with consistent precedence ordering throughout the document.

The previously flagged precedence contradiction has been resolved — the plan now correctly identifies message.reasoning as canonical and message.reasoning_content as the fallback, with the implementation approach ("falls back") clearly guiding developers toward the correct handling of both fields.

Confidence Score: 5/5

  • Documentation-only PR updating planning document; all reasoning field migration guidance is internally consistent with correct precedence and fallback strategy.
  • This PR is safe to merge. The major issue flagged in prior review — precedence contradiction between item 5 and the Risks section — has been correctly resolved. The vLLM >= 0.16.0 canonical field message.reasoning now takes precedence over message.reasoning_content consistently across all three locations (item 5 scope, response parsing table, and Risks section). The implementation approach ("checks reasoning first, falling back to reasoning_content") is clearly specified. This is a documentation-only change with no executable code, and all statements are internally consistent.
  • No files require special attention

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[vLLM / OpenAI-compatible response] --> B{message.reasoning present\nand truthy?}
    B -- Yes --> C[Use message.reasoning\nvLLM >= 0.16.0 canonical]
    B -- No --> D{message.reasoning_content\npresent and truthy?}
    D -- Yes --> E[Use message.reasoning_content\nlegacy / LiteLLM-normalized fallback]
    D -- No --> F[reasoning_content = None\nno reasoning trace]
    C --> G[canonical reasoning_content\ninternal model field]
    E --> G
    F --> G
Loading

Last reviewed commit: 5e0d5d0

Copy link
Contributor

@eric-tramel eric-tramel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just leaving a review to agree with greptile on this one: let's make sure the new field takes precedence over the one getting deprecated.

@nabinchha nabinchha requested a review from eric-tramel March 6, 2026 18:34
@nabinchha nabinchha merged commit e78be09 into main Mar 6, 2026
47 checks passed
@nabinchha nabinchha deleted the nmulepati/chore/address-reasoning-field-in-plan343 branch March 6, 2026 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Handle vLLM/OpenAI-compatible reasoning field migration before LiteLLM removal

2 participants