[#17146][fix] Fix Laguna's reasoning parser - #17151
Open
1MrazorT1 wants to merge 2 commits into
Open
Conversation
Signed-off-by: 1MrazorT1 <1mohamedtoujani1@gmail.com>
Contributor
WalkthroughThe PR adds ChangesPoolside V1 reasoning support
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ChatRequest
participant PoolsideV1ReasoningParser
participant DeepSeekV4ReasoningParser
ChatRequest->>PoolsideV1ReasoningParser: provide template options and generated text
PoolsideV1ReasoningParser->>PoolsideV1ReasoningParser: default enable_thinking when unspecified
PoolsideV1ReasoningParser->>DeepSeekV4ReasoningParser: delegate parsing
DeepSeekV4ReasoningParser-->>ChatRequest: return reasoning and content
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/unittest/llmapi/test_reasoning_parser.py (1)
739-744: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse snake_case for the local variable.
Rename
Poolside_v1topoolside_v1.Proposed fix
- Poolside_v1 = ReasoningParserFactory.create_reasoning_parser("poolside_v1") + poolside_v1 = ReasoningParserFactory.create_reasoning_parser("poolside_v1") laguna = ReasoningParserFactory.create_reasoning_parser("laguna") - assert isinstance(Poolside_v1, PoolsideV1ReasoningParser) + assert isinstance(poolside_v1, PoolsideV1ReasoningParser)As per coding guidelines, Python variables must use snake_case.
🤖 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 `@tests/unittest/llmapi/test_reasoning_parser.py` around lines 739 - 744, Rename the local variable Poolside_v1 to poolside_v1 in test_poolside_v1_alias_same_parser, and update its corresponding assertion to use the snake_case name.Source: Coding guidelines
🤖 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 `@tests/unittest/api_stability/references/trtllm_serve_cli.yaml`:
- Line 371: Synchronize the reasoning_parser telemetry metadata with the
registered choices shown in the CLI reference: add deepseek_v4, minimax_m3, and
nemotron-v3 to the corresponding TelemetryField.categorical allowlist and
telemetry table, then regenerate
tensorrt_llm/usage/llm_args_golden_manifest.json. Preserve the existing
API-stability reference and ordering conventions.
In `@tests/unittest/llmapi/test_reasoning_parser.py`:
- Around line 110-116: Update
test_poolside_v1_reasoning_parser_extracts_when_not_thinking to parameterize
both enable_thinking=False and thinking=False, ensuring either explicit option
disables the default thinking mode. Replace the unnecessary f-string in the
reasoning_parser.parse call with a regular string literal to remove Ruff F541.
- Around line 92-133: Register the reasoning-parser unittest module containing
test_poolside_v1_reasoning_parser_extracts_when_thinking and
test_poolside_v1_reasoning_parser_streams_when_thinking in the manual-QA
integration test list. Preserve the existing test-list registration structure
and avoid adding duplicate entries.
---
Nitpick comments:
In `@tests/unittest/llmapi/test_reasoning_parser.py`:
- Around line 739-744: Rename the local variable Poolside_v1 to poolside_v1 in
test_poolside_v1_alias_same_parser, and update its corresponding assertion to
use the snake_case name.
🪄 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: b1d96cba-008b-4bd1-9886-08a68e1458aa
📒 Files selected for processing (6)
docs/source/developer-guide/telemetry.mdtensorrt_llm/llmapi/llm_args.pytensorrt_llm/llmapi/reasoning_parser.pytensorrt_llm/usage/llm_args_golden_manifest.jsontests/unittest/api_stability/references/trtllm_serve_cli.yamltests/unittest/llmapi/test_reasoning_parser.py
| - "--post_processor_hook" | ||
| reasoning_parser: | ||
| type: Choice(['auto', 'deepseek-r1', 'deepseek_v4', 'gemma4', 'kimi_k2', 'kimi_k25', 'laguna', 'minimax_m2', 'minimax_m2_append_think', 'minimax_m3', 'nano-v3', 'nemotron-v3', 'qwen3', 'qwen3_5']) | ||
| type: Choice(['auto', 'deepseek-r1', 'deepseek_v4', 'gemma4', 'kimi_k2', 'kimi_k25', 'laguna', 'minimax_m2', 'minimax_m2_append_think', 'minimax_m3', 'nano-v3', 'nemotron-v3', 'poolside_v1', 'qwen3', 'qwen3_5']) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 3 \
'reasoning_parser|minimax_m3|deepseek_v4|nemotron-v3' \
tensorrt_llm docs testsRepository: NVIDIA/TensorRT-LLM
Length of output: 50376
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- reasoning_parser references ---'
rg -n -C 5 \
'reasoning_parser|TelemetryField\.categorical|deepseek_v4|nemotron-v3|minimax_m3' \
tensorrt_llm/llmapi/llm_args.py \
docs/source/developer-guide/telemetry.md \
tensorrt_llm/usage/llm_args_golden_manifest.json \
tests/unittest/api_stability/references/trtllm_serve_cli.yaml
printf '%s\n' '--- relevant file sections ---'
sed -n '330,390p' tests/unittest/api_stability/references/trtllm_serve_cli.yaml
rg -n -C 12 'reasoning_parser' tensorrt_llm/llmapi/llm_args.py
rg -n -C 8 'reasoning_parser' docs/source/developer-guide/telemetry.md
rg -n -C 8 'reasoning_parser' tensorrt_llm/usage/llm_args_golden_manifest.jsonRepository: NVIDIA/TensorRT-LLM
Length of output: 32151
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- parser dispatch and validation ---'
rg -n -C 6 \
'reasoning_parser|nemotron-v3|minimax_m3|deepseek_v4' \
tensorrt_llm \
--glob '*.py' \
--glob '!tests/**'
printf '%s\n' '--- CLI choice generation ---'
rg -n -C 8 \
'Choice\(.*reasoning_parser|reasoning_parser.*Choice|serve.*reasoning_parser|reasoning_parser.*choices' \
tensorrt_llm scripts tests \
--glob '*.py' \
--glob '*.yaml'
printf '%s\n' '--- telemetry implementation and manifest generator ---'
rg -n -C 10 \
'class TelemetryField|categorical\(|golden_manifest|allowed_values|allowlist' \
tensorrt_llm/usage scripts tensorrt_llm/llmapi/llm_args.py \
--glob '*.py'
printf '%s\n' '--- all exact parser-value references ---'
rg -n \
"'(minimax_m3|deepseek_v4|nemotron-v3)'|\"(minimax_m3|deepseek_v4|nemotron-v3)\"" \
tensorrt_llm tests docs \
--glob '*.py' \
--glob '*.yaml' \
--glob '*.md' \
--glob '*.json'Repository: NVIDIA/TensorRT-LLM
Length of output: 50376
🏁 Script executed:
#!/usr/bin/env bash
set -u
printf '%s\n' '--- reasoning_parser definitions and consumers ---'
rg -n -C 10 'reasoning_parser' \
tensorrt_llm/llmapi/llm_args.py \
tensorrt_llm/llmapi \
tensorrt_llm/serve \
tensorrt_llm/entrypoints \
scripts \
tests \
--glob '*.py' \
--glob '*.yaml' \
--glob '*.json' 2>/dev/null || true
printf '%s\n' '--- exact values outside generated/reference files ---'
rg -n \
'minimax_m3|deepseek_v4|nemotron-v3' \
tensorrt_llm/llmapi/llm_args.py \
tensorrt_llm/serve \
tensorrt_llm/entrypoints \
scripts \
--glob '*.py' 2>/dev/null || true
printf '%s\n' '--- telemetry implementation and generator files ---'
rg -l \
'class TelemetryField|def categorical|generate_llm_args_golden_manifest|allowed_values' \
tensorrt_llm scripts \
--glob '*.py' 2>/dev/null | sort
printf '%s\n' '--- manifest entry ---'
sed -n '1225,1260p' tensorrt_llm/usage/llm_args_golden_manifest.json
printf '%s\n' '--- source field ---'
sed -n '4468,4485p' tensorrt_llm/llmapi/llm_args.pyRepository: NVIDIA/TensorRT-LLM
Length of output: 50376
Synchronize reasoning_parser telemetry metadata. deepseek_v4, minimax_m3, and nemotron-v3 are registered parser choices but are absent from the telemetry allowlist, golden manifest, and telemetry table. Add them to TelemetryField.categorical(...), regenerate tensorrt_llm/usage/llm_args_golden_manifest.json, and update the table. Test coverage: no test functions changed; this file is an API-stability reference.
🤖 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 `@tests/unittest/api_stability/references/trtllm_serve_cli.yaml` at line 371,
Synchronize the reasoning_parser telemetry metadata with the registered choices
shown in the CLI reference: add deepseek_v4, minimax_m3, and nemotron-v3 to the
corresponding TelemetryField.categorical allowlist and telemetry table, then
regenerate tensorrt_llm/usage/llm_args_golden_manifest.json. Preserve the
existing API-stability reference and ordering conventions.
Comment on lines
+92
to
+133
| @pytest.mark.parametrize("chat_template_kwargs", [{ | ||
| "thinking": True | ||
| }, { | ||
| "enable_thinking": True | ||
| }, None, {}, { | ||
| "random_key": "random_value" | ||
| }]) | ||
| def test_poolside_v1_reasoning_parser_extracts_when_thinking( | ||
| chat_template_kwargs: dict): | ||
| reasoning_parser = ReasoningParserFactory.create_reasoning_parser( | ||
| "poolside_v1", chat_template_kwargs) | ||
|
|
||
| result = reasoning_parser.parse(f"hidden{R1_END}visible") | ||
|
|
||
| assert result.content == "visible" | ||
| assert result.reasoning_content == "hidden" | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("chat_template_kwargs", [{"enable_thinking": False}]) | ||
| def test_poolside_v1_reasoning_parser_extracts_when_not_thinking( | ||
| chat_template_kwargs: dict): | ||
| reasoning_parser = ReasoningParserFactory.create_reasoning_parser( | ||
| "poolside_v1", chat_template_kwargs) | ||
|
|
||
| result = reasoning_parser.parse(f"visible") | ||
|
|
||
| assert result.content == "visible" | ||
| assert result.reasoning_content == "" | ||
|
|
||
|
|
||
| def test_poolside_v1_reasoning_parser_streams_when_thinking(): | ||
| reasoning_parser = ReasoningParserFactory.create_reasoning_parser( | ||
| "poolside_v1", {"enable_thinking": True}) | ||
|
|
||
| deltas = ["hid", f"den{R1_END}visible", " tail"] | ||
| results = [reasoning_parser.parse_delta(delta) for delta in deltas] | ||
|
|
||
| assert [result.content for result in results] == ["", "visible", " tail"] | ||
| assert [result.reasoning_content | ||
| for result in results] == ["hid", "den", ""] | ||
|
|
||
|
|
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -t f . tests/integration/test_lists | sort
rg -n -i -C 2 'test_reasoning_parser|llmapi/test_reasoning_parser' \
tests/integration/test_lists || trueRepository: NVIDIA/TensorRT-LLM
Length of output: 5335
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- changed-file summary ---'
git diff --stat
echo '--- target test file diff ---'
git diff -- tests/unittest/llmapi/test_reasoning_parser.py
echo '--- test-list registrations ---'
rg -n -i -C 3 'test_reasoning_parser|llmapi/test_reasoning_parser' \
tests/integration/test_lists || true
echo '--- test names in the target file ---'
rg -n '^def test_|^async def test_' tests/unittest/llmapi/test_reasoning_parser.py
echo '--- relevant target-file sections ---'
cat -n tests/unittest/llmapi/test_reasoning_parser.py | sed -n '1,170p'
cat -n tests/unittest/llmapi/test_reasoning_parser.py | sed -n '300,365p'
cat -n tests/unittest/llmapi/test_reasoning_parser.py | sed -n '700,760p'Repository: NVIDIA/TensorRT-LLM
Length of output: 16692
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git diff --stat
git diff -- tests/unittest/llmapi/test_reasoning_parser.py
rg -n -i -C 3 'test_reasoning_parser|llmapi/test_reasoning_parser' tests/integration/test_lists || true
rg -n '^def test_|^async def test_' tests/unittest/llmapi/test_reasoning_parser.py
cat -n tests/unittest/llmapi/test_reasoning_parser.py | sed -n '1,170p'
cat -n tests/unittest/llmapi/test_reasoning_parser.py | sed -n '300,365p'
cat -n tests/unittest/llmapi/test_reasoning_parser.py | sed -n '700,760p'Repository: NVIDIA/TensorRT-LLM
Length of output: 16525
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- QA list documentation ---'
cat -n tests/integration/test_lists/qa/README.md | sed -n '1,220p'
echo '--- QA references to unit-test paths and reasoning parser ---'
rg -n -i -C 2 'unittest/|reasoning_parser|llmapi' tests/integration/test_lists/qa || true
echo '--- test-db references to the target file ---'
rg -n -i -C 2 'unittest/llmapi/test_reasoning_parser.py' \
tests/integration/test_lists/test-db
echo '--- available coverage artifacts ---'
fd -t f -i 'cbts_touchmap.sqlite' . || true
fd -t f -i '*coverage*' tests/integration tests/unittest . 2>/dev/null | head -100 || trueRepository: NVIDIA/TensorRT-LLM
Length of output: 10160
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n tests/integration/test_lists/qa/README.md | sed -n '1,220p'
rg -n -i -C 2 'unittest/|reasoning_parser|llmapi' \
tests/integration/test_lists/qa || true
rg -n -i -C 2 'unittest/llmapi/test_reasoning_parser.py' \
tests/integration/test_lists/test-db
fd -t f -i 'cbts_touchmap.sqlite' . || true
fd -t f -i '*coverage*' tests/integration tests/unittest . 2>/dev/null | head -100 || trueRepository: NVIDIA/TensorRT-LLM
Length of output: 9985
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- parser implementation ---'
rg -n -C 8 'class PoolsideV1ReasoningParser|chat_template_kwargs|enable_thinking|thinking' \
tensorrt_llm tests/unittest/llmapi/test_reasoning_parser.py | head -240
echo '--- QA list context around unit-test entries ---'
cat -n tests/integration/test_lists/qa/llm_function_core.txt | sed -n '775,805p'Repository: NVIDIA/TensorRT-LLM
Length of output: 25474
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 'class PoolsideV1ReasoningParser|chat_template_kwargs|enable_thinking|thinking' \
tensorrt_llm tests/unittest/llmapi/test_reasoning_parser.py | head -240
cat -n tests/integration/test_lists/qa/llm_function_core.txt | sed -n '775,805p'Repository: NVIDIA/TensorRT-LLM
Length of output: 25395
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- matching test files and integration definitions ---'
fd -t f -i 'test_reasoning_parser.py' .
fd -t f -i '_test_openai_embeddings.py' .
fd -t d 'unittest' tests/integration/defs tests 2>/dev/null | head -40 || true
echo '--- test-db list scope ---'
cat -n tests/integration/test_lists/test-db/l0_a10.yml | sed -n '125,160p'Repository: NVIDIA/TensorRT-LLM
Length of output: 2345
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -t f -i 'test_reasoning_parser.py' .
fd -t f -i '_test_openai_embeddings.py' .
fd -t d 'unittest' tests/integration/defs tests 2>/dev/null | head -40 || true
cat -n tests/integration/test_lists/test-db/l0_a10.yml | sed -n '125,160p'Repository: NVIDIA/TensorRT-LLM
Length of output: 2262
Add unittest/llmapi/test_reasoning_parser.py to tests/integration/test_lists/qa/llm_function_core.txt.
- CI registration already exists in
tests/integration/test_lists/test-db/l0_a10.yml. - Added tests cover thinking, streaming, alias resolution, and auto-detection.
- Existing cases at lines 367–378 and 405–408 cover
enable_thinking=False. - Coverage verdict: sufficient after manual-QA registration.
🧰 Tools
🪛 Ruff (0.16.0)
[error] 116-116: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 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 `@tests/unittest/llmapi/test_reasoning_parser.py` around lines 92 - 133,
Register the reasoning-parser unittest module containing
test_poolside_v1_reasoning_parser_extracts_when_thinking and
test_poolside_v1_reasoning_parser_streams_when_thinking in the manual-QA
integration test list. Preserve the existing test-list registration structure
and avoid adding duplicate entries.
Source: Path instructions
Comment on lines
+110
to
+116
| @pytest.mark.parametrize("chat_template_kwargs", [{"enable_thinking": False}]) | ||
| def test_poolside_v1_reasoning_parser_extracts_when_not_thinking( | ||
| chat_template_kwargs: dict): | ||
| reasoning_parser = ReasoningParserFactory.create_reasoning_parser( | ||
| "poolside_v1", chat_template_kwargs) | ||
|
|
||
| result = reasoning_parser.parse(f"visible") |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cover explicit thinking=False and remove the lint error.
Line 110 covers only enable_thinking=False. Add thinking=False to verify that this explicit option prevents the default thinking mode. Line 116 has an unnecessary f-string and triggers Ruff F541.
Proposed fix
-@pytest.mark.parametrize("chat_template_kwargs", [{"enable_thinking": False}])
+@pytest.mark.parametrize("chat_template_kwargs", [
+ {"thinking": False},
+ {"enable_thinking": False},
+])
def test_poolside_v1_reasoning_parser_extracts_when_not_thinking(
chat_template_kwargs: dict):
@@
- result = reasoning_parser.parse(f"visible")
+ result = reasoning_parser.parse("visible")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @pytest.mark.parametrize("chat_template_kwargs", [{"enable_thinking": False}]) | |
| def test_poolside_v1_reasoning_parser_extracts_when_not_thinking( | |
| chat_template_kwargs: dict): | |
| reasoning_parser = ReasoningParserFactory.create_reasoning_parser( | |
| "poolside_v1", chat_template_kwargs) | |
| result = reasoning_parser.parse(f"visible") | |
| `@pytest.mark.parametrize`("chat_template_kwargs", [ | |
| {"thinking": False}, | |
| {"enable_thinking": False}, | |
| ]) | |
| def test_poolside_v1_reasoning_parser_extracts_when_not_thinking( | |
| chat_template_kwargs: dict): | |
| reasoning_parser = ReasoningParserFactory.create_reasoning_parser( | |
| "poolside_v1", chat_template_kwargs) | |
| result = reasoning_parser.parse("visible") |
🧰 Tools
🪛 Ruff (0.16.0)
[error] 116-116: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 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 `@tests/unittest/llmapi/test_reasoning_parser.py` around lines 110 - 116,
Update test_poolside_v1_reasoning_parser_extracts_when_not_thinking to
parameterize both enable_thinking=False and thinking=False, ensuring either
explicit option disables the default thinking mode. Replace the unnecessary
f-string in the reasoning_parser.parse call with a regular string literal to
remove Ruff F541.
Source: Linters/SAST tools
1 task
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.
Dev Engineer Review
PoolsideV1ReasoningParserforpoolside_v1andlaguna.DeepSeek-V4behavior and removes the standalonelagunaregistration fromDeepSeekR1Parser.minimax_m3.thinkingandenable_thinkingbehavior.enable_thinking=true.QA Engineer Review
tests/unittest/llmapi/test_reasoning_parser.py.</think>handling.poolside_v1.poolside_v1andlagunaparser aliases.tests/integration/test_lists/.Fixes #17146 .
Description
This PR contains the fix for Laguna's reasoning parser mentioned in the issue #17146.
This will serve as a continuation to what I have already established in the issue's description.
After looking at other implementations of parsers within the same file, I found out that DeepSeekV4's is the most suitable to fit Laguna's chat template, (with a small caveat that I will detail later on):
As we established in the issue, DeepSeekR1's parser with
reasoning_at_start=Trueis fully functional as a Laguna parser in thinking mode, however, it fails for non thinking mode by putting actual model content in reasoning content.This DeepSeekV4 parser fixes this issue by correctly using DeepSeekR1's parser with
reasoning_at_start=Truewhen the model is reasoning, and when the model is not reasoning, the parser defaults to IdentityReasoningParser which correctly copies actual content into the correct content field.Let's test this: first, I registered Laguna's reasoning parser as a DeepSeekV4 one, then, after starting up the server, let's test both
enable_thinking=Trueandenable_thinking=False:enable_thinking=True:{ "role": "assistant", "content": "Oui, les jumeaux, surtout les jumeaux identiques (monozygotes), sont à un risque accru d'hypothyroïdie, car cette condition a une composante génétique (souvent liée à des maladies auto-immunes comme le chuti thyroïdien de Hashimoto). Les jumeaux partagent donc des gènes identiques ou similaires, ce qui augmente la probabilité qu'ils développent des troubles thyroïdiens. Cependant, les facteurs environnementaux (infections, stress, etc.) jouent également un rôle. Les études montrent une concordance plus élevée chez les jumeaux identiques (30-50 %) que chez les jumeaux fraternels (10-20 %).", "reasoning_content": "Okay, the user is asking if twins are at higher risk for hypothyroidism. First, I need to recall what I know about hypothyroidism and twins. Hypothyroidism is when the thyroid gland doesn't produce enough hormones. It's often caused by autoimmune conditions like Hashimoto's thyroiditis, (etc)", "reasoning": null, "tool_calls": [] }enable_thinking=False:{ "role": "assistant", "content": "Les jumeaux ne sont pas intrinsèquement à un risque accru d'**hypothyroïdie** par rapport aux enfants d'inUkosa simple naissance. Toutefois, certains facteurs liés à la naissance prématurée ou à un faible poids de naissance (plus fréquents chez les jumeaux) peuvent augmenter temporairement le risque. Par exemple : \n1. **Naissance prématurée** : Le cerveau hypothalamo-hypophyso-dimégétique et les thyroïdes pourraient être moins matures, entraînant une surootérieure. \n2. **Surcharge en thyroxine (T4)** : La mère peut transmettre des anticorps stimulant la thyroïde (TSI), provoquant une thyroïdite passagère chez le nouveau-né. \n\n**Conclusion** : Un dépistage thyroidien néonatal est généralement effectué, mais une surveillance complémentaire peut être nécessaire chez les jumeaux si des facteurs de risque (prématurité, pathologies maternelles) sont présents. En l'absence de ces facteurs, le risque est similaire à celui des mono gaz.", "reasoning_content": "", "reasoning": null, "tool_calls": [] }We can see that the parser worked correctly on both explicitly set thinking flags.
However, recall that I had mentioned that DeepSeekV4's reasoning parser has a caveat for Laguna, and it appears when the user does not explicitly set a
enable_thinkingflag at all, here's the test:Output:
{ "role": "assistant", "content": "Okay, the user is asking if twins are at risk for hypothyroidism. Let me start by recalling what I know about hypothyroidism and twins. I remember that thyroid issues can have both genetic and environmental factors. Since twins share genetic material, maybe there's a higher risk for one twin if the other has it. But wait, monozygotic (identical) twins share 100% of their genes, while dizygotic (fraternal) twins share about 50%, like regular siblings. \n\nI think studies show that if one identical twin has hypothyroidism, the other has a higher chance compared to fraternal twins. But how much higher? I should check the concordance rates. For hypothyroidism, especially autoimmune types like Hashimoto's, the genetic component is significant. So maybe the risk is higher in identical twins. But it's not 100%, so environment and other factors still play a role. \n\nAlso, conditions like Down syndrome or other genetic disorders that are more common in twins might be linked to thyroid issues. Wait, Down syndrome is actually more common in twins? No, actually, the incidence of Down syndrome is similar in twins as in singletons. But if a twin has a genetic condition that affects the thyroid, that could be a factor. \n\nAnother angle: during pregnancy, if the mother has thyroid problems, it might affect the twins' thyroid development. But that's more about maternal health rather than the twins themselves being at higher risk. \n\nSo putting it all together: yes, twins, especially identical twins, have a higher risk of hypothyroidism due to shared genetics. But the exact risk depends on the type of hypothyroidism and other factors. I should mention both genetic and environmental influences, and maybe note that if one twin is diagnosed, the other should be monitored.</think>Oui, les jumeaux, surtout les jumeaux identiques (monozygotes), sont à un **risque plus élevé** d'hypothyroïdie, en raison d'une composante génétique forte. Si un jumeau développe une hypothyroïdie (notamment une forme auto-immune comme l'athyrïdie de Hashimoto), l'autre jumeau a une probabilité accrue de présenter la même pathologie, bien que cela ne soit pas systématique. \n\nLes facteurs environnementaux et les spécificités de la grossesse (ex. : troubles thyroïdiens maternels) peuvent également jouer un rôle. Il est donc recommandé de surveiller la thyroïde des deux jumeaux si l'un d'eux est diagnostiqué.", "reasoning_content": "", "reasoning": null, "tool_calls": [] }We can see that, not setting the flag at all, gets us back all the way to square one:
reasoning_content + </think> + contentare all coexisting in the samecontentblock.The root cause of this is the following: when looking at Laguna S2.1 chat template, we can see that it enables thinking by default:
And this is tracing all the way down to:
Which means that the model is thinking by default when that flag is not set, and when it is not set, DeepSeekV4's reasoning parser has no way of knowing it:
And that way, the parser defaults to non thinking mode whereas the model is actually thinking.
To fix this,
thinkingmust default totruewhen the user is not sending aenable_thinkingflag. Which is the logic that the code changes in this PR is following.Defaulting
thinkingto true is a decision I have made to match the documentation andgeneration_config.jsonfiles that Poolside ships their models with. All their model cards and mention that they enable thinking by default, and everygeneration_config.jsonadds explicitlydefault_chat_template_kwargs": {"enable_thinking": true}.I have however noticed an inconsistency with their M1 and XS models' jinja templates: they default thinking to
falsedespite the verifiable facts that I have just mentioned, that is fine for vLLM because it overrides it and defaults it back totruethanks to theirgeneration_config.json, but this is not the case for TensorRT-LLM since it does not read such files. And that is the reason one I have opened a PR in these models aiming to fix that inconsistency, and so that everything will be coherent.Now the only limitation is that XS and M1 models output will end up in reasoning content when
enable_thinkingis not precised, until Poolside accepts the changes on their Jinja templates or unless the user passes"chat_template_kwargs": {"enable_thinking": true}".Test Coverage
141 unit tests in tests/unittest/llmapi/test_reasoning_parser.py are passing correctly, and among them newly written tests for the new parser.
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.