[skill] evaluation: add GDPVal (NeMo Gym Stirrup agent) support

[cjluo-nv]

What does this PR do?

Type of change: new feature (agent skill)

Adds GDPVal support to the evaluation agent skill. GDPVal is an agentic AA
benchmark: the NeMo Gym "Stirrup" agent produces office/PDF deliverables inside a
per-task Apptainer code-exec sandbox, and a judge panel scores them. It runs on the
0.2.6 launcher as a nemo_gym task, but it is standalone (one gym eval per
config) and mechanically unlike the aa/ nemo-skills tasks, so it gets its own
branch in the skill rather than being merged into the aa/ task list.

• recipes/tasks/aa_gym/gdpval.md — task recipe: standalone rule, rubric-vs-comparison
  scoring, canary, score extraction.
• references/gym-gdpval.md — the machinery: Apptainer SIF sandbox, the
  _gym_prepare venv-repair / process-group-reap workaround, deployment sizing,
  scoring modes, the MLflow deliverables trap, canary failure modes, and the
  SIF ↔ Gym-version rebuild coupling.
• recipes/examples/gym_gdpval/ — self-contained SLURM + vLLM template plus the
  co-located _gym_prepare.yaml Hydra include (it must travel with the config).
• scripts/gdpval-sif.sh — build-if-absent / reuse-if-present Apptainer SIF helper.
  Builds on the target cluster only (never copies across clusters), flock-guarded and
  atomic, driven by $GDPVAL_SIF_DIR.
• SKILL.md / references/quantization-benchmarks.md — GDPVal is part of the AA
  suite but a different harness, so it is generated as a companion standalone config.
• recipes/env.example — TAVILY_API_KEY (agent web search) and GDPVAL_SIF_DIR.

Usage

# 1. Set GDPVAL_SIF_DIR in .env, then build the sandbox once on the target cluster
#    (build-if-absent, reuse-if-present):
srun -p cpu -t 01:00:00 --pty .agents/scripts/gdpval-sif.sh

# 2. Copy the whole example dir (the _gym_prepare.yaml include must travel with it),
#    fill in the ??? values, then dry-run -> canary -> full:
nel run --config gym_gdpval/example_gym_gdpval.yaml --dry-run

Testing

Validated end-to-end on an aarch64 GB300 SLURM cluster with an NVFP4 MoE checkpoint:
deploy → SIF build + sandboxed exec → gym head server → 220 rollouts + deliverables →
judge scoring, producing a real rubric score with zero judge failures. Several traps
found during that run are now documented in the reference (silent unsandboxed
fallback, gym-commit/head-server hang, judge api-key value-vs-name, SIF ↔ Gym version
coupling, limit_samples not limiting gym rollouts).

Before your PR is "Ready for review"

• Is this change backward compatible?: ✅
• If you copied code from any other sources or added a new PIP dependency, did you follow guidance in CONTRIBUTING.md: N/A
• Did you write any new necessary tests?: N/A (agent skill documentation + helper script; no library code)
• Did you update Changelog?: N/A (agent skill only, no API change)
• Did you get Claude approval on this PR?: ❌ (not yet run)

Additional Information

Docs/skill-only change under .agents/; no modelopt/ source is touched.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Added standalone GDPVal evaluation support through NeMo Gym, with Slurm, vLLM, sandbox, judge, reasoning, sampling, and logging configuration.
  • Added automated Apptainer/Singularity image setup with reuse, validation, locking, and reliable publishing.
  • Added configurable settings for judge services, web search, and shared image caching.
  • Added GDPVal preparation and execution examples, including dry-run, canary, and full-run guidance.

• Documentation

  • Added GDPVal setup, troubleshooting, scoring, deployment, and quantization guidance.
  • Clarified separate GDPVal configuration, mandatory thinking mode, and repeat-count requirements.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds a GDPVal SIF builder and a standalone NeMo Gym evaluation path. The change includes runtime preparation, sandbox and judge configuration, environment examples, staged execution guidance, score extraction, and separate GDPVal handling in AA quantization recommendations.

Changes

GDPVal evaluation

