Skip to content

fix(processors): treat Fireworks model ids as reasoning-hint incompatible #123

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
himorishige wants to merge 1 commit into
base: main
Choose a base branch
from
+13 −5
Open

fix(processors): treat Fireworks model ids as reasoning-hint incompatible #123

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions switchyard/lib/processors/reasoning_hint.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,21 @@

from __future__ import annotations

# Anthropic-served (Claude) ids reject the vLLM ``chat_template_kwargs`` field;
# vLLM-served reasoning models that need the hint never carry these tokens.
_NO_REASONING_HINT_TAGS = ("anthropic", "bedrock", "claude")
# Anthropic-served (Claude) ids reject the vLLM ``chat_template_kwargs``
# field, and Fireworks AI's OpenAI-compatible API validates request bodies
# strictly and 400s on it as well ("Extra inputs are not permitted");
# 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")

@elyasmnvidian elyasmnvidian Jul 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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).

@himorishige himorishige Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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:

  • ReasoningEffortNormalizer already gives the inbound side a reasoning_effort vocabulary; 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_reasoning on the escalation judge (route-bundle key). The deterministic classifier has no config path at all — make_classifier_config() is called without disable_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.



def model_accepts_reasoning_hint(model: str) -> bool:
"""Whether ``model`` tolerates the vLLM ``enable_thinking=False`` hint.

``False`` for Anthropic/Bedrock/Claude ids (they 400 on it), else ``True``
— 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.

@elyasmnvidian elyasmnvidian Jul 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@himorishige himorishige Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

"""
lowered = model.lower()
return not any(tag in lowered for tag in _NO_REASONING_HINT_TAGS)
Expand Down
3 changes: 3 additions & 0 deletions tests/test_reasoning_hint.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@
"azure/anthropic/claude-opus-4-6",
"anthropic/claude-3-5-sonnet",
"Bedrock-Claude-Whatever", # case-insensitive
"accounts/fireworks/models/deepseek-v4-flash",
"accounts/fireworks/models/deepseek-v4-pro",
"accounts/fireworks/models/glm-5p2",
],
)
def test_claude_family_rejects_hint(model: str) -> None:
Expand Down