Skip to content

Conversation

@oandreeva-nv
Copy link
Contributor

@oandreeva-nv oandreeva-nv commented Aug 21, 2025

Overview:

Details:

Where should the reviewer start?

Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)

  • closes GitHub issue: #xxx

Summary by CodeRabbit

  • Tests

    • Streamlined test marker configuration for KVBM determinism tests to align with current CI capabilities. This affects only how tests are selected in CI and has no impact on product functionality or performance.
  • Chores

    • Added a note to re-enable broader test coverage (e.g., end-to-end, slow, nightly, GPU) once CI support is available, improving future test planning without affecting the user experience.

@copy-pr-bot
Copy link

copy-pr-bot bot commented Aug 21, 2025

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@oandreeva-nv oandreeva-nv changed the title [test] Temporary exclude kvbm tests from vllm, nightly, gpu1... test: Temporary exclude kvbm tests from vllm, nightly, gpu1... Aug 21, 2025
@github-actions github-actions bot added the test label Aug 21, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Aug 21, 2025

Walkthrough

The pytest marker list in tests/kvbm/test_determinism.py was reduced to only kvbm, with e2e, slow, nightly, and gpu_1 markers commented out. A TODO was added to re-enable them when kvbm builds in CI. No test logic or control flow was modified.

Changes

Cohort / File(s) Summary
Test marker adjustments
tests/kvbm/test_determinism.py
Simplified pytestmark to only include kvbm; commented out e2e, slow, nightly, gpu_1; added TODO comment. No changes to test logic.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

Poem

I thump my paw—just kvbm today,
The rest will hop back in when CI’s okay.
Markers tucked snug, in burrows they stay,
Till builds are warm like springtime hay.
For now, one carrot on the tray—
Determinism tested, hip-hip-hooray! 🥕

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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)
tests/kvbm/test_determinism.py (1)

32-32: Polish the TODO for consistency and traceability.

Capitalize TODO/CI and state exactly what to re-enable. Optionally reference a tracking issue.

Apply:

-# Todo: enable the rest when kvbm is built in the ci
+# TODO: Re-enable e2e/slow/nightly/gpu_1 markers when KVBM is built in CI. (track in ai-dynamo/dynamo#xxxx)
📜 Review details

Configuration used: Path: .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.

📥 Commits

Reviewing files that changed from the base of the PR and between ae4fb58 and 9102110.

📒 Files selected for processing (1)
  • tests/kvbm/test_determinism.py (2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Pre Merge Validation of (ai-dynamo/dynamo/refs/pull/2611/merge) by oandreeva-nv.
tests/kvbm/test_determinism.py

[error] 1-1: Black formatting failed. 1 file reformatted (tests/kvbm/test_determinism.py). Run 'black --write tests/kvbm/test_determinism.py' to fix.

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Build and Test - dynamo
🔇 Additional comments (2)
tests/kvbm/test_determinism.py (2)

32-39: Apply Black formatting to tests/kvbm/test_determinism.py

The CI gate is currently failing because this file needs to be reformatted with Black. Running python3 -m black tests/kvbm/test_determinism.py produces the following diff:

--- tests/kvbm/test_determinism.py	2025-08-21 19:13:06.789618+00:00
+++ tests/kvbm/test_determinism.py	2025-08-21 19:20:05.259435+00:00
@@ -1,6 +1,5 @@
-
 #!/usr/bin/env python3
 # SPDX-FileCopyrightText: Copyright (c) 2025 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
 # SPDX-License-Identifier: Apache-2.0
 
@@ -30,14 +29,14 @@
 
 # Test markers to align with repository conventions
 # Todo: enable the rest when kvbm is built in the ci
 pytestmark = [
     pytest.mark.kvbm,
-#    pytest.mark.e2e,
-#    pytest.mark.slow,
-#    pytest.mark.nightly,
-#    pytest.mark.gpu_1,
+    #    pytest.mark.e2e,
+    #    pytest.mark.slow,
+    #    pytest.mark.nightly,
+    #    pytest.mark.gpu_1,
 ]

Please apply and commit these changes to unblock CI:

python3 -m black tests/kvbm/test_determinism.py
git add tests/kvbm/test_determinism.py
git commit -m "chore: black-format tests/kvbm/test_determinism.py (CI gate)"

32-39: The script will reveal how the CI filters are configured and whether there’s an existing pytest config for marker deselection. Once we see the build-and-test job and any pytest.ini/pyproject.toml settings, we can confirm the appropriate skip-if guard usage.

@dmitry-tokarev-nv dmitry-tokarev-nv enabled auto-merge (squash) August 21, 2025 19:23
@dmitry-tokarev-nv dmitry-tokarev-nv merged commit 88d953b into main Aug 21, 2025
7 checks passed
@dmitry-tokarev-nv dmitry-tokarev-nv deleted the oandreeva_disable_test branch August 21, 2025 19:41
hhzhang16 pushed a commit that referenced this pull request Aug 27, 2025
KrishnanPrash pushed a commit that referenced this pull request Sep 2, 2025
nnshah1 pushed a commit that referenced this pull request Sep 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants