fix(llm): preserve all vLLM stream chunks - #2234
Conversation
🤖 Open Code ReviewTarget: PR #2234 🔍 OpenCodeReview found 3 issue(s) in this PR. 1.
|
✅ Automated Test Results: PASSEDAll tests passed (3/3 executed). memos_python_core/changed-repo-python: 3/3. Duration: 5s [advisory, non-gating] AI-generated tests on branch test/auto-gen-d68ac4a0e84e54cc-20260810165338: 34/34 passed — these do NOT affect the PR verdict; review the branch manually. Branch: |
|
Good catch — added both cases in 9ee2711: reasoning plus content with tags disabled, and a reasoning-only stream to make sure no closing tag leaks. The focused file is now 4/4, and the LLM suite (excluding the optional torch test) is 18/18. |
✅ Automated Test Results: PASSEDAll tests passed (4/4 executed). memos_python_core/changed-repo-python: 4/4. Duration: 5s [advisory, non-gating] AI-generated tests on branch test/auto-gen-8a14dde71c250f5a-20260810170421: 37/37 passed — these do NOT affect the PR verdict; review the branch manually. Branch: |
|
I double-checked the pushed SHA: the reasoning check on line 183 and the content check on line 189 both have 16 spaces, so they are already sibling branches. The first regression test also sends three content-only chunks after a separate reasoning chunk and asserts that all three are yielded; the new autotest run passed 4/4. This looks like an OCR false positive, so I'm leaving the implementation unchanged. |
✅ Automated Test Results: PASSEDAll tests passed (4/4 executed). memos_python_core/changed-repo-python: 4/4. Duration: 7s [advisory, non-gating] AI-generated tests on branch test/auto-gen-b3f6befecbe91872-20260813172203: 32/33 passed, 1 failed — these do NOT affect the PR verdict; review the branch manually. Branch: |
Description
VLLMLLM.generate_stream()handleddelta.contentafter the stream loop, so it only saw the final delta. Responses split across several content chunks were truncated, and an empty or choices-free stream could raiseUnboundLocalErrorbecausedeltawas never assigned.This moves content handling into the loop and closes an open
<think>block when a reasoning-only stream ends. It keeps the existingremove_think_prefixbehavior unchanged and adds focused regression coverage for multi-chunk content, empty/choices-free streams, reasoning-only streams, and output with think tags disabled.No dependencies are added. I couldn't find an existing issue or open PR for this path.
Related Issue (Required): N/A
Type of change
How Has This Been Tested?
Before the fix, the new regression file produced 2 failures and 2 errors. On the pushed commit:
uv run --frozen --with 'pytest==8.3.5' pytest tests/llms/test_vllm.py -q— 4 passeduv run --frozen --with 'pytest==8.3.5' pytest tests/llms/ --ignore=tests/llms/test_hf.py -q— 18 passed (4 existing warnings)uv run --frozen --with 'poetry>=2,<3' --with 'ruff==0.11.13' make format— Ruff passed; 617 files unchangedgit diff --check origin/dev-v2.0.29...HEAD— passedThe complete
tests/llms/collection was not available locally becausetest_hf.pyimports the optionaltorchdependency.Checklist
@WeiminLee when you have a moment, could you take a look?
Reviewer Checklist