fix(processors): treat Fireworks model ids as reasoning-hint incompatible - #123
fix(processors): treat Fireworks model ids as reasoning-hint incompatible#123himorishige wants to merge 1 commit into
Conversation
…ible
Fireworks AI's OpenAI-compatible API validates request bodies strictly
and rejects the vLLM chat_template_kwargs hint with HTTP 400 ("Extra
inputs are not permitted, field: 'chat_template_kwargs'"). Fireworks
model ids always carry the provider namespace
(accounts/fireworks/models/...), so extend the deny list used by
model_accepts_reasoning_hint() to match them.
This stops the LLM classifier's disable_reasoning auto-detect from
injecting the hint at Fireworks-served classifier models, and — once
the DeepSeek tier overrides consult the same check — deterministic
tier calls as well.
Signed-off-by: Hiroshi Morishige <hiroshi.morishige@gmail.com>
WalkthroughThe reasoning hint compatibility check now rejects model IDs containing ChangesReasoning Hint Compatibility
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_reasoning_hint.py (1)
21-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the parametrized test to cover Fireworks models.
The test now covers more than the Claude family, so
test_claude_family_rejects_hintis misleading. Rename it to a provider-neutral name such astest_models_rejecting_reasoning_hint.🤖 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/test_reasoning_hint.py` around lines 21 - 23, Rename the parametrized test function test_claude_family_rejects_hint to a provider-neutral name such as test_models_rejecting_reasoning_hint, keeping its existing parameters and assertions unchanged.
🤖 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.
Nitpick comments:
In `@tests/test_reasoning_hint.py`:
- Around line 21-23: Rename the parametrized test function
test_claude_family_rejects_hint to a provider-neutral name such as
test_models_rejecting_reasoning_hint, keeping its existing parameters and
assertions unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ecfbcbb0-16db-4487-b250-1d290351914a
📒 Files selected for processing (2)
switchyard/lib/processors/reasoning_hint.pytests/test_reasoning_hint.py
| # Fireworks ids always carry the provider namespace | ||
| # (``accounts/fireworks/models/...``). vLLM-served reasoning models that | ||
| # need the hint never carry these tokens. | ||
| _NO_REASONING_HINT_TAGS = ("anthropic", "bedrock", "claude", "fireworks") |
There was a problem hiding this comment.
This adds fireworks to the deny list so we stop sending chat_template_kwargs to Fireworks. Recap for anyone reading: chat_template_kwargs is a vLLM-specific request field — it passes keyword arguments into the model's chat template and isn't part of the OpenAI API. model_accepts_reasoning_hint decides whether we send it, answering "no" only when the model name contains one of these tags.
Two things I looked into that make me wonder if a different structure would hold up better:
1. This list has to grow one provider at a time, and it already misses one. azure/deepseek-ai/deepseek-v4-pro runs on the same gateway, its name matches none of these tags, so we'd still send it the field — and Azure also validates the body strictly. Every new strict provider needs another tag added here by hand.
2. Each provider turns reasoning off with a different knob, so a yes/no "does it accept the hint?" can only ever suppress the field — it can't send the right one. From the docs and source:
| Provider | Accepts chat_template_kwargs? |
How you actually turn reasoning off |
|---|---|---|
| vLLM | yes (unknown fields are ignored, not rejected) | chat_template_kwargs.enable_thinking=false for Qwen3 — DeepSeek-V3.1 uses the key thinking with the opposite default |
| Fireworks | no — 400 | top-level reasoning_effort: "none" |
| Azure AI (DeepSeek) | no — 400 | no request parameter; the model decides |
| OpenAI | no | reasoning_effort |
| Anthropic | no | thinking: {"type": "disabled"} |
| DeepSeek API | no | choose the model: deepseek-chat (off) vs deepseek-reasoner (on) |
Sources: vLLM defines the field and allows (ignores) unknown ones rather than rejecting them — protocol.py, engine/protocol.py, reasoning outputs; Fireworks reasoning guide; OpenAI reasoning; Anthropic extended thinking; DeepSeek thinking mode; Azure chat reasoning.
Concrete suggestion: instead of a deny-list, a map keyed by provider / served-model family → a small dict of per-provider request settings. reasoning_off is just one entry in that dict, so the next provider-specific quirk we hit (a required header, another body field, a format tweak) becomes another key alongside it — not a whole new parallel map to keep in sync:
# provider / model-family -> per-provider request settings
# reasoning_off is one key here; future per-provider quirks live alongside it
PROVIDER_SETTINGS = {
"vllm-qwen": {"reasoning_off": {"chat_template_kwargs": {"enable_thinking": False}}},
"vllm-deepseek": {"reasoning_off": {}}, # DeepSeek-V3.1 default is off (key is `thinking`)
"fireworks": {"reasoning_off": {"reasoning_effort": "none"}},
"openai": {"reasoning_off": {"reasoning_effort": "minimal"}},
"anthropic": {"reasoning_off": {"thinking": {"type": "disabled"}}},
# azure deepseek / deepseek-api: no per-request reasoning switch
}
# lookup: PROVIDER_SETTINGS.get(provider, {}).get("reasoning_off")One honest caveat so we don't over-build it: the hard part is the key, not the map — vLLM's own knob differs by model family (Qwen3 enable_thinking vs DeepSeek-V3.1 thinking), so keying on the model-id string still needs care. If we'd rather not maintain provider detection here at all, the other option is to let each target/endpoint declare its reasoning-off body in config (I left that note on #122).
There was a problem hiding this comment.
Agreed the deny-list doesn't scale — your Azure example is a concrete miss today, and the deeper point stands: a yes/no check can only suppress the vLLM field, it can never send the provider's own knob. The measurements on the other thread back the map shape: Fireworks-served DeepSeek V4 accepts reasoning_effort: "none" and it collapses the classifier call from ~300–640 completion tokens down to a flat 63, with identical decisions.
Two observations for the map design, from reading current main:
ReasoningEffortNormalizeralready gives the inbound side areasoning_effortvocabulary; a provider map on the outbound side would meet it in the middle nicely.- Right now the only user-facing switch for this hint is
disable_reasoningon the escalation judge (route-bundle key). The deterministic classifier has no config path at all —make_classifier_config()is called withoutdisable_reasoning, so the model-name check is the only decision point. Whichever shape wins (provider map here, or the per-target flag you suggested on fix(profiles): gate DeepSeek thinking-off default on reasoning-hint compatibility #122), also exposing an explicit classifier override would give strict backends an escape hatch that doesn't depend on name detection.
Scope-wise I'd keep this PR as the minimal stop-the-400 fix and do the map as a follow-up — happy to take that on if useful.
| — usable directly as a classifier ``disable_reasoning`` default. | ||
| ``False`` for Anthropic/Bedrock/Claude and Fireworks-served ids (they | ||
| 400 on it), else ``True`` — usable directly as a classifier | ||
| ``disable_reasoning`` default. |
There was a problem hiding this comment.
Some context for this line: the LLM classifier is a small model that reads each incoming request and returns a JSON decision about which tier (weak or strong) should handle it. disable_reasoning controls whether we send enable_thinking=false. That hint matters because DeepSeek V4 is a reasoning model: without it, the model can put its JSON decision in the reasoning_content field and leave the normal content field empty. The classifier only reads content, so an empty content means it can't parse a decision and falls back to the default tier.
This change makes model_accepts_reasoning_hint return False for Fireworks, so the classifier stops sending the hint to a Fireworks classifier model. My question: does a Fireworks-served DeepSeek V4 still put its answer in content when we don't send the hint? If it behaves like the vLLM version and leaves content empty, we've replaced a clear error (the 400) with a silent one — the classifier gets nothing back and falls back on every request, with no error to notice.
Concrete suggestion: worth one real call to a Fireworks DeepSeek V4 with a classifier-style prompt and no hint, then check whether content is filled in. If it comes back empty, note that Fireworks disables reasoning with the top-level reasoning_effort: "none" field, not chat_template_kwargs (docs) — so the fix would be to send that for Fireworks rather than nothing, which is what the map on the line above would let us do. The two places that read disable_reasoning are llm_classifier/request_processor.py and stage_router/classifier.py.
There was a problem hiding this comment.
Ran the experiment you asked for — real calls to Fireworks-served DeepSeek V4 Flash (accounts/fireworks/models/deepseek-v4-flash), using the actual coding_agent classifier system prompt + its JSON schema as strict structured outputs (response_format: json_schema), temperature 0, max_tokens 4096 — i.e. the same request OpenAIChatLLMClassifierClient.classify() builds, varying only the body under test:
| prompt | extra body | HTTP | content |
valid JSON | decision | reasoning_content |
completion tokens | wall |
|---|---|---|---|---|---|---|---|---|
| simple | (none — post-#123 behavior) | 200 | filled | yes | simple | 941 chars | 303 | 3.2s |
| simple | reasoning_effort: "none" |
200 | filled | yes | simple | — | 63 | 0.9s |
| simple | chat_template_kwargs.enable_thinking=false |
400 | — | — | — | — | — | 0.2s |
| complex | (none) | 200 | filled | yes | complex | 2574 chars | 636 | 4.8s |
| complex | reasoning_effort: "none" |
200 | filled | yes | complex | — | 63 | 1.4s |
So: no silent failure. Without the hint, Fireworks still returns the JSON decision in content — reasoning lands in reasoning_content alongside it, not instead of it. The misroute from vllm-project/vllm#41132 doesn't reproduce on Fireworks' serving stack. In an earlier 18-prompt A/B I ran while tuning this setup, decisions with and without reasoning_effort: "none" also matched 18/18 — the hint's absence changes what the call costs, not what it decides.
The honest summary of this PR, then: it converts a hard 400 into a working-but-taxed classifier (~5–10× completion tokens, ~3.5× wall time per classifier call). The right endgame for Fireworks is exactly what you suggest — send reasoning_effort: "none" instead of nothing, which is what the provider-settings map would enable (see the other thread). I'd suggest landing this PR as the correctness fix — no more 400s, and no silent regression per the data above — with the map as a follow-up; happy to implement it.
2 participants
Fixes the classifier half of #121.
What
Fireworks AI's OpenAI-compatible API validates request bodies strictly and rejects the vLLM
chat_template_kwargshint with HTTP 400 (Extra inputs are not permitted, field: 'chat_template_kwargs'). Fireworks model ids always carry the provider namespace (accounts/fireworks/models/...), so this PR addsfireworksto_NO_REASONING_HINT_TAGS.With this, the LLM classifier's
disable_reasoningauto-detect stops injecting the hint for Fireworks-served classifier models, and — combined with #122 — deterministic tier calls stop injecting it as well.Validation
deepseek-v4-flash,deepseek-v4-pro,glm-5p2);tests/test_reasoning_hint.py13 passed.accounts/fireworks/models/deepseek-v4-flash) classifies successfully end to end (main @060ad758, Fireworks serverless).Summary by CodeRabbit