Skip to content

Conversation

@yuki-97
Copy link
Contributor

@yuki-97 yuki-97 commented Feb 5, 2026

  1. remove tests/functional/grpo_math_env.sh since duplicated with tests/functional/grpo.sh.
  2. add missing functional tests:
    1. tests/functional/dpo_megatron.sh
    2. tests/functional/grpo_rm_env.sh (this one is disabled since it's currently broken in CI, but it works locally)
    3. tests/functional/sft_megatron.sh
    4. tests/functional/test_converters.sh (this one is disabled since DTensor v2 converter has some issue now and fix: Fix DCP-to-HF conversion for model-wrapped checkpoints #1881 is fixing it)
  3. add unit test to prevent.

Summary by CodeRabbit

  • Tests
    • Reorganized functional GPU test suite with expanded coverage including new distillation, DPO, and evaluation tests.
    • Removed one redundant test script to streamline test execution.
    • Updated test configurations for improved stability and accuracy.
    • Added validation to ensure all functional tests are properly tracked.

@yuki-97 yuki-97 requested a review from a team as a code owner February 5, 2026 16:10
@yuki-97 yuki-97 added the CI:L1 Run doctests, unit tests, and functional tests label Feb 5, 2026
@yuki-97 yuki-97 requested a review from terrykong February 5, 2026 16:11
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 5, 2026

📝 Walkthrough

Walkthrough

The PR reorders and expands the GPU functional test suite by replacing and adding test invocations, updates a Megatron DPO configuration file with a v2 config path and additional flags, removes the grpo_math_env.sh test script, and adds a unit test to ensure all functional test scripts are referenced in the main test orchestration script.

Changes

Cohort / File(s) Summary
Functional GPU Test Suite
tests/functional/L1_Functional_Tests_GPU.sh, tests/functional/dpo_megatron.sh
Reordered and expanded functional test invocations; replaced SFT-based tests with distillation, DPO, and evaluation tests earlier in suite. Updated Megatron DPO config from v1 to v2 with sequence parallel flag; increased loss threshold from 5 to 6.
Removed Tests
tests/functional/grpo_math_env.sh
Deleted entire functional test script for GRPO math environment setup and execution.
Test Coverage Validation
tests/unit/test_recipes_and_test_suites.py
Added new unit test function to verify all functional test scripts in tests/functional/ are referenced in the main L1_Functional_Tests_GPU.sh orchestrator.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested labels

CI:L1

Suggested reviewers

  • terrykong
  • yaoyu-33
  • chtruong814
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix: add missing functional test' is partially related to the changeset. It refers to adding missing functional tests, which is a real aspect of the change, but it's somewhat incomplete—the PR also involves removing a duplicate test (grpo_math_env.sh) and reordering/reorganizing the test suite.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Test Results For Major Changes ✅ Passed PR contains minor test suite maintenance changes including removing duplicates, adding missing tests, and reorganizing test execution without major product changes.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

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

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch yukih/functional-test

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.

terrykong
terrykong previously approved these changes Feb 5, 2026
@terrykong terrykong enabled auto-merge (squash) February 5, 2026 17:34
@yuki-97 yuki-97 added CI:L1 Run doctests, unit tests, and functional tests and removed CI:L1 Run doctests, unit tests, and functional tests labels Feb 6, 2026
@yuki-97 yuki-97 requested a review from terrykong February 6, 2026 04:36
terrykong
terrykong previously approved these changes Feb 6, 2026
@yuki-97 yuki-97 force-pushed the yukih/functional-test branch from 1575a0a to 9550f8c Compare February 9, 2026 02:59
@yuki-97 yuki-97 added CI:L1 Run doctests, unit tests, and functional tests and removed CI:L1 Run doctests, unit tests, and functional tests labels Feb 9, 2026
@yuki-97 yuki-97 added CI:L1 Run doctests, unit tests, and functional tests and removed CI:L1 Run doctests, unit tests, and functional tests labels Feb 9, 2026
Signed-off-by: Yuki Huang <yukih@nvidia.com>
Signed-off-by: Yuki Huang <yukih@nvidia.com>
@yuki-97 yuki-97 force-pushed the yukih/functional-test branch from a8e7037 to fba72bb Compare February 10, 2026 02:59
@yuki-97 yuki-97 added CI:L1 Run doctests, unit tests, and functional tests and removed CI:L1 Run doctests, unit tests, and functional tests labels Feb 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI:L1 Run doctests, unit tests, and functional tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants