Skip to content

Conversation

@RayenTian
Copy link
Contributor

@RayenTian RayenTian commented Feb 5, 2026

Summary

  • Add a shared register_omegaconf_resolvers() helper in nemo_rl/utils/config.py.
  • Replace ad-hoc resolver registration in entry scripts with the shared helper.
  • Apply the same helper in config validation tests and the template script.

Related issue

Summary by CodeRabbit

Release Notes

  • Refactor

    • Centralized configuration resolver registration across example scripts and tests for improved consistency and maintainability.
  • Tests

    • Updated test setup to use centralized resolver registration.

@RayenTian RayenTian added the CI:L1 Run doctests, unit tests, and functional tests label Feb 5, 2026
@RayenTian RayenTian changed the title chore:Centralize OmegaConf resolver registration chore: Centralize OmegaConf resolver registration Feb 5, 2026
@RayenTian RayenTian marked this pull request as ready for review February 9, 2026 02:07
@RayenTian RayenTian requested review from a team and terrykong as code owners February 9, 2026 02:07
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 9, 2026

📝 Walkthrough

Walkthrough

A new register_omegaconf_resolvers() function was added to nemo_rl/utils/config.py to centralize OmegaConf resolver registration. All example scripts, research templates, and tests were updated to import and call this function at startup, replacing inline OmegaConf.register_new_resolver() calls.

Changes

Cohort / File(s) Summary
Centralized Resolver Registration
nemo_rl/utils/config.py
Added new public function register_omegaconf_resolvers() that registers "mul" and "max" OmegaConf resolvers if not already present, centralizing resolver setup logic.
Example Scripts
examples/run_distillation.py, examples/run_grpo.py, examples/run_grpo_sliding_puzzle.py, examples/run_sft.py, examples/run_vlm_grpo.py, examples/nemo_gym/run_grpo_nemo_gym.py
Updated to import register_omegaconf_resolvers() and call it in main() before argument parsing. Removed inline OmegaConf.register_new_resolver() calls.
Research Template
research/template_project/single_update.py
Updated to import and call register_omegaconf_resolvers() in __main__ before parsing arguments; removed inline resolver registration.
Test Updates
tests/unit/test_config_validation.py
Refactored to use centralized register_omegaconf_resolvers() in test setup instead of manual resolver registration with conditional checks.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 6
✅ Passed checks (6 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title 'chore: Centralize OmegaConf resolver registration' accurately and concisely summarizes the main change: consolidating OmegaConf resolver registration into a shared helper function.
Linked Issues check ✅ Passed The PR successfully implements objective 5 from issue #1552: moving customized OmegaConf operations into a common/shared location by creating register_omegaconf_resolvers() and replacing all ad-hoc registrations across entry scripts and tests.
Out of Scope Changes check ✅ Passed All changes are directly scoped to centralizing OmegaConf resolver registration. No unrelated modifications were introduced; the PR focuses solely on the stated objective without addressing items 1-4 or 6 from issue #1552.
Docstring Coverage ✅ Passed Docstring coverage is 93.75% which is sufficient. The required threshold is 80.00%.
Test Results For Major Changes ✅ Passed Minor code consolidation refactoring that centralizes existing OmegaConf resolver registration logic without introducing new features, breaking changes, or affecting numerics or performance.

✏️ 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 ruit/issue_1552_5

No actionable comments were generated in the recent review. 🎉


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.

@RayenTian RayenTian 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
@RayenTian RayenTian 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
@RayenTian RayenTian requested a review from yuki-97 February 9, 2026 06:34
…le example scripts and the config utility. This change enhances configuration management by ensuring necessary resolvers are available for use across different training scripts.

Signed-off-by: ruit <ruit@nvidia.com>
@RayenTian RayenTian 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
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.

Refactor dataset part to use the new dataset and processor interface

2 participants