fix(translation): report the served model on every response path - #169
Conversation
WalkthroughChangesServed model identity
Estimated code review effort: 4 (Complex) | ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
switchyard_rust/translation.py (1)
143-163: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument that
served_modelonly affects streaming translations.
served_modelis threaded throughresponse_to,response_for_request, andstream_for_request, but only the streaming branch of each actually uses it — the buffered branch always falls through totranslate_response/_response_bodyuntouched. This is intentional (the buffered body already carries the real served model from the upstream call), but none of the docstrings say so, which could mislead a future caller into thinkingserved_modelalso stamps buffered responses.📝 Suggested docstring clarification
def response_to( self, target: str | ChatRequestType, response: ChatResponse, *, served_model: str | None = None, ) -> ChatResponse: - """Return a ChatResponse wrapper in the target wire format.""" + """Return a ChatResponse wrapper in the target wire format. + + ``served_model`` only affects streaming responses (stamped into the + translated stream state); buffered responses keep whatever model the + source body already carries. + """As per coding guidelines, "Add concise triple-quoted docstrings for public functions, classes, methods, and API entry points, documenting behavior, important invariants, and relevant errors."
Also applies to: 165-199
🤖 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 `@switchyard_rust/translation.py` around lines 143 - 163, Update the docstrings for response_to, response_for_request, and stream_for_request to state that served_model is used only for streaming translations; buffered responses preserve the served-model information already present in their upstream body and are not stamped or modified by this parameter.Source: Coding guidelines
tests/test_response_translation_engine.py (1)
277-387: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove unnecessary
@pytest.mark.asynciodecorators.All five new tests in
TestTranslateStampsTheServedModeladd@pytest.mark.asyncio, but the repo relies on automatic asyncio mode.♻️ Suggested fix
- `@pytest.mark.asyncio` async def test_anthropic_stream_reports_the_served_model(self): ... - `@pytest.mark.asyncio` async def test_responses_stream_reports_the_served_model(self): ... - `@pytest.mark.asyncio` async def test_openai_chat_stream_reports_the_served_model(self): ... - `@pytest.mark.asyncio` async def test_buffered_response_reports_the_served_model(self): ... - `@pytest.mark.asyncio` async def test_stream_falls_back_to_the_upstream_model_without_a_selection(self):As per coding guidelines, "
tests/**/*.py: ... rely on the configured automatic asyncio mode without adding@pytest.mark.asynciounnecessarily."🤖 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_response_translation_engine.py` around lines 277 - 387, Remove the unnecessary `@pytest.mark.asyncio` decorators from all five async test methods in TestTranslateStampsTheServedModel, relying on the repository’s configured automatic asyncio mode while leaving the test bodies unchanged.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 `@crates/switchyard-server/tests/server.rs`:
- Around line 487-489: Remove the “SWITCH-922:” tracker reference from the
comment near the streaming codec behavior, while preserving its explanation that
codecs must report the routed target rather than the caller-addressed route ID
and why that matters to trajectory consumers.
In `@tests/test_deterministic_routing_profile.py`:
- Around line 644-652: Remove the SWITCH-922 tracker reference from the
docstring in tests/test_deterministic_routing_profile.py lines 644-652 while
preserving the clone-semantics and stats explanation; also remove “(SWITCH-922)”
from the section-header comment in tests/test_response_translation_engine.py
lines 261-264, with no other content changes.
---
Nitpick comments:
In `@switchyard_rust/translation.py`:
- Around line 143-163: Update the docstrings for response_to,
response_for_request, and stream_for_request to state that served_model is used
only for streaming translations; buffered responses preserve the served-model
information already present in their upstream body and are not stamped or
modified by this parameter.
In `@tests/test_response_translation_engine.py`:
- Around line 277-387: Remove the unnecessary `@pytest.mark.asyncio` decorators
from all five async test methods in TestTranslateStampsTheServedModel, relying
on the repository’s configured automatic asyncio mode while leaving the test
bodies unchanged.
🪄 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: 1c9787fb-ed36-46fe-aeb8-b4a2355f94a5
📒 Files selected for processing (14)
CHANGELOG.mdcrates/libsy-llm-client/src/client.rscrates/switchyard-server/src/lib.rscrates/switchyard-server/src/response.rscrates/switchyard-server/tests/server.rscrates/switchyard-translation/src/codecs/openai_chat/stream.rscrates/switchyard-translation/src/codecs/stream.rscrates/switchyard-translation/src/helpers.rscrates/switchyard-translation/tests/stream_translation.rsswitchyard/lib/processors/format_translate.pyswitchyard_rust/translation.pytests/test_deterministic_routing_profile.pytests/test_format_translate_processor.pytests/test_response_translation_engine.py
Signed-off-by: Eric Liu <zengyuanl@nvidia.com>
0f3b71b to
5a886c2
Compare
What
Switchyard sometimes reported the route id the client asked for as the response
model, instead of the model that actually served the request. This makes every serving path and wire format report the served model.Why
Closes SWITCH-922.
Benchmark route bundles register their route under the literal key
switchyard, so the Bench runs UI labelled trajectory stepsSwitchyard— not a model. It showed up as a mix within one run because only some paths were affected: streamed Anthropic/Responses replies on the staged chain, and everything libsy-served. Buffered and same-format-passthrough turns already reported the real model.The routed model was already correct in
x-model-router-selected-model,/v1/routing/stats, and Intake'sserved_model— only the response body disagreed. The fix feeds the existingtarget_modelplumbing fromProxyContext.selected_model(staged chain) andDecision::selected_model()(libsy) instead of from a request body.How tested
uv run ruff check ./uv run mypy switchyardcleanuv run pytest tests/green (1728 passed)cargo fmt --check,clippy --workspace --all-targets -- -D warnings,cargo test --workspacegreen (611 tests)message_start modelwent frommy-route-alias→upstream/real-model, same for/v1/responses,/v1/chat/completions, and both buffered formats.Each new regression test was confirmed to fail without the fix, not just pass with it.
One deselected test,
tests/test_verify.py::...::test_secrets_file_wins_when_no_cli_or_env, fails identically on a cleanmainon this machine (localsecrets.jsonleaking a base URL) — not a regression.Checklist
snake_caseof the primary class.switchyard/__init__.py.__all__— none added.--helpupdated if customer-facing surface changed — CHANGELOG entry added; no CLI change.Notes for reviewers
One deliberate behavior change beyond the bug fix: streamed OpenAI-chat replies now report Switchyard's configured target id (
anthropic/claude-opus-4.7) rather than the provider's own string, since I converged that codec on the accessor the other two already used. It also stops emitting the literal"unknown"when a provider omitsmodelon delta chunks. Worth a deliberate yes/no.Knock-on: a front proxy attributing spend by
response.model(e.g. LiteLLM) will now group by routed model rather than route alias.🤖 Generated with Claude Code