Skip to content

fix(ci): unblock main build (stop_reason refs + omni-test CPU collection) - #9407

Merged
krishung5 merged 4 commits into
mainfrom
krish/fix-main-stop-reason-build
May 12, 2026
Merged

fix(ci): unblock main build (stop_reason refs + omni-test CPU collection)#9407
krishung5 merged 4 commits into
mainfrom
krish/fix-main-stop-reason-build

Conversation

@krishung5

@krishung5 krishung5 commented May 12, 2026

Copy link
Copy Markdown
Contributor

Summary

1. Drop dead stop_reason refs (f804b56255)

Main is currently failing the dynamo-llm build with error[E0609]: no field stop_reason on type &mut ChatChoiceStream at lib/llm/src/preprocessor.rs:1581. ChatChoiceStream (lib/protocols/src/types/chat.rs:813) only has index, delta, finish_reason, logprobs — no stop_reason.

PR #9058 (f6117b82ec, landed earlier today) introduced two dead assignments to a field that was already removed in PR #8119.

2. Skip omni test collection on runners without vllm_omni (9e086c21c0)

sample-runtime / Unified Test cuda12.9, amd64 runs pytest -m "pre_merge and gpu_0 and unified" on a CPU-only runner with no path filter, so pytest walks the whole repo and imports every test_*.py before applying marker filters. components/src/dynamo/vllm/tests/omni/test_*.py transitively imports vllm_omni at module-load time; on platforms vllm_omni doesn't support, that raises something other than ImportError, so the per-file try/except ImportError: pytest.skip(...) doesn't catch it.

Extends the existing pytest_ignore_collect hook in components/src/dynamo/vllm/tests/conftest.py (which already skips test_vllm_*.py when vllm is unavailable) to also skip files under an omni/ directory when vllm_omni is unavailable.

Test plan

  • vllm-runtime / Build multi-arch cuda12.9 and dynamo-runtime / image / Build multi-arch cuda12.9 go green
  • sample-runtime / Unified Test cuda12.9, amd64 goes green
  • No change in behavior on runners that DO have vllm_omni installed — omni tests still get collected and run

🤖 Generated with Claude Code

ChatChoiceStream has no stop_reason field (lib/protocols/src/types/chat.rs:813).
PR #9058 (f6117b8, landed today) re-introduced two assignments that
were already removed by PR #8119, breaking dynamo-llm compilation on
main and blocking every PR's build.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@krishung5
krishung5 requested a review from a team May 12, 2026 00:33
@github-actions github-actions Bot added frontend `python -m dynamo.frontend` and `dynamo-run in=http|text|grpc` fix labels May 12, 2026
@coderabbitai

coderabbitai Bot commented May 12, 2026

Copy link
Copy Markdown
Contributor

Walkthrough

This PR adjusts streaming choice data handling during the end-of-stream flush operation. The preprocessor now clears reasoning-related delta fields instead of stop_reason, and the test helper is updated to align with this field-handling change.

Changes

Stream Flush Behavior for Reasoning Fields

Layer / File(s) Summary
Flush path refusal and reasoning field clearing
lib/llm/src/preprocessor.rs
In the end-of-stream flush branch of strip_leading_reasoning_start_from_stream, the emitted buffered choice now explicitly clears choice.delta.refusal and choice.delta.reasoning_content instead of clearing choice.stop_reason.
Test helper alignment
lib/llm/tests/postprocessor_parsing_stream.rs
The mock_multi_choice_content_chunk helper no longer initializes the stop_reason field on ChatChoiceStream to reflect the change in how stop_reason is handled during flush operations.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Title check ⚠️ Warning The title mentions 'omni-test CPU collection' but the raw summary shows only stop_reason fixes; the second change (pytest collection) is not reflected in changed files. Update title to accurately reflect only the stop_reason fix, or provide context for the omni-test change mentioned in description.
✅ Passed checks (4 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The PR description is comprehensive and well-structured, clearly explaining the problem, root cause, and proposed fix.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

The sample-unified-test CPU runner imports test_*.py files in the
vllm/omni/ tree during pytest collection, which triggers a vllm_omni
import that fails on platforms vllm_omni doesn't support. Extend the
existing pytest_ignore_collect guard in vllm/tests/conftest.py to
also cover omni/ files when vllm_omni isn't importable.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@krishung5
krishung5 requested review from a team as code owners May 12, 2026 00:48
@github-actions github-actions Bot added the backend::vllm Relates to the vllm backend label May 12, 2026
@krishung5 krishung5 changed the title fix(llm): drop dead stop_reason refs unblocking main build fix(ci): unblock main build (stop_reason refs + omni-test CPU collection) May 12, 2026

@Aphoh Aphoh left a comment

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.

LGTM

@krishung5
krishung5 enabled auto-merge (squash) May 12, 2026 00:57
importlib.util.find_spec("vllm_omni") is insufficient — vllm_omni IS
installed on sample-runtime CPU runners. The real failure is deep in
the import chain (vllm._C → libcuda.so.1) and surfaces as
NotImplementedError, not ImportError. Attempt the actual import once,
cache the result, and use that to decide collection.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
tests/serve/test_vllm_omni.py was already attempting a module-level
skip when omni isn't available, but only caught ImportError. On CPU-only
sample-runtime runners the chain reaches vllm._C → libcuda.so.1 and
raises NotImplementedError, slipping past the guard and surfacing as a
collection error. Broaden to catch any exception.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@krishung5
krishung5 merged commit 0afd149 into main May 12, 2026
106 checks passed
@krishung5
krishung5 deleted the krish/fix-main-stop-reason-build branch May 12, 2026 02:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend::vllm Relates to the vllm backend fix frontend `python -m dynamo.frontend` and `dynamo-run in=http|text|grpc` size/M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants