OLS-3442 Add reasoning token support spec#2985
Open
thoraxe wants to merge 1 commit into
Open
Conversation
Update service specs for per-model reasoning_config across all providers. Covers provider-specific reasoning enablement, vLLM ChatVLLMReasoning subclass, streaming reasoning accumulation, Bedrock provider addition, and deprecation of reasoning_effort/reasoning_summary/verbosity fields. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
openshift-ci
Bot
added
the
needs-rebase
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
3 tasks
📝 WalkthroughWalkthroughThis PR updates documentation across ChangesReasoning config specification updates
Estimated code review effort: 2 (Simple) | ~10 minutes Related Issues: OLS-3442 (reasoning token support via config-driven reasoning_config) Suggested labels: documentation Suggested reviewers: (none specified) 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.ai/spec/how/query-pipeline.md:
- Around line 300-302: The query-pipeline spec is describing behavior that is
not yet implemented in streaming_ols.py: StreamChunkType.REASONING is streamed,
but only StreamChunkType.TEXT is currently appended to the stored response.
Update the wording around LLMExecutionAgent._invoke_llm(),
_collect_round_llm_chunks(), and the [PLANNED: OLS-3442] note so it reflects the
actual shipped behavior, or clearly mark reasoning accumulation as a future
change rather than completed.
In @.ai/spec/what/config.md:
- Around line 112-123: Clarify the defaulting behavior for llm_providers[].type
in the config spec: the current default-to-name rule only works when the
provider name already matches a supported provider type, while aliased names
require an explicit type. Update the documentation in the llm_providers schema
section to state this constraint clearly, and note that aliases must set type
explicitly to avoid startup failures.
In @.ai/spec/what/llm-providers.md:
- Around line 36-49: Clarify the precedence rule for mixed reasoning settings in
the provider spec: when `parameters.reasoning_config` is present alongside
`reasoning_effort`, `reasoning_summary`, or `verbosity`, define that one source
wins and the other inputs are ignored or rejected. Update the `llm-providers`
guidance near the `reasoning_config` rules so providers like `ChatOpenAI`,
`AzureChatOpenAI`, `ChatGoogleGenerativeAI`, and `ChatAnthropicVertex` have a
deterministic payload shape during migration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 33d6f8aa-e857-4bf6-85ee-7e39198ea6cc
📒 Files selected for processing (5)
.ai/spec/how/llm-providers.md.ai/spec/how/query-pipeline.md.ai/spec/what/config.md.ai/spec/what/conversation-history.md.ai/spec/what/llm-providers.md
Comment on lines
+300
to
+302
| In non-final rounds, `LLMExecutionAgent._invoke_llm()` binds tools to the LLM (no `tool_choice` constraint). `_collect_round_llm_chunks()` accumulates all chunks from one LLM invocation, separating tool-call chunks from text/reasoning chunks. Text and reasoning chunks are yielded immediately for streaming. [PLANNED: OLS-3442] For vLLM providers, reasoning arrives via `chunk.additional_kwargs["reasoning_content"]` (captured by `ChatVLLMReasoning`) rather than `content_blocks` — a check for this source is added after the existing `match chunk.content` block. After the LLM stream ends, `_process_tool_calls_for_round()` emits `TOOL_CALL` events, executes the tools, then emits `TOOL_RESULT` events. The AI message (including any reasoning content) and all tool messages are appended to the prompt template for the next round. | ||
|
|
||
| [PLANNED: OLS-3442] In `streaming_ols.py`, both `StreamChunkType.TEXT` and `StreamChunkType.REASONING` chunks are accumulated into the `response` string. Previously only TEXT chunks were accumulated, causing reasoning to be lost from the stored conversation history. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== files ==\n'
git ls-files | rg '(^|/)(streaming_ols\.py|query-pipeline\.md|StreamChunkType|stream_chunk|streaming_ols)' || true
printf '\n== search for StreamChunkType handling ==\n'
rg -n "StreamChunkType|reasoning_content|response\s*\+=" -S . || true
printf '\n== inspect relevant docs snippet ==\n'
if [ -f .ai/spec/how/query-pipeline.md ]; then
nl -ba .ai/spec/how/query-pipeline.md | sed -n '285,310p'
fi
printf '\n== inspect streaming_ols.py if present ==\n'
if [ -f streaming_ols.py ]; then
nl -ba streaming_ols.py | sed -n '1,260p'
fi
printf '\n== inspect matching path(s) if elsewhere ==\n'
fd -a 'streaming_ols.py|.*streaming.*ols.*\.py' . || trueRepository: openshift/lightspeed-service
Length of output: 11183
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
for path, start, end in [
("ols/app/endpoints/streaming_ols.py", 435, 460),
(".ai/spec/how/query-pipeline.md", 295, 305),
]:
print(f"\n== {path} ==")
lines = Path(path).read_text().splitlines()
for i in range(start, min(end, len(lines)) + 1):
print(f"{i:4d}: {lines[i-1]}")
PYRepository: openshift/lightspeed-service
Length of output: 3675
Update the spec to match streaming_ols.py.
REASONING is streamed, but only TEXT is appended to response today, so this reads like a shipped fix that isn’t in place yet. Either soften the spec to a planned change or add the missing append in code.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.ai/spec/how/query-pipeline.md around lines 300 - 302, The query-pipeline
spec is describing behavior that is not yet implemented in streaming_ols.py:
StreamChunkType.REASONING is streamed, but only StreamChunkType.TEXT is
currently appended to the stored response. Update the wording around
LLMExecutionAgent._invoke_llm(), _collect_round_llm_chunks(), and the [PLANNED:
OLS-3442] note so it reflects the actual shipped behavior, or clearly mark
reasoning accumulation as a future change rather than completed.
Comment on lines
+112
to
+123
| | `llm_providers[].type` | string | =name | Provider type (openai, azure_openai, watsonx, rhoai_vllm, rhelai_vllm, google_vertex, google_vertex_anthropic, bedrock, fake_provider) | | ||
| | `llm_providers[].url` | URL | none | Provider endpoint URL | | ||
| | `llm_providers[].credentials_path` | string | none | Path to credential file or directory | | ||
| | `llm_providers[].models[]` | list | (required, >= 1) | Model definitions | | ||
| | `llm_providers[].models[].name` | string | (required) | Model identifier | | ||
| | `llm_providers[].models[].context_window_size` | int | 128000 | Context window size in tokens | | ||
| | `llm_providers[].models[].parameters.max_tokens_for_response` | int | 4096 | Tokens reserved for response | | ||
| | `llm_providers[].models[].parameters.tool_budget_ratio` | float | 0.25 | Fraction of context window for tool outputs (0.1--0.6) | | ||
| | `llm_providers[].models[].parameters.reasoning_effort` | enum | low | Reasoning effort level (low, medium, high) | | ||
| | `llm_providers[].models[].parameters.reasoning_summary` | enum | concise | Reasoning summary style (auto, concise, detailed) | | ||
| | `llm_providers[].models[].parameters.verbosity` | enum | low | General verbosity level (low, medium, high) | | ||
| | `llm_providers[].models[].parameters.reasoning_config` | dict | none | [PLANNED: OLS-3442] Freeform dict of provider-specific reasoning/thinking parameters. See `what/llm-providers.md` rule 13 for valid keys per provider | | ||
| | `llm_providers[].models[].parameters.reasoning_effort` | enum | low | [DEPRECATED: OLS-3442] Reasoning effort level (low, medium, high). Replaced by `reasoning_config` | | ||
| | `llm_providers[].models[].parameters.reasoning_summary` | enum | concise | [DEPRECATED: OLS-3442] Reasoning summary style (auto, concise, detailed). Replaced by `reasoning_config` | | ||
| | `llm_providers[].models[].parameters.verbosity` | enum | low | [DEPRECATED: OLS-3442] General verbosity level (low, medium, high). Replaced by `reasoning_config` | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the cited spec sections with line numbers.
file=".ai/spec/what/config.md"
if [ -f "$file" ]; then
echo "== $file (lines 100-170) =="
sed -n '100,170p' "$file" | nl -ba
else
echo "Missing $file"
fi
echo
echo "== Search for llm_providers[].type / allowlist / validation references =="
rg -n --no-heading "llm_providers\\[\\]\\.type|allowlist|registered provider|type.*name|type = name|provider instance name|aliases|alias" .ai/spec . 2>/dev/null || true
echo
echo "== Nearby references to line 153 in config.md =="
if [ -f "$file" ]; then
sed -n '145,160p' "$file" | nl -ba
fiRepository: openshift/lightspeed-service
Length of output: 252
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=".ai/spec/what/config.md"
echo "== $file (lines 100-170) =="
awk 'NR>=100 && NR<=170 { printf "%d:%s\n", NR, $0 }' "$file"
echo
echo "== Search for type/allowlist references =="
rg -n --no-heading "llm_providers\\[\\]\\.type|allowlist|registered provider|type.*name|type = name|provider instance name|aliases|alias" .ai/spec . 2>/dev/null || true
echo
echo "== Lines around 153 =="
awk 'NR>=145 && NR<=160 { printf "%d:%s\n", NR, $0 }' "$file"Repository: openshift/lightspeed-service
Length of output: 19131
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="ols/app/models/config.py"
echo "== provider type logic =="
awk 'NR>=430 && NR<=490 { printf "%d:%s\n", NR, $0 }' "$file"
echo
echo "== related tests =="
awk 'NR>=1500 && NR<=1545 { printf "%d:%s\n", NR, $0 }' tests/unit/app/models/test_config.py
echo
echo "== search for type validation / unsupported provider references =="
rg -n --no-heading "unsupported provider|supported provider types|Provider type must be one of|set_provider_type|type.*lower\\(\\)|defaulted from provider name|allowlist" ols tests .ai/spec/what/llm-providers.md .ai/spec/how/llm-providers.mdRepository: openshift/lightspeed-service
Length of output: 8555
Clarify llm_providers[].type defaulting The default only works when name is already one of the supported provider types; aliased provider names must set type explicitly or startup fails.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.ai/spec/what/config.md around lines 112 - 123, Clarify the defaulting
behavior for llm_providers[].type in the config spec: the current
default-to-name rule only works when the provider name already matches a
supported provider type, while aliased names require an explicit type. Update
the documentation in the llm_providers schema section to state this constraint
clearly, and note that aliases must set type explicitly to avoid startup
failures.
Comment on lines
+36
to
+49
| 11. Reasoning enablement is an explicit per-model configuration decision, not auto-detected from the model name. When a model's `parameters.reasoning_config` is present (a freeform `dict[str, Any]`), the provider must apply provider-specific reasoning/thinking parameters to the LLM invocation and must not set standard sampling parameters (`temperature`, `top_p`, `frequency_penalty`). When `reasoning_config` is absent, the provider must apply standard sampling defaults. | ||
|
|
||
| 12. For reasoning-capable models, the provider must set reasoning-specific parameters (`reasoning_effort`, `reasoning_summary`, `verbosity`) drawn from the model's `parameters` configuration. Standard sampling parameters (`temperature`, `top_p`, `frequency_penalty`) must not be set. | ||
| 12. Each provider interprets the keys within `reasoning_config` according to its own backend API. Invalid keys produce a clear error from the provider API — OLS does not validate model-reasoning compatibility. The operator is responsible for correct configuration. [PLANNED: OLS-3442] | ||
|
|
||
| 13. For standard (non-reasoning) models, the provider must set sampling parameters with sensible defaults (`temperature`, `top_p`, `frequency_penalty`). Reasoning parameters must not be set. | ||
| 13. The valid `reasoning_config` keys vary by provider and model generation: | ||
| - **OpenAI / Azure OpenAI**: `effort` (low/medium/high), `summary` (auto/concise/detailed), `verbosity` (low/medium/high). Passed as the `reasoning` dict and `verbosity` param to `ChatOpenAI` / `AzureChatOpenAI`. | ||
| - **Gemini 3 (Vertex)**: `thinking_level` (low/medium/high), `include_thoughts` (bool). Passed as kwargs to `ChatGoogleGenerativeAI`. | ||
| - **Gemini 2.5 (Vertex)**: `thinking_budget` (int token count), `include_thoughts` (bool). `thinking_budget` and `thinking_level` are mutually exclusive at the Gemini API level. | ||
| - **Anthropic (Vertex)**: `type` (enabled/adaptive), `display` (omitted/summarized), optionally `budget_tokens` (int). Passed as the `thinking` dict to `ChatAnthropicVertex`. | ||
| - **Bedrock (Anthropic models)**: `type` (adaptive). Passed as thinking configuration to `ChatBedrockConverse`. | ||
| - **Bedrock (OpenAI models)**: `effort` (low/medium/high), `summary` (auto/concise/detailed). Passed as the `reasoning` dict to `ChatOpenAI`. | ||
| - **vLLM (RHELAI / RHOAI)**: `enabled` (bool). When `true`, triggers use of the `ChatVLLMReasoning` subclass instead of `ChatOpenAI`. | ||
|
|
||
| 14. `reasoning_effort` and `verbosity` accept values `low`, `medium`, or `high`. `reasoning_summary` accepts `auto`, `concise`, or `detailed`. | ||
| 14. Provider APIs return 400 errors if reasoning parameters are sent to non-reasoning models. There is no reliable cross-provider API to auto-detect reasoning capability at startup. [PLANNED: OLS-3442] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- file outline ---'
ast-grep outline .ai/spec/what/llm-providers.md || true
echo '--- search reasoning-related terms ---'
rg -n "reasoning_config|reasoning_effort|reasoning_summary|verbosity|thinking_budget|thinking_level|include_thoughts" .ai/spec/what/llm-providers.md .ai/spec -S || true
echo '--- read nearby sections ---'
nl -ba .ai/spec/what/llm-providers.md | sed -n '1,260p'Repository: openshift/lightspeed-service
Length of output: 13767
Define how mixed reasoning configs resolve.
reasoning_config is marked as replacing reasoning_effort / reasoning_summary / verbosity, but the spec never says whether mixed inputs are rejected or whether reasoning_config wins. Add one rule so migration configs can’t produce ambiguous provider payloads.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.ai/spec/what/llm-providers.md around lines 36 - 49, Clarify the precedence
rule for mixed reasoning settings in the provider spec: when
`parameters.reasoning_config` is present alongside `reasoning_effort`,
`reasoning_summary`, or `verbosity`, define that one source wins and the other
inputs are ignored or rejected. Update the `llm-providers` guidance near the
`reasoning_config` rules so providers like `ChatOpenAI`, `AzureChatOpenAI`,
`ChatGoogleGenerativeAI`, and `ChatAnthropicVertex` have a deterministic payload
shape during migration.
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
1 participant
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.
Summary
what/llm-providers.md,how/llm-providers.md) for config-driven reasoning enablement viareasoning_config, replacing model-name detectionChatVLLMReasoningsubclass designwhat/config.md) withreasoning_configfield, deprecatereasoning_effort/reasoning_summary/verbositywhat/conversation-history.md) with reasoning accumulation behaviorhow/query-pipeline.md) with streaming reasoning sources and accumulationContext
Companion spec PRs:
Test plan
reasoningConfigreference table for accuracy per providerwhat/llm-providers.md🤖 Generated with Claude Code
Summary by CodeRabbit
reasoning_configapproach and deprecated legacy reasoning fields.