-
Notifications
You must be signed in to change notification settings - Fork 609
fix: Update pytest markers for deepep test. #2561
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
/ok to test 4931f08 |
WalkthroughUpdates the vLLM launch script’s --model help text to display the default via ${MODEL}. Adjusts test metadata by adding the nightly mark to the deepep vLLM test configuration. No changes to parsing logic, defaults, or runtime behavior. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
components/backends/vllm/launch/dsr1_dep.sh (1)
50-51
: Help text now reflects the actual default via ${MODEL} — nice quality-of-life improvement.Optional consistency tweaks:
- Allow env override for the default model so
MODEL=... ./dsr1_dep.sh -h
shows the caller’s intended default without editing the script.- Mirror this dynamic pattern for
--master-addr
and--log-dir
as well.Outside the changed lines, consider:
- At defaults section (current Line 13):
# before MODEL="deepseek-ai/DeepSeek-R1" # after (allows env to override) MODEL="${MODEL:-deepseek-ai/DeepSeek-R1}"
- In help text for other options (near Lines 48–49):
echo " --master-addr ADDR Master node address (default: ${MASTER_ADDR})" echo " --log-dir DIR Directory for log files (default: ${LOG_DIR})"I can push a follow-up commit if you’d like these refinements included now.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
components/backends/vllm/launch/dsr1_dep.sh
(1 hunks)tests/serve/test_vllm.py
(1 hunks)
🔇 Additional comments (1)
tests/serve/test_vllm.py (1)
267-272
: Nightly marker is registered and excluded by CI
pyproject.toml
(around line 164) lists"nightly: marks tests to run nightly"
under themarkers
setting, so noPytestUnknownMarkWarning
will be raised.- The GitHub Actions workflow in
.github/workflows/build-and-test.yml
invokes pytest with-m "pre_merge or mypy"
, which inherently omits any tests markednightly
.No further changes are needed—the heavy H100 tests remain gated to nightly runs as intended.
/ok to test badba57 |
/ok to test 7c8bf7e |
Signed-off-by: Hannah Zhang <[email protected]>
Signed-off-by: Krishnan Prashanth <[email protected]>
Signed-off-by: nnshah1 <[email protected]>
Overview:
Updated the pytest markers so that the deepep test will not run the kvbm test in nightly.
Details:
Where should the reviewer start?
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit
Documentation
Tests