fix: normalize empty content-filter responses instead of raising - #358
Conversation
WalkthroughThe LiteLLM response parser now accepts empty content when the finish reason is ChangesLiteLLM content filter handling
Estimated code review effort: 1 (Trivial) | ~5 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
examples/experimental/litellm/tests/test_client.py (1)
557-576: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a regression test for the preserved non-filter error path.
This test covers the allowed
content_filterpath. It does not verify that empty content with anotherfinish_reasonstill raisesValueError("LiteLLM returned no text content"). Add a companion test forfinish_reason="stop"withcontent=None.🤖 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 `@examples/experimental/litellm/tests/test_client.py` around lines 557 - 576, Add a companion test alongside test_response_content_filter_empty_content using finish_reason="stop" and content=None, then assert that _response raises ValueError with the exact message "LiteLLM returned no text content". Keep the existing content_filter test 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.
Inline comments:
In `@examples/experimental/litellm/tests/test_client.py`:
- Around line 563-576: Update the _response regression test fixture to construct
a typed ModelResponse instead of SimpleNamespace, preserving the content_filter
response values. Narrow or validate the returned outputs collection before
indexing it so the dict[str, object] result passes strict mypy checks.
---
Nitpick comments:
In `@examples/experimental/litellm/tests/test_client.py`:
- Around line 557-576: Add a companion test alongside
test_response_content_filter_empty_content using finish_reason="stop" and
content=None, then assert that _response raises ValueError with the exact
message "LiteLLM returned no text content". Keep the existing content_filter
test unchanged.
🪄 Autofix
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: ee5a76f1-690f-44cf-81a4-ce4df4182125
📒 Files selected for processing (2)
examples/experimental/litellm/src/switchyard_litellm/client.pyexamples/experimental/litellm/tests/test_client.py
|
Completed all CodeRabbit review recommendations:
Validation: |
## Summary
`_response()` raised `ValueError("LiteLLM returned no text content")` for any
response with empty `content`, including content-filter responses where empty
output is expected. This caused legitimate content-filter results to fail
instead of being normalized.
## Root cause
The guard at the end of `_response()` did not distinguish between a truly
empty/invalid response and a `content_filter` finish reason, which by design
returns no content.
## Fix
Allow empty `content` when the finish reason is `content_filter`; keep the
existing raise for all other empty-content cases so unexpected responses still
fail fast.
```diff
- if not content:
- raise ValueError("LiteLLM returned no text content")
+ if not content and choice.finish_reason != "content_filter":
+ raise ValueError("LiteLLM returned no text content")
```
## Testing
Added `test_response_content_filter_empty_content` to
`examples/experimental/litellm/tests/test_client.py` and verified the full
non-e2e suite passes:
```
uv run --project examples/experimental/litellm --python 3.12 \
pytest examples/experimental/litellm/tests/test_client.py -m "not e2e" -v
# 34 passed
```
## Contributor guidelines
- DCO sign-off included.
- One focused commit per PR.
Signed-off-by: andrewwhitecdw <andrewwhitecdw@users.noreply.github.com>
…path
CodeRabbit review feedback on the content-filter normalization fix:
- The regression test constructed the LiteLLM response with
`SimpleNamespace`, which mypy strict rejects because `_response()`
requires a typed `litellm.ModelResponse`. Construct a real
`ModelResponse` with `Choices`/`Message`/`Usage` from
`litellm.types.utils` instead, preserving the content_filter response
values.
- The normalized `outputs` collection is now narrowed with isinstance
checks before indexing so the `dict[str, object]` result passes
strict mypy checks (previously two new [index] errors).
- Added a companion regression test: empty content with any finish
reason other than `content_filter` (here `stop`) must still raise
`ValueError("LiteLLM returned no text content")`, guarding the
non-filter error path the original change intentionally preserves.
mypy strict on the two changed files: 11 errors before (base) -> 9 now,
all 9 remaining are pre-existing on main (verified via git stash); the
two errors introduced by the original test are gone.
Ruff check: clean.
pytest (non-e2e), from examples/experimental/litellm:
PYTHONPATH=src .venv/bin/python -m pytest tests/test_client.py -m "not e2e" -q
35 passed (34 before; the new companion test is the +1)
Signed-off-by: andrewwhitecdw <andrewwhitecdw@users.noreply.github.com>
d2c4535 to
09ea680
Compare
* origin/main: (31 commits) feat(server): add Dockerfile for switchyard-server container image (NVIDIA-NeMo#421) fix: normalize empty content-filter responses instead of raising (NVIDIA-NeMo#358) feat(python): unify LLM classifier bindings (NVIDIA-NeMo#465) feat(libsy): record task_kind and agent_role on the run span (NVIDIA-NeMo#249) fix(translation): accept SSE data fields with no space after the colon (NVIDIA-NeMo#447) fix(client): strip api-key and OpenAI org/project headers before forwarding (NVIDIA-NeMo#420) fix(llm-client): detect native sglang context-overflow messages (NVIDIA-NeMo#426) refactor(protocol): use typed HTTP status codes (NVIDIA-NeMo#457) fix(translation): preserve chat reasoning details (NVIDIA-NeMo#415) docs(changelog): note packaging extras removal in Unreleased (NVIDIA-NeMo#433) docs(changelog): fix broken Metrics Reference link (NVIDIA-NeMo#432) feat(libsy-llm-client): Move retry logic from libsy to libsy-llm-client (NVIDIA-NeMo#431) fix(server): use normalized session IDs in routing stats (NVIDIA-NeMo#430) feat(benchmark): pre-bake hermes agent into dataset images for closed-book runs (NVIDIA-NeMo#350) fix: support json_object classifier responses (NVIDIA-NeMo#411) fix(metrics): extend LLM latency histogram buckets (NVIDIA-NeMo#385) feat(server): forward inbound Anthropic auth (NVIDIA-NeMo#372) fix(protocol): normalize nested metadata strings (NVIDIA-NeMo#422) Revert "ci: base full CI on changed paths (NVIDIA-NeMo#403)" (NVIDIA-NeMo#414) feat: Move `Decision::reasoning` to a log message (NVIDIA-NeMo#413) ... Co-authored-by: Michael Neale <michael.neale@gmail.com> Signed-off-by: Michael Neale <michael.neale@gmail.com> # Conflicts: # crates/switchyard-server/src/config.rs
fix: content-filter responses raise instead of normalizing
Summary
_response()raisedValueError("LiteLLM returned no text content")for anyresponse with empty
content, including content-filter responses where emptyoutput is expected. This caused legitimate content-filter results to fail
instead of being normalized.
Root cause
The guard at the end of
_response()did not distinguish between a trulyempty/invalid response and a
content_filterfinish reason, which by designreturns no content.
Fix
Allow empty
contentwhen the finish reason iscontent_filter; keep theexisting raise for all other empty-content cases so unexpected responses still
fail fast.
Testing
Added
test_response_content_filter_empty_contenttoexamples/experimental/litellm/tests/test_client.pyand verified the fullnon-e2e suite passes:
Contributor guidelines
Signed-off-by: andrewwhitecdw andrewwhitecdw@users.noreply.github.com
Summary by CodeRabbit
Bug Fixes
Tests