Skip to content

Conversation

@kevalmorabia97
Copy link
Collaborator

@kevalmorabia97 kevalmorabia97 commented Oct 8, 2025

What does this PR do?

Summary by CodeRabbit

  • Style

    • Clarified a runtime warning to state certain Transformers versions are “not tested with nvidia-modelopt” instead of “incompatible.”
  • Tests

    • Expanded example test matrix to include quantization-aware training.
    • Removed the special-case allowed failure for that example.
    • Standardized parameter-sorting in tests via a shared utility for consistency.
  • Documentation

    • Added an "Advanced: Adding a new NAS Algorithm" section with guidance and a template link.
  • Refactor

    • Removed a previously re-exported plugin symbol from the public plugin surface.

@kevalmorabia97 kevalmorabia97 requested review from a team as code owners October 8, 2025 14:10
@kevalmorabia97 kevalmorabia97 requested review from RalphMao and removed request for RalphMao October 8, 2025 14:10
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 8, 2025

Walkthrough

CI matrix updated to include llm_qat and its separate failing job removed; warning text in torch init revised; a test replaced an in-place parameter sort with a shared utility; docs gained an Advanced NAS section; a star-import of megatron_model_config from a plugin init was removed. Public export set changed.

Changes

Cohort / File(s) Summary of Changes
CI pipeline matrix update
.gitlab/tests.yml
Added llm_qat to the example-torch test matrix and removed the separate example-failing block that tested llm_qat.
Warning message text tweak
modelopt/torch/__init__.py
Modified the warning string from “incompatible with nvidia-modelopt” to “not tested with nvidia-modelopt”; no logic or threshold changes.
Plugin export removal
modelopt/torch/opt/plugins/__init__.py
Removed the conditional from .megatron_model_config import * (the star-import) from the megatron plugin initialization, reducing re-exported symbols.
Test refactor to utility function
tests/gpu/torch/nas/plugins/test_megatron_mamba_dynamic_modules.py
Replaced dynamic_space.sort_parameters() with mtn.utils.sort_parameters(model); assertions and test flow remain the same.
Documentation addition
docs/source/guides/7_nas.rst
Appended an “[Advanced] Adding a new NAS/Prune Algorithm” section with references to a template and example pruning algorithm; docs-only change.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant Test as Test (unit)
    participant Model as DynamicSpace / Model
    participant Util as mtn.utils.sort_parameters

    Note over Test,Model: Before -> in-place call (old)
    Test->>Model: dynamic_space.sort_parameters()
    Model-->>Test: parameters sorted

    Note over Test,Util: After -> centralized utility (new)
    Test->>Util: mtn.utils.sort_parameters(model)
    Util-->>Test: parameters sorted
Loading
sequenceDiagram
    autonumber
    participant Importer as plugin __init__
    participant MegatronConfig as megntron_model_config
    participant Plugin as megatron plugin

    Note over Importer,MegatronConfig: Old (before)
    Importer->>MegatronConfig: from .megatron_model_config import *
    MegatronConfig-->>Importer: symbols re-exported

    Note over Importer,Plugin: New (after)
    Importer->>Plugin: plugin init without star-import
    Plugin-->>Importer: no direct re-exports from megatron_model_config
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

I twitch my whiskers at CI's new beat,
QAT hops in, no failing seat.
A gentler warning, calmly said,
Params sorted now by a shared thread.
I twitch, I nibble—changes neat. 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title clearly highlights the primary change—fixing the multi-GPU Mamba Minitron parameter sorting test—and concisely notes ancillary cleanups without extraneous detail, making it specific and easy to understand for reviewers scanning PR history.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ 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 kmorabia/mamba-sort-fix

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.

@codecov
Copy link

codecov bot commented Oct 8, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.79%. Comparing base (e09d36c) to head (4e1067a).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #412   +/-   ##
=======================================
  Coverage   73.79%   73.79%           
=======================================
  Files         171      171           
  Lines       17591    17591           
=======================================
  Hits        12982    12982           
  Misses       4609     4609           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@kevalmorabia97 kevalmorabia97 force-pushed the kmorabia/mamba-sort-fix branch 3 times, most recently from 6ffe2d8 to 4f0057a Compare October 8, 2025 14:40
Signed-off-by: Keval Morabia <[email protected]>
@kevalmorabia97 kevalmorabia97 force-pushed the kmorabia/mamba-sort-fix branch from 4f0057a to 4e1067a Compare October 8, 2025 14:40
@kevalmorabia97 kevalmorabia97 requested a review from a team as a code owner October 8, 2025 14:40
@kevalmorabia97 kevalmorabia97 changed the title Fix multi-gpu mamba param sorting test Fix multi-gpu mamba minitron param sorting test + minor cleanup Oct 8, 2025
@kevalmorabia97 kevalmorabia97 merged commit 3a76d28 into main Oct 8, 2025
27 checks passed
@kevalmorabia97 kevalmorabia97 deleted the kmorabia/mamba-sort-fix branch October 8, 2025 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants