Skip to content

test: fix bug in deselection and make fast tests even faster#2038

Merged
yuki-97 merged 1 commit intomainfrom
tk/smaller-test-set
Feb 28, 2026
Merged

test: fix bug in deselection and make fast tests even faster#2038
yuki-97 merged 1 commit intomainfrom
tk/smaller-test-set

Conversation

@terrykong
Copy link
Copy Markdown
Collaborator

@terrykong terrykong commented Feb 28, 2026

What does this PR do ?

Add a one line overview of what this PR aims to accomplish.

Issues

List issues that this PR closes (syntax):

Usage

  • You can potentially add a usage example below
# Add a code snippet demonstrating how to use this

Before your PR is "Ready for review"

Pre checks:

  • Make sure you read and followed Contributor guidelines
  • Did you write any new necessary tests?
  • Did you run the unit tests and functional tests locally? Visit our Testing Guide for how to run tests
  • Did you add or update any necessary documentation? Visit our Document Development Guide for how to write, build and test the docs.

Additional Information

  • ...

Summary by CodeRabbit

  • Tests
    • Modified GPU functional test execution modes and expanded test suite with additional test variants
    • Reduced configuration parameters for distillation and GRPO tests to optimize testing efficiency
    • Updated unit test exclusion paths and expanded exclusion coverage across multiple test categories

Signed-off-by: Terry Kong <terryk@nvidia.com>
@terrykong terrykong requested a review from yuki-97 February 28, 2026 08:55
@terrykong terrykong requested a review from a team as a code owner February 28, 2026 08:55
@terrykong terrykong added the CI:Lfast Runs a fast test suite and re-use nightly `main` container (but sync dependencies to PRs version) label Feb 28, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Feb 28, 2026

📝 Walkthrough

Walkthrough

This PR modifies test infrastructure files to adjust test configurations and expand coverage. Changes include removing fast-mode flags from specific tests, reducing batch sizes and sequence lengths in distillation and GRPO configurations, updating unit test exclusion path references, and expanding excluded test variants for improved test suite management.

Changes

Cohort / File(s) Summary
GPU Functional Test Suite
tests/functional/L1_Functional_Tests_GPU.sh
Removed fast-mode execution for test_frozen_env.sh and rm.sh tests; expanded test matrix with additional test invocations increasing overall test coverage.
Functional Test Configurations
tests/functional/distillation.sh, tests/functional/grpo_megatron_async.sh
Reduced training/validation batch sizes and sequence lengths in distillation configuration; reduced maximum training steps in GRPO Megatron async configuration; added validation split parameter to distillation.
Unit Test Exclusions
tests/unit/excluded_unit_tests.sh
Updated test exclusion paths from unit/... to tests/unit/... format; expanded exclusion blocks for vLLM, DTensor, Megatron, and SGLang test variants with comprehensive v1/v2 and configuration-specific deselections.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested labels

CI, CI:Lfast, Run CICD

Suggested reviewers

  • yuki-97
  • yaoyu-33
  • chtruong814
🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Test Results For Major Changes ⚠️ Warning PR claims performance improvements but lacks documented test results, before-and-after timing measurements, or concrete wall-clock deltas as previously requested. Complete PR description with before-and-after test execution times, hardware environment details, and commands used for measurements to validate the speedup claim.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the two main objectives of the PR: fixing a bug in test deselection (reflected in excluded_unit_tests.sh path corrections) and optimizing fast tests by reducing parameters and removing the fast flag from certain tests.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch tk/smaller-test-set

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.


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.

Copy link
Copy Markdown
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: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
tests/unit/excluded_unit_tests.sh (1)

24-253: ⚠️ Potential issue | 🔴 Critical

Fix prefix mismatch in --deselect entries: change tests/unit/ to unit/.

All --deselect entries use the incorrect prefix tests/unit/... but pytest is invoked from the tests/ directory (which becomes the rootdir), expecting node IDs like unit/.... This causes deselection to silently fail, allowing slow tests to run in FAST mode.

The --ignore entries correctly use the unit/... prefix. Change all --deselect entries from tests/unit/ to unit/ to match the pytest rootdir context.

Example fix
--deselect=tests/unit/algorithms/test_grpo.py::test_calculate_rewards_multiple_tasks

should be:

--deselect=unit/algorithms/test_grpo.py::test_calculate_rewards_multiple_tasks
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/unit/excluded_unit_tests.sh` around lines 24 - 253, The --deselect node
IDs use the wrong prefix "tests/unit/"; update every --deselect entry to use
"unit/" instead (e.g. change
--deselect=tests/unit/algorithms/test_grpo.py::test_calculate_rewards_multiple_tasks
to
--deselect=unit/algorithms/test_grpo.py::test_calculate_rewards_multiple_tasks)
so pytest rootdir-based node IDs match and deselection actually works; leave the
existing --ignore entries as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/functional/L1_Functional_Tests_GPU.sh`:
- Line 39: This change moves tests out of FAST mode and the PR must include
concrete before-and-after wall-clock timings for FAST-suite runs; run the
FAST-suite target (the test invoked by run_test in
tests/functional/L1_Functional_Tests_GPU.sh and the analogous invocation near
line 61) under the original FAST configuration and the new configuration,
capture wall-clock durations using a reliable timer (e.g., /usr/bin/time -v or
bash time) for each run, and add those numbers plus exact commands, environment
variables, commit hashes, runner/CI machine details, and any relevant FAST-suite
flags into the PR description so the performance delta is auditable.

---

Outside diff comments:
In `@tests/unit/excluded_unit_tests.sh`:
- Around line 24-253: The --deselect node IDs use the wrong prefix
"tests/unit/"; update every --deselect entry to use "unit/" instead (e.g. change
--deselect=tests/unit/algorithms/test_grpo.py::test_calculate_rewards_multiple_tasks
to
--deselect=unit/algorithms/test_grpo.py::test_calculate_rewards_multiple_tasks)
so pytest rootdir-based node IDs match and deselection actually works; leave the
existing --ignore entries as-is.

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4a7aa47 and d2eebea.

📒 Files selected for processing (4)
  • tests/functional/L1_Functional_Tests_GPU.sh
  • tests/functional/distillation.sh
  • tests/functional/grpo_megatron_async.sh
  • tests/unit/excluded_unit_tests.sh

Copy link
Copy Markdown
Contributor

@yuki-97 yuki-97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool! faster so much. thanks @terrykong !

@yuki-97 yuki-97 enabled auto-merge (squash) February 28, 2026 12:20
@yuki-97 yuki-97 merged commit 42f3043 into main Feb 28, 2026
46 of 47 checks passed
@yuki-97 yuki-97 deleted the tk/smaller-test-set branch February 28, 2026 12:21
seonjinn pushed a commit that referenced this pull request Mar 8, 2026
Signed-off-by: Terry Kong <terryk@nvidia.com>
seonjinn pushed a commit that referenced this pull request Mar 8, 2026
Signed-off-by: Terry Kong <terryk@nvidia.com>
seonjinn pushed a commit that referenced this pull request Mar 9, 2026
Signed-off-by: Terry Kong <terryk@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI:Lfast Runs a fast test suite and re-use nightly `main` container (but sync dependencies to PRs version)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants