Skip to content

Commit ffa6142

Browse files
robinnarsinghranabhatmattfgithub-actions[bot]
authored
refactor: clean up reasoning implementation (#5429)
## Summary Follow-up cleanups to #5206 (reasoning output in responses API) and #5407 (type fixes), addressing items from [mattf's review](#5206 (comment)). **Code changes** - **Extracted shared reasoning util** (`src/llama_stack/providers/utils/inference/reasoning.py`): All three providers (vLLM, Ollama, Bedrock) had identical message mapping and chunk wrapping code. Pulled it into a shared `reasoning.py` util with no defaults -- each provider explicitly passes its field names. - **Simplified return type**: `openai_chat_completions_with_reasoning` is streaming-only across all providers. Removed `OpenAIChatCompletionWithReasoning` from return type in the API protocol, router, and all provider implementations. - **Removed dead code**: Deleted no-op `_prepare_reasoning_params` stubs from vLLM and Bedrock providers. Inlined Ollama's version (2-line default for `reasoning_effort`). - **Removed type ignores**: Eliminated `# type: ignore` on `_separate_tool_calls` assignment (reordered if/else to assign base type first) and on `tool_call.index` usage in streaming chunk processing. **Test changes:** - Added `test_reasoning_basic_streaming` and `test_reasoning_multi_turn_with_tool_call` to the `ollama-reasoning` test suite. - Un-skipped vLLM reasoning tests -- the CI action.yaml now configures a reasoning parser, so vLLM actually returns reasoning tokens. ## Addressed from [review](#5206 (comment)) | Item | Status | |------|--------| | `openai_chat_completions_with_reasoning` is streaming only, remove non-streaming return type | Done | | Remove unused `_prepare_reasoning_params` from vLLM | Done | | Remove unused `_prepare_reasoning_params` from Bedrock | Done | | Inline `_prepare_reasoning_params` for Ollama | Done | | bedrock, vllm, ollama need same transformations -- consider a util | Done | | Put `message = OpenAIAssistantMessageParam` case first to resolve type ignore | Done | | Figure out why there are new type ignores | Done (removed 5 type ignores) | | Type of `completion_result` confused by storing both CC and CC-with-reasoning | Done (removed non-streaming type from union) | ## TODO ( needs design discussion ) | Item | Question | |------|----------| | **Non-adjacent reasoning items** | `_get_preceding_reasoning` only checks `index - 1`, based on what I observed from official openai's response output. Is there a missing Edge case ? | | **Error vs silent fallback** | When a user asks for reasoning and the provider doesn't support it, should we return an error or silently proceed without reasoning? Currently falls back silently with a `critical` log. | | **Move merge logic to provider** | Suggestions Appreciated ... | | **Gemini implementation** | Gemini is strict on input shape and may use different field names (e.g. `"thinking"`). The shared util now supports custom field names, so this is ready to implement. | | **`n != 1` simplification** | Not sure what this means | | **Reduce `model_copy()` calls** | For safety, currently copying params in multiple places. Need to trace mutation paths to determine redundant copies. | | **Complete types on `_separate_tool_calls`** | Didn't tamper for now | ## Test plan - [x] BFCL Eval maintains accuracy - [x] vllm server renders prompt correctly with reasoning inside `analysis` channel - [x] `uv run pre-commit run mypy-full --hook-stage manual --all-files` # passes --------- Signed-off-by: robinnarsinghranabhat <robinnarsingha123@gmail.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Matthew Farrellee <matt@cs.wisc.edu> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
1 parent 5e79a81 commit ffa6142

19 files changed

Lines changed: 24046 additions & 2401 deletions

docs/docs/api-openai/provider_matrix.md

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ inference provider, based on integration test results.
2121
|----------|--------|---------|---------|----------|
2222
| azure | 111 | 111 | 0 | 82% |
2323
| bedrock | 27 | 27 | 0 | 20% |
24-
| ollama | 2 | 2 | 0 | 2% |
24+
| ollama | 3 | 3 | 0 | 2% |
2525
| openai | 136 | 136 | 0 | 100% |
2626
| vertexai | 70 | 70 | 0 | 52% |
2727
| vllm | 3 | 3 | 0 | 2% |
@@ -35,10 +35,10 @@ Models, endpoints, and versions used during test recordings.
3535
|----------|----------|----------|--------------|
3636
| azure | gpt-4o | llama-stack-test.openai.azure.com, lls-test.openai.azure.com, ogx-test.openai.azure.com | openai sdk: 2.43.0 |
3737
| bedrock | openai.gpt-oss-20b-1:0 | bedrock-runtime.us-west-2.amazonaws.com | openai sdk: 2.30.0 |
38-
| ollama | deepseek-r1:1.5b || openai sdk: 2.30.0 |
38+
| ollama | deepseek-r1:1.5b || openai sdk: 2.43.0 |
3939
| openai | gpt-4o, o4-mini, text-embedding-3-small | api.openai.com | openai sdk: 2.43.0 |
4040
| vertexai | publishers/google/models/gemini-2.0-flash || openai sdk: 2.5.0, provider: vertexai |
41-
| vllm | Qwen/Qwen3-0.6B || openai sdk: 2.5.0, vllm server: 0.18.1rc1.dev197+g0e9358c11 |
41+
| vllm | Qwen/Qwen3-0.6B || openai sdk: 2.5.0, vllm server: 0.17.2rc1.dev79+g5ce2d10e4 |
4242
| watsonx | meta-llama/llama-3-3-70b-instruct | us-south.ml.cloud.ibm.com | openai sdk: 2.5.0 |
4343

4444
## Basic Responses
@@ -165,7 +165,7 @@ Models, endpoints, and versions used during test recordings.
165165

166166
| Feature | azure | bedrock | ollama | openai | vertexai | vllm | watsonx |
167167
| --- | --- | --- | --- | --- | --- | --- | --- |
168-
| reasoning basic streaming ||| ⏭️ |||||
168+
| reasoning basic streaming ||| |||||
169169
| reasoning multi turn passthrough | ⏭️ |||| ⏭️ |||
170170
| reasoning no summary without request | ⏭️ | ⏭️ | ⏭️ || ⏭️ | ⏭️ ||
171171
| reasoning non streaming | ⏭️ |||| ⏭️ |||

src/ogx/core/routers/inference.py

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,6 @@
4545
OpenAIChatCompletionResponseMessage,
4646
OpenAIChatCompletionToolCall,
4747
OpenAIChatCompletionToolCallFunction,
48-
OpenAIChatCompletionWithReasoning,
4948
OpenAIChoice,
5049
OpenAIChoiceLogprobs,
5150
OpenAICompletion,
@@ -316,7 +315,7 @@ async def openai_chat_completion(
316315
async def openai_chat_completions_with_reasoning(
317316
self,
318317
params: OpenAIChatCompletionRequestWithExtraBody,
319-
) -> OpenAIChatCompletionWithReasoning | AsyncIterator[OpenAIChatCompletionChunkWithReasoning]:
318+
) -> AsyncIterator[OpenAIChatCompletionChunkWithReasoning]:
320319
"""Called by the Responses layer when a user requests reasoning.
321320
322321
Routes to the provider's reasoning-aware CC implementation, which

src/ogx/providers/inline/responses/builtin/responses/streaming.py

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -569,11 +569,12 @@ async def create_response(self) -> AsyncIterator[OpenAIResponseObjectStream]:
569569
)
570570
# Use reasoning-aware method when reasoning is explicitly requested
571571
completion_result: (
572-
OpenAIChatCompletion
573-
| AsyncIterator[OpenAIChatCompletionChunk]
574-
| OpenAIChatCompletionWithReasoning
572+
OpenAIChatCompletionWithReasoning
573+
| OpenAIChatCompletion
575574
| AsyncIterator[OpenAIChatCompletionChunkWithReasoning]
576-
)
575+
| AsyncIterator[OpenAIChatCompletionChunk]
576+
| None
577+
) = None
577578
if self.reasoning and self.reasoning.effort and self.reasoning.effort != "none":
578579
try:
579580
# Pass a copy — the router mutates params.model (strips provider prefix).
@@ -598,6 +599,7 @@ async def create_response(self) -> AsyncIterator[OpenAIResponseObjectStream]:
598599
completion_result_data = stream_event_or_result
599600
else:
600601
yield stream_event_or_result
602+
601603
# If violation detected, skip the rest of processing since we already sent refusal
602604
if self.violation_detected:
603605
return
@@ -810,20 +812,18 @@ def _separate_tool_calls(
810812

811813
for choice in current_response.choices:
812814
# Convert response message to input message format for multi-turn.
813-
# Use AssistantMessageWithReasoning if reasoning was present in the
814-
# CC response. Providers will be check for this AssistantMessageWithReasoning
815-
# message
815+
# Assign base type first, then narrow to AssistantMessageWithReasoning
816+
# if reasoning was present in the CC response.
817+
message: OpenAIAssistantMessageParam | AssistantMessageWithReasoning = OpenAIAssistantMessageParam(
818+
content=choice.message.content,
819+
tool_calls=choice.message.tool_calls,
820+
)
816821
if reasoning_content:
817822
message = AssistantMessageWithReasoning(
818823
content=choice.message.content,
819824
tool_calls=choice.message.tool_calls,
820825
reasoning_content=reasoning_content,
821826
)
822-
else:
823-
message = OpenAIAssistantMessageParam( # type: ignore[assignment]
824-
content=choice.message.content,
825-
tool_calls=choice.message.tool_calls,
826-
)
827827
next_turn_messages.append(message)
828828
logger.debug("Choice message content", content=choice.message.content)
829829
logger.debug("Choice message tool_calls", tool_calls=choice.message.tool_calls)
@@ -1239,7 +1239,7 @@ async def _process_streaming_chunks(
12391239
# chunk: OpenAIChatCompletionChunk annotation above.
12401240
if chunk_choice.delta.tool_calls:
12411241
for tool_call in chunk_choice.delta.tool_calls:
1242-
response_tool_call = chat_response_tool_calls.get(tool_call.index, None) # type: ignore[arg-type]
1242+
response_tool_call = chat_response_tool_calls.get(tool_call.index, None)
12431243
# Create new tool call entry if this is the first chunk for this index
12441244
is_new_tool_call = response_tool_call is None
12451245
if is_new_tool_call:
@@ -1251,16 +1251,16 @@ async def _process_streaming_chunks(
12511251
if tool_call_dict.get("function") and tool_call_dict["function"].get("arguments") is None:
12521252
tool_call_dict["function"]["arguments"] = ""
12531253
response_tool_call = OpenAIChatCompletionToolCall(**tool_call_dict)
1254-
chat_response_tool_calls[tool_call.index] = response_tool_call # type: ignore[index]
1254+
chat_response_tool_calls[tool_call.index] = response_tool_call
12551255

12561256
# Create item ID for this tool call for streaming events
12571257
tool_call_item_id = f"fc_{uuid.uuid4()}"
1258-
tool_call_item_ids[tool_call.index] = tool_call_item_id # type: ignore[index]
1258+
tool_call_item_ids[tool_call.index] = tool_call_item_id
12591259

12601260
# Emit output_item.added event for the new function call
12611261
self.sequence_number += 1
1262-
is_mcp_tool = tool_call.function.name and tool_call.function.name in self.mcp_tool_to_server # type: ignore[union-attr]
1263-
if not is_mcp_tool and tool_call.function.name not in _SERVER_SIDE_BUILTIN_TOOL_NAMES: # type: ignore[union-attr]
1262+
is_mcp_tool = tool_call.function.name and tool_call.function.name in self.mcp_tool_to_server
1263+
if not is_mcp_tool and tool_call.function.name not in _SERVER_SIDE_BUILTIN_TOOL_NAMES:
12641264
# for MCP tools (and even other non-function tools) we emit an output message item later
12651265
function_call_item = OpenAIResponseOutputMessageFunctionToolCall(
12661266
arguments="", # Will be filled incrementally via delta events

tests/integration/TARGET_MODELS.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ These jobs come from the `default` section of `ci_matrix.json`. They all run in
2222
| `bedrock-responses` | `bedrock` | 6 roots; Responses coverage: 27/136 (20%) |
2323
| `base-vllm-subset` | `vllm` | `inference` only |
2424
| `vllm-reasoning` | `vllm` | `test_reasoning.py` only; Responses coverage: 3/136 (2%) |
25-
| `ollama-reasoning` | `ollama-reasoning` | 3 roots; Responses coverage: 2/136 (1%) |
25+
| `ollama-reasoning` | `ollama-reasoning` | 5 roots; Responses coverage: 3/136 (2%) |
2626
| `messages` | `ollama` | `messages` only |
2727
| `messages-openai` | `gpt` | `messages` only |
2828
| `interactions` | `gemini` | `interactions` only |
@@ -80,7 +80,7 @@ This section is derived from the same replay recordings used to generate `docs/d
8080
| Vertex AI | 70 | 70 | 51% |
8181
| WatsonX | 61 | 61 | 45% |
8282
| Bedrock | 27 | 27 | 20% |
83+
| Ollama | 3 | 3 | 2% |
8384
| vLLM | 3 | 3 | 2% |
84-
| Ollama | 2 | 2 | 1% |
8585

8686
Total Responses features counted: 136.

tests/integration/common/recordings/models-64a2277c90f0f42576f60c1030e3a020403d34a95f56931b792d5939f4cebc57-fb68f5a6.json

Lines changed: 5 additions & 5 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

tests/integration/common/recordings/models-d98e7566147f9d534bc0461f2efe61e3f525c18360a07bb3dda397579e25c27b-be0c4c6b.json

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)