Layer / File(s)	Summary
GDPVal SIF builder .agents/scripts/gdpval-sif.sh	Adds path resolution, image reuse, locked builds, runtime discovery, fakeroot fallback, validation, cleanup, and atomic publication.
Standalone GDPVal evaluation configuration .agents/skills/evaluation/recipes/env.example, .agents/skills/evaluation/recipes/examples/gym_gdpval/example_gym_gdpval.yaml	Adds environment settings, Slurm and vLLM deployment, judge integration, rollout controls, sandbox setup, and MLflow export.
Gym runtime preparation .agents/skills/evaluation/recipes/examples/gym_gdpval/_gym_prepare.yaml	Adds Gym commit selection, dependency repair, environment setup, rollout execution, process cleanup, and Ray shutdown.
GDPVal workflow and benchmark guidance .agents/skills/evaluation/SKILL.md, .agents/skills/evaluation/recipes/tasks/aa_gym/gdpval.md, .agents/skills/evaluation/references/gym-gdpval.md, .agents/skills/evaluation/references/quantization-benchmarks.md	Documents standalone execution, SIF and judge requirements, staged validation, score extraction, and separate GDPVal inclusion in AA recommendations.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant nel
  participant vLLM
  participant nemo_gym
  participant GDPValJudge
  nel->>vLLM: Deploy reasoning-enabled policy
  nel->>nemo_gym: Start pinned GDPVal task
  nemo_gym->>vLLM: Collect policy rollouts
  nemo_gym->>GDPValJudge: Submit outputs for scoring
  GDPValJudge-->>nemo_gym: Return scores and deliverables
  nemo_gym-->>nel: Export results to MLflow

Loading

🚥 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 title clearly and concisely identifies the addition of GDPVal support to the evaluation skill.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Security Anti-Patterns	✅ Passed	The full PR adds no Python or dependency-manifest files. Added-line scans found none of the prohibited loaders, trust_remote_code=True, eval/exec, or # nosec patterns.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch chenjiel/gdpval-eval-skill

---

