Skip to content

fix(vllm-tests): skip omni test collection when vllm_omni is missing - #9406

Closed
krishung5 wants to merge 1 commit into
mainfrom
krish/fix-ci-omni-cpu-collect
Closed

fix(vllm-tests): skip omni test collection when vllm_omni is missing#9406
krishung5 wants to merge 1 commit into
mainfrom
krish/fix-ci-omni-cpu-collect

Conversation

@krishung5

@krishung5 krishung5 commented May 12, 2026

Copy link
Copy Markdown
Contributor

Summary

  • The sample-runtime / Unified Test cuda12.9, amd64 CI job (.github/workflows/pr.yaml:479) has been failing on every PR that touches lib/** or other paths that trigger changed-files.outputs.core. The job runs pytest -m "pre_merge and gpu_0 and unified" on a CPU-only runner with no test-path filter, so pytest walks the whole repo and imports every test_*.py it finds before filtering by marker.
  • components/src/dynamo/vllm/tests/omni/test_*.py files transitively import vllm_omni at module-load time. vllm_omni isn't installed in the sample-runtime image (and on CPU-only platforms the import can raise something other than ImportError), so the per-file try/except ImportError: pytest.skip(...) guard inside each test file doesn't always catch it.
  • The repo already has a pytest_ignore_collect guard in components/src/dynamo/vllm/tests/conftest.py that skips test_vllm_*.py files when vllm isn't importable. This PR extends that same hook to also skip any file under an omni/ directory when vllm_omni isn't importable.

Test plan

  • CI sample-runtime / Unified Test cuda12.9, amd64 job passes on this PR (it has been failing on PRs that touch core-changed files)
  • No change in behavior on runners that DO have vllm_omni installed — omni tests still get collected and run

🤖 Generated with Claude Code


Open in Devin Review

Summary by CodeRabbit

  • Tests
    • Improved test collection logic to conditionally skip optional component tests when required dependencies are unavailable.

Review Change Stack

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:26
@github-actions github-actions Bot added fix backend::vllm Relates to the vllm backend labels May 12, 2026
@coderabbitai

coderabbitai Bot commented May 12, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 78e16d08-1d75-4f64-b44b-949802813a61

📥 Commits

Reviewing files that changed from the base of the PR and between e2f63d0 and 43c87a5.

📒 Files selected for processing (1)
  • components/src/dynamo/vllm/tests/conftest.py

Walkthrough

The pytest_ignore_collect hook in the test configuration now skips omni test files when the vllm_omni module is unavailable, extending the existing pattern used for test_vllm_* files.

Changes

Test Collection Skip Logic

Layer / File(s) Summary
Omni Test Skip Condition
components/src/dynamo/vllm/tests/conftest.py
pytest_ignore_collect adds a conditional branch that skips files containing an omni path segment and starting with test_ when vllm_omni import fails.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: extending the test collection skip logic to handle omni tests when vllm_omni is missing.
Description check ✅ Passed The description provides comprehensive context including the problem statement, existing behavior, the proposed solution, and a clear test plan, covering all required template sections.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
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.

✏️ 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.

@krishung5

Copy link
Copy Markdown
Contributor Author

Folded into #9407 — single PR is easier to land since both fixes are needed for main to build + sample-runtime to pass.

@krishung5 krishung5 closed this May 12, 2026
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 size/XS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant