fix(llm): enhance fallback logic to handle empty completions and impr…#5491
Conversation
…ove metadata handling
|
I ran into this same bug and verified this patch's approach end-to-end against a live SSE endpoint: a One thing I noticed reviewing the diff:
substantive = (
isinstance(delta, str) and bool(delta)
) or (
event_data.get("type") == "tool_calls"
and bool(event_data.get("calls"))
)But Suggestion — count streamed tool arguments as substantive output too: or event_data.get("type") in ("tool_call_delta", "tool_calls")Pre-patch, |
|
@RonitSachdev You were right about I’ve updated the fallback logic to treat |
|
Hi @alteixeira20 , this PR is ready for review. Could you please take a look when you have time? Please let me know if any changes are needed. Thanks! |
alteixeira20
left a comment
There was a problem hiding this comment.
LGTM.
Verified at exact head cb358b6305fffb3c7aec4f8a73bd1f213b9c3c62.
The empty and metadata-only stream fallback behavior is correctly fixed. Metadata no longer commits a candidate, failed-candidate metadata is discarded, substantive text/reasoning and completed tool calls preserve the selected candidate, and tool_call_delta remains progressively streamed.
The PR merges cleanly into current dev. Compilation, focused fallback tests, the broader LLM core suite, and all effective GitHub checks pass.
No blocking findings.
…ove metadata handling
Summary
Updates
stream_llm_with_fallback()so empty or metadata-only streams correctly move to the next fallback model.A candidate is now accepted only after it produces text, reasoning, or a completed tool call. Buffered metadata is discarded when switching models.
Added regression tests for
[DONE]-only streams, metadata-only streams, valid output, tool calls, and cases where all candidates return empty responses.Target branch
dev, notmain. All PRs land indev;mainis curated by the maintainer at each release. If your PR is onmainby accident, click "Edit" on this PR and change the base.Linked Issue
Fixes #5489
Type of Change
Checklist
devdocker compose uporuvicorn app:app) and verified the change works end-to-end. Type-checks and unit tests are not enough.How to Test
Run the focused fallback tests:
pytest -q tests/test_llm_core_fallback.pyRun the broader LLM core test suite:
pytest -q tests/test_llm_core*.pyVerify the following cases:
[DONE]invokes the fallback.[DONE]invokes the fallback and discards the primary metadata.Visual / UI changes — REQUIRED if you touched anything that renders
Anything that changes what the UI looks like — buttons, icons, padding, colors, fonts, spacing, layout, CSS, HTML, SVG, or any
static/js/module that draws to the DOM — needs all of the following. PRs that change rendering without these WILL be closed.--red,--fg,--bg,--card,--border, etc.) — do not introduce new color values, font sizes, or spacing units.static/index.html) or plain text.Fira Code) for primary UI text. Don't override.Screenshots / clips