_{Comment @coderabbitai help to get the list of available commands.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.24%. Comparing base (01c708e) to head (3f8c7dd).
⚠️ Report is 26 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2039      +/-   ##
==========================================
- Coverage   75.81%   74.24%   -1.58%     
==========================================
  Files         518      520       +2     
  Lines       58574    62737    +4163     
==========================================
+ Hits        44406    46577    +2171     
- Misses      14168    16160    +1992     

Flag	Coverage Δ
unit	55.25% <ø> (+0.23%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 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.

[coderabbitai]

Warning

CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.

Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.

👉 Steps to fix this

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.agents/scripts/gdpval-sif.sh:
- Around line 45-46: Update GDPVAL_GYM_COMMIT and GDPVAL_SIF_NAME defaults in
.agents/scripts/gdpval-sif.sh to dd41196f620f2af99947d776cbe5da9439d2a08d and
python-3.13.gdpval.sif. In
.agents/skills/evaluation/recipes/examples/gym_gdpval/example_gym_gdpval.yaml:30-40,
no direct change is needed once the script defaults are corrected. Update the
documented SIF path to python-3.13.gdpval.sif in
.agents/skills/evaluation/SKILL.md:64-70,
.agents/skills/evaluation/recipes/tasks/aa_gym/gdpval.md:23-28, and
.agents/skills/evaluation/references/gym-gdpval.md:22-29.
- Around line 108-123: Update both Apptainer build invocations in the fakeroot
fallback flow to include the --force option, ensuring the commands using
"$APPTAINER_BIN" overwrite an existing "$tmp" without prompting.

In @.agents/skills/evaluation/recipes/examples/gym_gdpval/_gym_prepare.yaml:
- Around line 27-70: Update the final PYTHONPATH export in gym_prepare to
preserve any inherited $PYTHONPATH while prepending /opt/Gym and the main venv
site-packages. Use a shell conditional or equivalent that references $PYTHONPATH
without ${...} syntax, and keep the existing path ordering for Gym and its
dependencies.

In
@.agents/skills/evaluation/recipes/examples/gym_gdpval/example_gym_gdpval.yaml:
- Around line 166-175: Update the pre_cmd installation flow to detect the
container architecture and install an architecture-compatible Apptainer runtime,
avoiding the hardcoded amd64 1.4.2 package on aarch64 where no matching release
exists. Ensure installation failure is not masked by the current &&/fallback
behavior: verify Apptainer is available after installation and explicitly
terminate pre_cmd if it is unavailable, preserving the existing squashfuse setup
only after Apptainer succeeds.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 963909ee-d867-411c-a876-056e093a3662

📥 Commits

Reviewing files that changed from the base of the PR and between a23390d and 0c6a105.

📒 Files selected for processing (8)

• .agents/scripts/gdpval-sif.sh
• .agents/skills/evaluation/SKILL.md
• .agents/skills/evaluation/recipes/env.example
• .agents/skills/evaluation/recipes/examples/gym_gdpval/_gym_prepare.yaml
• .agents/skills/evaluation/recipes/examples/gym_gdpval/example_gym_gdpval.yaml
• .agents/skills/evaluation/recipes/tasks/aa_gym/gdpval.md
• .agents/skills/evaluation/references/gym-gdpval.md
• .agents/skills/evaluation/references/quantization-benchmarks.md

[coderabbitai]

Warning

CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.

Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.

👉 Steps to fix this

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
@.agents/skills/evaluation/recipes/examples/gym_gdpval/example_gym_gdpval.yaml:
- Around line 175-183: Update the pre_cmd dependency-installation flow to remove
“|| true” from the apt-get install command so missing squashfuse or fuse3 causes
the setup to fail. Keep apptainer installation validation and add an SIF
execution canary on the target cluster to verify mount dependencies are usable,
not merely that the binary exists.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 3adfdf25-b2a6-4f9a-bc34-7f661848dc9f

📥 Commits

Reviewing files that changed from the base of the PR and between 0c6a105 and 74dfa8b.

📒 Files selected for processing (5)

• .agents/scripts/gdpval-sif.sh
• .agents/skills/evaluation/recipes/examples/gym_gdpval/_gym_prepare.yaml
• .agents/skills/evaluation/recipes/examples/gym_gdpval/example_gym_gdpval.yaml
• .agents/skills/evaluation/recipes/tasks/aa_gym/gdpval.md
• .agents/skills/evaluation/references/gym-gdpval.md

🚧 Files skipped from review as they are similar to previous changes (1)

• .agents/skills/evaluation/recipes/tasks/aa_gym/gdpval.md

[meenchen]

Bot review (bedrock-claude-opus-5) — DM the bot to share feedback.

Docs/skill-only change that correctly reuses the existing aa_next/-style "branch" convention (recipe + shared reference + example dir + .agents/scripts helper), so the design question is largely settled by precedent — no competing in-repo system is introduced. Tests are legitimately N/A (no test harness exists for .agents/skills, and the license pre-commit hook only covers python/shell).

Issues found:

1. Contradictory num_repeats guidance — references/quantization-benchmarks.md tells the agent to "bump it back to 2 for quant validation", while references/gym-gdpval.md says "Use 1 … Do not carry a =2 into a current run" and recipes/tasks/aa_gym/gdpval.md says both goldens use 1. Quant validation is the use case in this repo, so the agent will hit the contradiction on the default path.
2. gdpval-sif.sh non-fakeroot fallback — apptainer build refuses to write an existing output file without --force, and the fakeroot attempt usually leaves a partial $tmp, so the elif fallback will most likely fail spuriously.
3. Score-extraction snippet is comparison-only while the shipped template is rubric-only — the copy-pasteable python3 -c will KeyError on the default config's results.
4. --env-file flag appears in two places but nowhere else in the skill (which documents set -a && source .env); please confirm 0.2.6 nel run accepts it.
5. GDPVAL_MAX_TURNS is presented as a container lit: env var while the effective value comes from a submit-time ${oc.env:GDPVAL_MAX_TURNS,250} interpolation — the two can't both be right, and the skill elsewhere warns against ${oc.env:...} for non-exported vars.

Nits: the two new YAML headers use Copyright (c) 2025, NVIDIA CORPORATION. which diverges from the canonical LICENSE_HEADER (2026, "& AFFILIATES") that gdpval-sif.sh matches exactly — worth aligning (no third-party code is vendored, so no legal concern beyond consistency). Also note the AA rule change now makes every "run AA" request additionally require TAVILY_API_KEY, a judge endpoint and a prebuilt SIF; the "let the user opt out" wording is doing a lot of work there. Size (~1000 lines) is fine given it's almost entirely prose.

[meenchen]

Bot comment.

This contradicts the other two new docs. references/gym-gdpval.md ("## num_repeats") says "Use 1. Both current goldens do … Do not carry a =2 into a current run", and recipes/tasks/aa_gym/gdpval.md says "num_repeats=1 — already set by the template … both current goldens use it". Here the agent is told the golden is 2 and that the template lowered it to 1 "to halve cost", so on the default quant-validation path (the primary use case in this repo) it will override back to ++num_repeats=2 — doubling a multi-hour, judge-metered run against explicit guidance elsewhere. Please pick one story and state it in a single place, with the others pointing at it.

[cjluo-nv]

Fixed — now says 1 everywhere. The reviewed golden uses num_repeats=1; this file was telling the primary quant-validation path to bump back to 2.

[meenchen]

Bot comment.

The non-fakeroot fallback will probably never succeed: apptainer build errors out when the destination image file already exists unless --force is passed, and a failed --fakeroot attempt commonly leaves a partial $tmp behind. As written, a cluster without an /etc/subuid entry gets _die "apptainer build failed" even though the unprivileged build would have worked. Suggest rm -f "$tmp" (or --force) before the fallback:

if "$APPTAINER_BIN" build --fakeroot "$tmp" "$def_local"; then
  :
else
  rm -f "$tmp"
  if "$APPTAINER_BIN" build "$tmp" "$def_local"; then ...

[cjluo-nv]

Fixed — clearing $tmp before the unprivileged fallback.

[meenchen]

Bot comment.

Under set -e, this [[ -d "$sif_dir" ]] && { … } is a non-final command in the list, so when the dir does not exist the script exits right here and the "Build it with: …" hint on the next line never prints — which is exactly the case where the hint is most useful. Use if [[ -d … ]]; then … fi (or append || true).

Also, inside the block ls -1 … | sed … || echo " (no .sif files)" binds || to the pipeline, and sed exits 0 on empty input, so the "(no .sif files)" branch is effectively dead. Gate on the ls exit status instead.

[cjluo-nv]

Fixed — if/then/fi so the "Build it with:" hint prints when the dir is missing, and the (no .sif files) branch is now gated on ls rather than the (always-0) sed.

[meenchen]

Bot comment.

Design question worth answering in the PR body: this file describes itself as "shared … by every benchmark", but there is exactly one gym benchmark and one consumer (example_gym_gdpval.yaml), and the split creates a footgun that then has to be warned about in five separate places (SKILL.md, the recipe, the reference, the config header, the failure-modes list: "if it still hangs, the _gym_prepare.yaml include didn't travel with the config").

Inlining the two shell blocks directly into the task command: would make the example a single self-contained file like example_eval.yaml / example_eval_next.yaml and remove the whole co-location hazard. If the include exists to anticipate future aa_gym/ tasks, please say so explicitly — otherwise the simpler single-file form looks preferable.

[cjluo-nv]

Agreed — inlined both blocks into the task command: and deleted this file. The example is now a single self-contained YAML and the "must travel with the config" warning is gone from all five places it appeared.

[meenchen]

Bot comment.

GDPVAL_MAX_TURNS is documented three ways that don't agree: here as an optional container env var (lit:100), in references/gym-gdpval.md's env table as lit (optional) … default 100; golden uses 250, but the value the gym actually uses comes from ++…agent_max_turns=${oc.env:GDPVAL_MAX_TURNS,250} in common_params — an OmegaConf interpolation resolved at submit time against the launching shell, not the container env. Uncommenting GDPVAL_MAX_TURNS: lit:100 therefore has no effect on agent_max_turns, and SKILL.md Step 5 explicitly warns against ${oc.env:...} for values that aren't exported with set -a. Please make it one mechanism (either drop the lit: line and document it as a host-side .env value, or hardcode 250 in the override) and fix the reference table to match.

[cjluo-nv]

Fixed — removed. It could never affect agent_max_turns, which is an ${oc.env:…} resolved at submit time against the launching shell; the golden doesn't set it either.

[meenchen]

Bot comment.

nel run … --env-file .env doesn't appear anywhere else in the skill — SKILL.md Step 8 and every other example use set -a && source .env && set +a with host: prefixes, and this is the only occurrence of --env-file in the repo. If 0.2.6 nel doesn't have that flag, the canary command as written fails immediately (it's repeated in recipes/tasks/aa_gym/gdpval.md). Please confirm against the installed CLI or switch to the documented source .env form for consistency.

[cjluo-nv]

Checked: --env-file is a real nel 0.2.6 flag (verified against the installed CLI), so leaving it.

[meenchen]

Bot comment.

This copy-pasteable snippet hardcodes gdpval_stirrup_agent/comparison/{k}, but the shipped template is rubric-only (gdpval.reward_mode: rubric), so on the default config it will KeyError — and the section above states flatly "The reported GDPVal score is normalized_elo", which is undefined in rubric mode per your own table row. Since rubric is what most users will run first, add the rubric extraction (mean reward over evaluator_rollouts.jsonl) alongside it, or gate the snippet with an explicit "comparison mode only" heading.

[cjluo-nv]

Fixed — added the rubric extraction (mean reward over evaluator_rollouts.jsonl) beside the comparison one, each labelled by mode, so the shipped rubric-default config no longer KeyErrors.

[cjluo-nv]

Thanks — worked through all of these. Summary of what changed, and what I deliberately didn't change.

The four CodeRabbit inline comments were made against 0c6a105b2 (the first commit); the branch is now 13 commits in, so several were already resolved by later work. Marking those as such rather than silently ignoring them.

Fixed

Finding	Resolution
gdpval-sif.sh: SIF commit/filename defaults stale vs the template	Already fixed after that review — defaults are now dd41196f… / python-3.13.gdpval.sif, matching the template pin.
gdpval-sif.sh: unprivileged fallback can't succeed if --fakeroot leaves $tmp	Real bug, fixed: rm -f "$tmp" before the fallback build. Our successful build only worked because fakeroot failed before creating the file.
gdpval-sif.sh --check: [[ -d … ]] && { … } under set -e	Real bug, fixed with if/then/fi. The script exited before printing the "Build it with:" hint — exactly when it's most useful. The (no .sif files) branch was also dead (sed exits 0 on empty input); now gated on ls.
pre_cmd hardcodes an amd64 apptainer .deb	Fixed: installs via the Ubuntu PPA (arch-aware). aarch64 is most Blackwell/Grace clusters, so the hardcoded deb simply could not install.
PYTHONPATH overwritten, discarding inherited value	Fixed: prepend instead. Note ${PYTHONPATH:+…} can't be used — OmegaConf parses ${…} in the surrounding YAML — so it uses a plain $VAR test.
GDPVAL_MAX_TURNS documented three inconsistent ways	Fixed: removed the container env var. It could never affect agent_max_turns, which is an ${oc.env:…} resolved at submit time against the launching shell. The golden doesn't set it either.
num_repeats contradiction in quantization-benchmarks.md	Fixed. The reviewed golden uses 1; that file was telling the primary quant-validation path to bump back to 2, i.e. to double a multi-hour judge-metered run against guidance in the other two docs.
Score-extraction snippet is comparison-only but the template ships rubric	Fixed: added the rubric form (mean reward over evaluator_rollouts.jsonl) beside it, each labelled by mode.
Design: _gym_prepare.yaml split is a footgun for one benchmark / one consumer	Agreed and done — inlined into the task command: and deleted the file. The example is now a single self-contained YAML like example_eval.yaml, and the "must travel with the config" warning is gone from all five places it appeared.

Not changed, with reasons

• --env-file .env — this is a real nel 0.2.6 flag (verified against the installed CLI), not a typo for the source .env form.
• || true on the squashfuse/fuse3 install — deliberate. Those are optional: without them apptainer falls back to per-call extraction (slower, still sandboxed). The apptainer install itself has no || true and fails loudly, which is the case that actually matters. The perf caveat is documented in the reference.
• MLflow auto-export being rejected on aws-cmh — evidence says otherwise. The export job ran on partition cpu and COMPLETED in 2m20s, because sbatch_extra_flags is not propagated to the export sbatch. Leaving the docs as-is.

Validation

The skill was exercised end-to-end rather than just proofread: a GDPVal comparison-mode run on an aarch64 GB300 cluster completed and produced a real score (normalized_elo), and after each round of edits a subagent regenerated the config from the skills alone — blocked from seeing the known-good answer — and the output was diffed against it. All score-determining settings (9 references + ELOs, both mount maps, container, SIF, reward_mode, num_comparison_trials, judge_sampling_seed, judge auth, both throttles, num_repeats, multistage shape) matched every time, including after the inlining, where the agent copied the inlined command: verbatim.

[sugunav14]

Two line-anchored findings below. Separately, the SIF helper's flock/double-check/atomic-mv handling and the test -d vs -f validation-gap analysis are solid, and the python-3.13 filename + dd41196f... gym pin are now consistent across all five files that reference them.

Lower-severity items (no cleanup trap in gdpval-sif.sh, --check testing the local FS rather than the cluster, -f-only SIF validation, unscoped pkill -f raylet, unpinned apptainer PPA in pre_cmd, GDPVAL_MAX_TURNS missing from env.example) are in my earlier comment on this PR.

[sugunav14]

set -ex here puts HF_TOKEN / INFERENCE_API_KEY / TAVILY_API_KEY in plaintext in the eval log.

The task command: opens with set -ex (L244), and these two param blobs expand to +hf_token=$HF_TOKEN (data_prep_params) and, in common_params:

++gdpval_judge_model.responses_api_models.openai_model.openai_api_key=$INFERENCE_API_KEY
++gdpval_stirrup_agent.responses_api_agents.stirrup_agent.tavily_api_key=$TAVILY_API_KEY

set -x traces commands after expansion, so the + ng_prepare_benchmark ... trace line carries all three secret values. They land in the eval log on shared FS, and export.mlflow.log_logs: true (L369) then uploads that log to MLflow.

What suggests an oversight rather than an accepted tradeoff: the rollout call at L297 passes the same common_params, but it's written into /tmp/gym_run.sh via the quoted heredoc (L296-299) and run by a shell that never sets -x -- so $INFERENCE_API_KEY stays literal and is expanded only at run time, never traced. The right pattern is already there one line down; this call just isn't covered by it.

Suggested change

	ng_prepare_benchmark {{config.params.extra.nemo_gym.data_prep_params}} {{config.params.extra.nemo_gym.common_params}}
	set +x
	ng_prepare_benchmark {{config.params.extra.nemo_gym.data_prep_params}} {{config.params.extra.nemo_gym.common_params}}
	set -x

(The set -ex at L171 in pre_cmd is fine -- nothing on those lines carries a secret.)

[cjluo-nv]

Confirmed and fixed in 4bc6937d9 — thank you, this was a real leak, not a tradeoff.

I checked a completed run's client log rather than reasoning about it, and the traced line carries the expanded value:

hf_token=$HF_TOKEN     <- what the config says
hf_token=hf_TOBgm...   <- what the log actually contains

Both HF_TOKEN and INFERENCE_API_KEY were present. Applied your suggestion (set +x / set -x around that one call). Your read of why it was an oversight is exactly right: the rollout call below already gets this for free via the quoted heredoc, and this call simply wasn't covered by the same pattern.

Beyond the code fix, the already-completed runs need remediation — those logs went to MLflow via export.mlflow.log_logs: true, so the affected keys are being rotated and the uploaded artifacts purged.

[sugunav14]

Is limit_samples actually a canary on the gym path?

The PR description lists, among traps found during the validated run:

limit_samples not limiting gym rollouts

But limit_samples=2 is still presented as the canary in three places -- here, SKILL.md (GDPVal branch, step 4), and the example_gym_gdpval.yaml header -- and that trap appears nowhere in the final docs (no occurrence of limit_samples in references/gym-gdpval.md). It reads like the finding was dropped in the 436->341-line compression pass while the instruction depending on it survived.

If the trap is real, someone following this step launches the full 220-task multi-hour run believing it's a two-task smoke test -- inverting the purpose of the step, on the heaviest benchmark in the suite. Worth either documenting how to actually bound a gym canary, or stating plainly that limit_samples is inert here and describing what the canary is instead (e.g. watch the first rollouts for the SIF-fallback warning and judge auth, then cancel).

I couldn't verify this one empirically -- if limit_samples does work on the gym path, disregard.

[cjluo-nv]

Confirmed — limit_samples is inert on the gym path, and your inference about how it happened is right: it was observed during the validated run (the "limit_samples=2 canary" ran all 220 tasks), made the PR description, and was then lost in the 436->341-line compression while the three instructions depending on it survived.

Fixed in 4bc6937d9 in all three places (recipe, SKILL.md GDPVal branch, template header). The gym does its own data prep and rollout collection, so the launcher-level limiter never reaches it.

Since there is no cheap sample-limited canary, the docs now describe the real procedure instead: launch, and treat the first ~20-30 minutes as the canary, with the greps to run —

grep -c "Using Apptainer container"   $RD/logs/client-*.log        # sandbox actually used
grep -c "falling back\|not a git repo" $RD/logs/client-*.log       # unsandboxed / inert pin
grep -ciE " 401 | 403 |Internal Server Error" $RD/artifacts/nemo_gym_logs/gdpval_judge_model.log
wc -l $RD/artifacts/evaluator_rollouts.jsonl                       # rollouts flowing

— plus a note that in comparison mode stage 1 (45 tasks) is a natural early checkpoint, since an ELO estimate lands before the 220-task stage 2 begins.

[cjluo-nv]

@sugunav14 — both line-anchored findings were real and are fixed in 4bc6937d9; the lower-severity list from your earlier comment is fixed in 507007b56. Details on each thread, summary here.

The two findings. The set -x one was a genuine leak, not an accepted tradeoff — I verified it against a completed run's log rather than reasoning about it, and the trace line carries the expanded HF_TOKEN / INFERENCE_API_KEY. Applied your suggested set +x / set -x. Separately, the affected keys are being rotated and the MLflow-uploaded logs purged, since log_logs: true had already shipped them. The limit_samples one was exactly as you guessed: observed during the validated run, reached the PR description, then lost in the compression pass while the three instructions depending on it survived. Now documented as inert, with the real canary procedure in its place.

Lower-severity items (507007b56):

Item	Resolution
No cleanup trap in gdpval-sif.sh	Added an EXIT trap so an interrupted build leaves no .build.* / .def leftovers in the shared SIF dir.
-f-only SIF validation	Real gap — -f passes on a 0-byte file, which is what an interrupted copy leaves, and the failure it guards against is silent. Now also requires a plausible size (>100 MB) and a successful apptainer inspect where available. Verified on-cluster: real SIF passes, 0-byte one is rejected with a rebuild hint.
--check tests the local FS	Documented that it inspects the filesystem it runs on, so it must be run on the cluster (srun/ssh), not the submitting box.
Unscoped pkill -f raylet	Scoped to our own uid — the unscoped form matches every raylet on a shared node, including other jobs'.
Unpinned apptainer PPA	Kept unpinned deliberately (it is the only source with current arm64 builds, and pinning risks unavailability), but documented the reasoning and the resolved version is echoed into the run log.
GDPVAL_MAX_TURNS missing from env.example	Added, with the reason it belongs there: it is read at submit time from the launching shell via ${oc.env:...}, so the container-env form has no effect.

Thanks for the careful pass — the two anchored findings were the highest-value review input this PR has had, and both were things the end-to-end run had already brushed past without surfacing.

[sugunav14]

LGTM!

[cjluo-nv]

Follow-up: compressed the template in 3f8c7dd5f — 391 → 360 lines, comments 168 → 137.

Context for why this was worth doing separately: the two markdown files have held at ~365 lines since the earlier compression pass, but the template kept growing, because each review fix in this PR added a three-to-six line explanation immediately next to the line it fixed. That is the right instinct while fixing, and the wrong end state — the file people copy ended up with more comment than config.

The rule I applied: the template keeps whatever prevents an edit mistake at that line; the reasoning moves to references/gym-gdpval.md, which is where a reader has already been sent. So these stay, in one or two lines each:

• never put # inside the folded (>-) scalar — it folds to one line and comments out every override after it
• mount keys are never interpolated, so use the literal path
• set +x around the traced call, or the params' secrets land in the log
• api_key is injected as a VALUE, not a name
• plain $VAR only, because OmegaConf parses ${...}
• the uid-scoped pkill

and the six-line explanations behind each are gone from the yaml.

Verified after the cut, not just eyeballed: the YAML parses, the inlined command: still passes bash -n with quote-containing params substituted, no # leaked into any folded scalar, and all nine load-bearing markers are still present.

Running totals for the docs in this PR: reference 293 → 236, recipe 143 → 129, template 391 → 360.

[github-actions]

PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-08-03 22:10 UTC