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 (6)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughGDPVal evaluation now uses ChangesGDPVal launcher integration
Estimated code review effort: 2 (Simple) | ~10 minutes Mergeability Score: ⚪ Minimal · up to The PR makes GDPVal launches use the exact 0.2.6 evaluator launcher and updates the related documentation and tests; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1📝 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.
There was a problem hiding this comment.
Addressed in 938abd2. NEL_GDPVAL_SPEC is now a hard-coded readonly nemo-evaluator-launcher[all]==0.2.6, so values sourced from .env cannot bypass the correctness pin. I also added a hermetic wrapper test that puts a stub uvx on PATH, injects a conflicting NEL_GDPVAL_SPEC=...0.2.4, and asserts that the wrapper still forwards the exact 0.2.6 spec. The focused skill pytest and pre-commit checks pass.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| config has a `pre_cmd`). Thinking mode is mandatory (non-thinking loses ~86%). | ||
| 4. Dry-run → launch. **`limit_samples` is inert on the gym path** (the gym runs all | ||
| 4. Run both dry-run and launch through `"$SKILL_DIR/scripts/nel-gdpval.sh"`; it pins | ||
| the required 0.2.6 launcher and avoids older launchers' unset |
There was a problem hiding this comment.
Why did agent install an outdated version of nel in the first place? Does newer than 0.2.6 work?
There was a problem hiding this comment.
Good question. The failing GDPVal attempt did not deliberately install 0.2.4; it reused an existing base-environment nel 0.2.4 because the pre-existing Step 1 only checked whether nel was present. I verified on the PR base commit that SKILL.md already identifies nemo-evaluator-launcher 0.2.6 as the default for Steps 1–9 and explicitly places GDPVal on that launcher. As of this review, 0.2.6 is the latest public nemo-evaluator-launcher release, so there is no newer public launcher version to test. The nemo-evaluator 0.3.x/0.4.x nel-next workflow is a separate package, CLI, and schema rather than a newer compatible launcher. I updated the docs to name the observed failing version precisely and added the design comparison to the PR description.
Signed-off-by: Zhiyu Cheng <zhiyuc@nvidia.com>
Signed-off-by: Zhiyu Cheng <zhiyuc@nvidia.com>
|
4 participants
What does this PR do?
Type of change: Bug fix
Pins GDPVal evaluation commands to the currently validated
nemo-evaluator-launcher==0.2.6through a dedicateduvxwrapper and updates theGDPVal skill, recipe, example, and reference documentation to use it. The reference
also documents how to validate and adopt a future launcher release without mixing
harness versions in baseline-versus-candidate comparisons.
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.uvx; a conflicting ambientconflicting ambient version/spec values still invoke the validated 0.2.6 package.
python -m pytest plugins/modelopt/skills/evaluation/ -o addopts="" -p no:cacheprovider -vBefore 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/Astubs
uvxand verifies the exact package spec despite a conflicting environment value.Additional Information
The fix preserves a stable GDPVal cache namespace across Slurm walltime resumes;
using
SLURM_JOB_IDwould not.The failing run reused an existing base-environment
nel0.2.4 because thepre-existing prerequisite checked only whether
nelwas installed. The skillalready identified 0.2.6 as the default launcher for Steps 1–9 and GDPVal. Version
0.2.6 is also the latest public
nemo-evaluator-launcherrelease, so there is nonewer public launcher version to validate.
A dedicated
uvxwrapper keeps the fix narrow and deterministic:nel-next.shmanages the separatenemo-evaluator0.4.x package, CLI, andschema; generalizing it would couple incompatible workflows.
while
uvx --from ...==0.2.6already provides an isolated cached environment.mutating it; the wrapper guarantees the known-good launcher on every GDPVal
invocation without changing the existing
nel.Summary by CodeRabbit
New Features
Documentation
Tests