Pin GDPVal evaluator launcher to 0.2.6 - #2188
Conversation
Signed-off-by: Zhiyu Cheng <zhiyuc@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughGDPVal evaluation now uses a strict ChangesGDPVal launcher integration
Estimated code review effort: 2 (Simple) | ~10 minutes Mergeability Score: ⚪ Minimal · up to This change pins GDPVal evaluations to the corrected launcher version and updates the related usage documentation; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2188 +/- ##
=======================================
Coverage 67.09% 67.09%
=======================================
Files 522 522
Lines 60461 60461
=======================================
Hits 40567 40567
Misses 19894 19894
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
cjluo-nv
left a comment
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review (gpt-5.6-sol) — DM the bot to share feedback.
The GDPVal failure is clear: launcher versions older than 0.2.6 can generate a Slurm script that re-exports NEL_INVOCATION_ID before assigning it. The proposed solution is a dedicated uvx launcher wrapper. Existing alternatives include extending/generalizing the repository's existing scripts/nel-next.sh isolation wrapper, using a project/skill-level uv lock and uv run, or retaining the normal launcher with an exact-version preflight. The PR body explains why 0.2.6 is required, but does not compare the dedicated second wrapper with those alternatives, so the required design gate is not yet satisfied.
There is also a correctness hole in the pin itself: an ambient or sourced NEL_GDPVAL_SPEC silently bypasses 0.2.6, including selecting the older launcher this change is meant to prevent. Please make the required version non-overridable (or strictly validate the override) and add an automated wrapper test that stubs uvx and verifies the exact install spec even with a conflicting environment. The new file's NVIDIA Apache-2.0 header matches LICENSE_HEADER, so it does not require separate licensing escalation.
| # launchers can instead fail under `set -u` before the evaluation client starts. | ||
| set -euo pipefail | ||
|
|
||
| NEL_GDPVAL_SPEC="${NEL_GDPVAL_SPEC:-nemo-evaluator-launcher[all]==0.2.6}" |
There was a problem hiding this comment.
Bot comment.
The wrapper and docs promise that GDPVal is pinned to 0.2.6 for correctness, but this ambient-env override silently defeats that guarantee. In particular, set -a; source .env is part of the documented run flow, so a stale NEL_GDPVAL_SPEC can select exactly the older launcher that produces the unbound-variable failure. Unlike the documented escape hatch in nel-next.sh, there does not seem to be a valid compatibility path here because the generated-script behavior specifically requires 0.2.6. Please hard-code the spec (ideally readonly), or reject anything that is not the exact required release. Please also cover this with a shell test using a stub uvx, including a conflicting ambient value, since the exact pin is the core behavior of this fix.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
3 participants
What does this PR do?
Type of change: Bug fix
Pins GDPVal evaluation commands to
nemo-evaluator-launcher==0.2.6through adedicated
uvxwrapper and updates the GDPVal skill, recipe, example, and referencedocumentation to use it.
Older launchers generate a runtime re-export for
NEL_INVOCATION_IDwithout firstassigning the launcher-created invocation ID. The resulting Slurm script exits under
set -uwithNEL_INVOCATION_ID: unbound variablebefore the evaluation clientstarts. Launcher 0.2.6 writes the stable invocation ID into the script before the
re-export, including for walltime-resume chains.
Usage
Testing
bash -n plugins/modelopt/skills/evaluation/scripts/nel-gdpval.shnemo_evaluator_launcher: 0.2.6.NEL_INVOCATION_ID="<invocation-id>"before the runtime re-export.Before your PR is "Ready for review"
Make sure you read and follow Contributor guidelines
and your commits are signed (
git commit -s -S).Make sure you read and follow the Security Best Practices.
follow guidance in
CONTRIBUTING.md: N/Aversion, generated Slurm ordering, and pre-commit were validated.
Additional Information
The fix preserves a stable GDPVal cache namespace across Slurm walltime resumes;
using
SLURM_JOB_IDwould not.Summary by CodeRabbit
New Features
Documentation