Agent Skills Updates From Live Trials#1493
Conversation
chadvoegele
changed the title
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR enhances evaluation workflows with improved quantization detection and baseline comparison requirements, standardizes task configurations into Markdown-based recipes with Python extraction helpers, introduces PTQ post-quantization validation gates, adds job monitoring via durable polling, and enables SLURM Quality of Service configuration support. ChangesEvaluation Workflow Enhancement
Task Recipe Standardization and Configuration Updates
PTQ Validation Framework
Monitoring, Baseline Verification, and Infrastructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 @.claude/skills/monitor/SKILL.md:
- Around line 54-59: The documentation/logic currently enforces "report only
state changes" universally; update it so that user-initiated checks (e.g., when
the user explicitly asks "check status") return the full current status for each
job rather than only deltas—leave monitor-driven checks to still compare against
`last_status` in `.claude/active_jobs.json` and report only changes. Adjust the
wording and any associated pseudocode/implementation notes to branch on the
trigger type ("monitor output" vs "user-initiated") and on user-initiated flows
ensure you read the registry, check each job, return current state for each job,
and then update `last_status` accordingly. Ensure references to `last_status`
and `.claude/active_jobs.json` remain consistent so maintainers can find and
implement the conditional behavior.
🪄 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: 27a4dd3f-7246-45c7-8699-ec80a318c50a
📒 Files selected for processing (11)
.claude/skills/common/environment-setup.md.claude/skills/evaluation/SKILL.md.claude/skills/evaluation/tests/evals.json.claude/skills/launching-evals/references/analyze-results.md.claude/skills/monitor/SKILL.md.claude/skills/ptq/SKILL.md.claude/skills/ptq/references/checkpoint-validation.md.gitignoremodelopt/torch/quantization/model_quant.pytools/launcher/core.pytools/launcher/slurm_config.py
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1493 +/- ##
==========================================
+ Coverage 76.75% 77.20% +0.45%
==========================================
Files 476 476
Lines 51811 51811
==========================================
+ Hits 39767 40001 +234
+ Misses 12044 11810 -234
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.claude/skills/evaluation/SKILL.md (1)
203-212:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix task snippet schema mismatch (
tasksvsevaluation.tasks).The Step 5 example contradicts earlier instructions to edit
evaluation.tasks. Keepingtasks:here can make generated configs invalid or ignored.Suggested fix
- tasks: - - name: <task> - nemo_evaluator_config: - config: - params: - temperature: <value> - max_new_tokens: <value> - ... + evaluation: + tasks: + - name: <task> + nemo_evaluator_config: + config: + params: + temperature: <value> + max_new_tokens: <value> + ...🤖 Prompt for 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. In @.claude/skills/evaluation/SKILL.md around lines 203 - 212, The YAML example uses a top-level "tasks:" key which conflicts with the expected "evaluation.tasks" namespace; update the snippet so the tasks list is nested under "evaluation.tasks" (e.g., replace "tasks:" with "evaluation.tasks:" and keep the existing task entries like "name" and "nemo_evaluator_config" intact), and verify any references to "tasks" in the surrounding text or examples are corrected to "evaluation.tasks" to keep schema consistent.
🤖 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 @.claude/skills/evaluation/recipes/tasks/gpqa.md:
- Around line 73-75: The extractor extract_gpqa_score currently can raise
IndexError when called without args and uses raw open(...) with yaml.safe_load;
add basic argument validation (ensure path is provided and repeats, if given, is
an int) and use a safe file context (with open(path, "r") as f) and
yaml.safe_load(f) while catching FileNotFoundError and yaml.YAMLError and
re-raising a clear ValueError; also validate that the expected keys exist in the
loaded dict (results -> groups -> gpqa -> metrics) and raise ValueError if
missing. Apply the same validation and safe-loading pattern to the similar
extractor function around lines 94-97 to ensure consistent error handling.
In @.claude/skills/evaluation/recipes/tasks/scicode.md:
- Around line 105-108: The extract_score function currently assumes a valid path
and opens the YAML without a context manager; fix it by validating the path
argument (raise ValueError or return a clear error if path is falsy), check the
file exists (catch FileNotFoundError), and read the YAML using a context manager
(with open(path) as f: data = yaml.safe_load(f)); then safely access
TASKS[group] and data["results"]["groups"][group"]["metrics"] (use .get or catch
KeyError to provide a clearer error). Apply the same changes to the other
identical snippet that reads the YAML and accesses metrics.
---
Outside diff comments:
In @.claude/skills/evaluation/SKILL.md:
- Around line 203-212: The YAML example uses a top-level "tasks:" key which
conflicts with the expected "evaluation.tasks" namespace; update the snippet so
the tasks list is nested under "evaluation.tasks" (e.g., replace "tasks:" with
"evaluation.tasks:" and keep the existing task entries like "name" and
"nemo_evaluator_config" intact), and verify any references to "tasks" in the
surrounding text or examples are corrected to "evaluation.tasks" to keep schema
consistent.
🪄 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: 1c2365f0-8719-4c16-9dd8-7640d57849f7
📒 Files selected for processing (18)
.claude/skills/evaluation/SKILL.md.claude/skills/evaluation/recipes/examples/example_eval.yaml.claude/skills/evaluation/recipes/tasks/aime2025.md.claude/skills/evaluation/recipes/tasks/aime2025.yaml.claude/skills/evaluation/recipes/tasks/gpqa.md.claude/skills/evaluation/recipes/tasks/gpqa.yaml.claude/skills/evaluation/recipes/tasks/ifbench.md.claude/skills/evaluation/recipes/tasks/ifbench.yaml.claude/skills/evaluation/recipes/tasks/livecodebench.md.claude/skills/evaluation/recipes/tasks/livecodebench.yaml.claude/skills/evaluation/recipes/tasks/mmlu_pro.md.claude/skills/evaluation/recipes/tasks/mmlu_pro.yaml.claude/skills/evaluation/recipes/tasks/scicode.md.claude/skills/evaluation/recipes/tasks/scicode.yaml.claude/skills/evaluation/tests/evals.json.claude/skills/ptq/SKILL.md.claude/skills/ptq/references/checkpoint-validation.md.claude/skills/ptq/tests.json
💤 Files with no reviewable changes (6)
- .claude/skills/evaluation/recipes/tasks/ifbench.yaml
- .claude/skills/evaluation/recipes/tasks/mmlu_pro.yaml
- .claude/skills/evaluation/recipes/tasks/aime2025.yaml
- .claude/skills/evaluation/recipes/tasks/scicode.yaml
- .claude/skills/evaluation/recipes/tasks/gpqa.yaml
- .claude/skills/evaluation/recipes/tasks/livecodebench.yaml
✅ Files skipped from review due to trivial changes (3)
- .claude/skills/evaluation/recipes/examples/example_eval.yaml
- .claude/skills/evaluation/recipes/tasks/ifbench.md
- .claude/skills/evaluation/recipes/tasks/mmlu_pro.md
🚧 Files skipped from review as they are similar to previous changes (1)
- .claude/skills/evaluation/tests/evals.json
| def extract_score(path, group="scicode"): | ||
| spec = TASKS[group] | ||
| data = yaml.safe_load(open(path)) | ||
| metrics = data["results"]["groups"][group]["metrics"] |
There was a problem hiding this comment.
Harden CLI/file handling in the score extractor snippet.
The snippet crashes with IndexError when no path is passed, and it opens the YAML file without a context manager.
Suggested fix
def extract_score(path, group="scicode"):
spec = TASKS[group]
- data = yaml.safe_load(open(path))
+ with open(path, "r", encoding="utf-8") as f:
+ data = yaml.safe_load(f)
@@
if __name__ == "__main__":
- path = sys.argv[1]
+ if len(sys.argv) < 2:
+ raise SystemExit("Usage: python extract_score.py <results.yaml> [scicode|gpqa]")
+ path = sys.argv[1]
group = sys.argv[2] if len(sys.argv) > 2 else "scicode"
print(extract_score(path, group))Also applies to: 135-138
🤖 Prompt for 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.
In @.claude/skills/evaluation/recipes/tasks/scicode.md around lines 105 - 108,
The extract_score function currently assumes a valid path and opens the YAML
without a context manager; fix it by validating the path argument (raise
ValueError or return a clear error if path is falsy), check the file exists
(catch FileNotFoundError), and read the YAML using a context manager (with
open(path) as f: data = yaml.safe_load(f)); then safely access TASKS[group] and
data["results"]["groups"][group"]["metrics"] (use .get or catch KeyError to
provide a clearer error). Apply the same changes to the other identical snippet
that reads the YAML and accesses metrics.
There was a problem hiding this comment.
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 @.claude/skills/evaluation/SKILL.md:
- Around line 260-267: The preflight check currently tests for any credential
entry using the generic grep command; change it to verify credentials per
registry host used by the selected images by searching for the specific registry
hostnames (not just any "machine" entry) in ~/.config/enroot/.credentials.
Update the documented check (the grep invocation shown) to demonstrate matching
the actual registry host(s) (e.g., loop or run grep for each selected image's
registry host) so the preflight returns true only when credentials exist for
those specific hosts.
🪄 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: 466a2d74-5bf4-40f5-a94e-78d17d191a59
📒 Files selected for processing (7)
.claude/skills/evaluation/SKILL.md.claude/skills/evaluation/recipes/env.example.claude/skills/evaluation/recipes/tasks/aa_lcr.md.claude/skills/evaluation/recipes/tasks/aime2025.md.claude/skills/evaluation/recipes/tasks/hle_aa.md.claude/skills/evaluation/recipes/tasks/ifbench.md.claude/skills/evaluation/recipes/tasks/mmlu_pro_aa_v3.md
✅ Files skipped from review due to trivial changes (6)
- .claude/skills/evaluation/recipes/env.example
- .claude/skills/evaluation/recipes/tasks/mmlu_pro_aa_v3.md
- .claude/skills/evaluation/recipes/tasks/aa_lcr.md
- .claude/skills/evaluation/recipes/tasks/hle_aa.md
- .claude/skills/evaluation/recipes/tasks/ifbench.md
- .claude/skills/evaluation/recipes/tasks/aime2025.md
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.claude/skills/evaluation/recipes/tasks/tau2_bench_telecom.md (1)
38-39:⚠️ Potential issue | 🟠 Major | ⚡ Quick winComplete the Score Extraction section.
The Score Extraction section contains only a header with no content. Users need guidance on how to extract and interpret the
pass_1metric for this task. Please add content similar to other task recipes (e.g., AIME 2025, GPQA) that explains which metric to use and how to extract it from the evaluation results.Do you want me to help draft the score extraction guidance based on the primary metric
pass_1and the tau2_bench harness documentation?🤖 Prompt for 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. In @.claude/skills/evaluation/recipes/tasks/tau2_bench_telecom.md around lines 38 - 39, The Score Extraction section is empty—add a short paragraph stating that the primary metric is pass_1 and instruct users to extract the pass_1 value from the tau2_bench evaluation results (e.g., from the metrics or results JSON under the "pass_1" key), report it as a percentage (multiply by 100 if the harness returns a fraction), and include any aggregation used (mean across seeds or runs). Reference the task name tau2_bench_telecom and the harness docs for exact JSON field names and show that the reported score should be the aggregated pass_1 value used for comparisons.
♻️ Duplicate comments (1)
.claude/skills/evaluation/SKILL.md (1)
260-262:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake credential verification registry-specific.
The grep command checks for any credential entry, not credentials for the specific registry host(s) used by the selected images. This can pass the preflight but still fail image pulls if credentials for the required registries are missing.
📝 Suggested fix
-ssh <host> "grep -E '^\s*machine\s+' ~/.config/enroot/.credentials 2>/dev/null" +ssh <host> "awk '/^\s*machine\s+/ {print $2}' ~/.config/enroot/.credentials 2>/dev/null" +# Verify the required registry host(s) from selected images are present (e.g., docker.io, nvcr.io, registry.internal).🤖 Prompt for 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. In @.claude/skills/evaluation/SKILL.md around lines 260 - 262, The current preflight uses the command string ssh <host> "grep -E '^\s*machine\s+' ~/.config/enroot/.credentials 2>/dev/null" which only checks for any credential entry; change it to verify registry-specific credentials by extracting registry hostnames from the selected images and running grep for each host (e.g., grep -E "^\s*machine\s+<registryHost>\b" ~/.config/enroot/.credentials) or equivalent per-host checks over SSH; update the code that emits the ssh grep command in SKILL.md to iterate the image registry list and fail the preflight if any registryHost lookup returns no match.
🤖 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 @.claude/skills/evaluation/SKILL.md:
- Around line 59-68: Update the ambiguous phrase "Do Step 3, Step 4, then Step
7.5/8" in the SKILL.md shortcut section to explicitly state which Step 8
substeps apply; replace it with something like "Do Step 3, Step 4, Step 7.5,
then Step 8 (complete the applicable substeps 8.1/8.2/8.3)" or "Do Step 3, Step
4, Step 7.5, then Step 8.1–8.3 as applicable" so readers aren’t confused by the
restructured Step 8; locate the exact string "Do Step 3, Step 4, then Step
7.5/8" and update it accordingly.
---
Outside diff comments:
In @.claude/skills/evaluation/recipes/tasks/tau2_bench_telecom.md:
- Around line 38-39: The Score Extraction section is empty—add a short paragraph
stating that the primary metric is pass_1 and instruct users to extract the
pass_1 value from the tau2_bench evaluation results (e.g., from the metrics or
results JSON under the "pass_1" key), report it as a percentage (multiply by 100
if the harness returns a fraction), and include any aggregation used (mean
across seeds or runs). Reference the task name tau2_bench_telecom and the
harness docs for exact JSON field names and show that the reported score should
be the aggregated pass_1 value used for comparisons.
---
Duplicate comments:
In @.claude/skills/evaluation/SKILL.md:
- Around line 260-262: The current preflight uses the command string ssh <host>
"grep -E '^\s*machine\s+' ~/.config/enroot/.credentials 2>/dev/null" which only
checks for any credential entry; change it to verify registry-specific
credentials by extracting registry hostnames from the selected images and
running grep for each host (e.g., grep -E "^\s*machine\s+<registryHost>\b"
~/.config/enroot/.credentials) or equivalent per-host checks over SSH; update
the code that emits the ssh grep command in SKILL.md to iterate the image
registry list and fail the preflight if any registryHost lookup returns no
match.
🪄 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: 38a710dc-b0aa-4860-99a8-27d5da2f52cc
📒 Files selected for processing (13)
.claude/skills/evaluation/SKILL.md.claude/skills/evaluation/recipes/env.example.claude/skills/evaluation/recipes/examples/example_eval.yaml.claude/skills/evaluation/recipes/tasks/aa_lcr.md.claude/skills/evaluation/recipes/tasks/aime2025.md.claude/skills/evaluation/recipes/tasks/gpqa.md.claude/skills/evaluation/recipes/tasks/hle_aa_v2.md.claude/skills/evaluation/recipes/tasks/ifbench.md.claude/skills/evaluation/recipes/tasks/livecodebench.md.claude/skills/evaluation/recipes/tasks/mmlu_pro.md.claude/skills/evaluation/recipes/tasks/mmmu_pro.md.claude/skills/evaluation/recipes/tasks/scicode.md.claude/skills/evaluation/recipes/tasks/tau2_bench_telecom.md
✅ Files skipped from review due to trivial changes (6)
- .claude/skills/evaluation/recipes/tasks/mmmu_pro.md
- .claude/skills/evaluation/recipes/tasks/hle_aa_v2.md
- .claude/skills/evaluation/recipes/tasks/ifbench.md
- .claude/skills/evaluation/recipes/tasks/mmlu_pro.md
- .claude/skills/evaluation/recipes/tasks/livecodebench.md
- .claude/skills/evaluation/recipes/tasks/aa_lcr.md
🚧 Files skipped from review as they are similar to previous changes (4)
- .claude/skills/evaluation/recipes/env.example
- .claude/skills/evaluation/recipes/tasks/gpqa.md
- .claude/skills/evaluation/recipes/examples/example_eval.yaml
- .claude/skills/evaluation/recipes/tasks/scicode.md
chadvoegele
force-pushed
the
cvoegele/agent_evals
branch
3 times, most recently
from
f2b92ba to
32c2072
Compare
| Prefer the `pass@1[avg-of-N]` metric matching the configured repeat count. | ||
| If the repeat count is unknown, use the highest available `avg-of-N`. | ||
|
|
||
| ```python |
There was a problem hiding this comment.
qq why we still need these python code here?
There was a problem hiding this comment.
This is to help guide the agent to pull the right score from the results.yml. There is a range of possible instructions:
- "extract the score"
- "extract the
pass@1[avg-of-N] - "run
yq '.results.groups.aime25.metrics."pass@1[avg-of-16]".scores.symbolic_correct.value' results.yml" - "run the python snippet below..."
The first instruction is far too vague. The agent will write something but probably not what you want.
This is from a recent run. Even with the Python snippet in the evaluation skill recipe, it ignored it and made it's own parser:
python3 -c "
import yaml
r = yaml.safe_load(open(\"$BASE/results.yml\"))
m = r.get(\"results\",{}).get(\"groups\",{}).get(\"mmlu-pro\",{}).get(\"metrics\",{}).get(\"pass@1\",{}).get(\"scores\",{})
print(\"symbolic_correct:\", m.get(\"symbolic_correct\",{}).get(\"value\"))
print(\"n:\", m.get(\"symbolic_correct\",{}).get(\"stats\",{}).get(\"count\"))
"'
Then I instructed the agent with Please reference the score extraction snippet in skills/evaluation/recipes/tasks/ when pullings scores from the results.yml for each dataset.
and it did
def extract_aime2025_score(path, repeats=None):
data = yaml.safe_load(open(path))
metrics = data["results"]["groups"]["aime25"]["metrics"]
name = select_metric(metrics, repeats)
scores = metrics[name]["scores"]
accuracy = scores["symbolic_correct"]["value"]
se = scores.get("symbolic_correct_statistics_std_err_across_runs", {}).get("value")
return {"group": "aime25", "metric": name, "score_key": "symbolic_correct",
"accuracy": accuracy,
"stderr": se * 100 if se is not None else None}
I'm err-ing on the side of over-specifying because I want the agent to pull the exact score I want. If I can get it reliably read the skill that's there, I believe it will.
There was a problem hiding this comment.
Will this make adding new evals harder because then we need to add a code section.
cjluo-nv
left a comment
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review — DM the bot to share feedback.
Operator asked to prioritize landing this. Production code changes are small and look fine (qos is an optional, backward-compatible field on SlurmConfig plumbed through build_slurm_executor; default None so existing callers are unaffected). The bulk of the PR is iteration on existing .claude/skills/... agent docs, which doesn't introduce a new subsystem (design review protocol doesn't really fire — these skills already exist), and a human reviewer (shengliangxu) has already LGTM'd.
A few things still warrant a human eye before merge rather than an auto-approve:
- PR is still marked WIP. Title is "WIP: Agent Skills Updates From Live Trials" and the PR body has unchecked TODOs ("Did you update Changelog: TODO", "Did you get Claude approval on this PR: TODO"). The author's own description says they're still iterating ("I'm running the full loop with the above prompt, and iterating on skills to resolve undesired agent behavior"). Worth confirming with @chadvoegele that this is actually ready to land vs. still being iterated on.
- 💬 Unanswered maintainer question from @cjluo-nv on
.claude/skills/evaluation/recipes/tasks/aime2025.md:51: "qq why we still need these python code here?" — this is about the score-extraction Python snippets that now appear inline in every task.md(gpqa, aime2025, scicode, mmlu_pro, mmmu_pro, ns_hle_aa, aa_lcr). Same question implicitly applies to all of them. No reply yet; should be resolved before merge. tools/launcher/slurm_config.pystyle drift. The file now usesOptional[str]+from typing import Optional+ a# ruff: noqa: UP045suppression, which is inconsistent with the rest of the codebase using PEP-604str | None. If there's a concrete reason (e.g.nemo_run's@run.cli.factorydecorator can't introspectX | Noneon this Python version), a one-line comment explaining it would help future readers; otherwise the cleaner fix is to usestr | Noneand drop both the import and the noqa.- Minor, unaddressed CodeRabbit nits in the skill snippets (low impact since these are agent reference docs, not executed library code, but easy fixes):
extract_gpqa_score/extract_scoreinrecipes/tasks/gpqa.mdandrecipes/tasks/scicode.mduse bareopen(path)instead of awithcontext manager and crash withIndexErrorif invoked without args.
No blocking correctness issues in the production code itself.
|
|
||
| """Slurm configuration and factory for the ModelOpt Launcher.""" | ||
|
|
||
| # ruff: noqa: UP045 |
There was a problem hiding this comment.
Bot comment.
Style: the rest of the codebase uses PEP-604 str | None rather than Optional[str]. Adding from typing import Optional plus a # ruff: noqa: UP045 suppression here is inconsistent. If nemo_run's @run.cli.factory / @run.autoconvert decorators can't introspect X | None annotations and that's why you reverted, please leave a one-line comment to that effect — otherwise switching to str | None and dropping both the import and the noqa would match the rest of the project.
There was a problem hiding this comment.
There was a incompatibility between ruff and NeMo run launcher. Codex picked this as a happy middle ground. I can add a comment to reflect that.
| Prefer the `pass@1[avg-of-N]` metric matching the configured repeat count. | ||
| If the repeat count is unknown, use the highest available `avg-of-N`. | ||
|
|
||
| ```python |
There was a problem hiding this comment.
Bot comment.
Following up on @cjluo-nv's question ("qq why we still need these python code here?"): the same inlined extract_*_score Python snippets now appear in gpqa.md, scicode.md, mmlu_pro.md, mmmu_pro.md, ns_hle_aa.md, and aa_lcr.md as well. Could you reply on the PR with the rationale (e.g. agents copy-paste these into a session vs. an importable helper module under recipes/)? Worth resolving before merge so it's not re-asked on every future task recipe added.
There was a problem hiding this comment.
Answered above.
chadvoegele
changed the title
| "score_key": "subtask_accuracy", | ||
| "stderr_scale": "subtasks", | ||
| }, | ||
| "gpqa": { |
|
Did a design-doc alignment pass on this PR (vs the ModelOpt Agent Skills Design). Strong overall alignment — the PTQ pre-flight sanity check, post-quant validation as a required gate, NEL timeout/resume framing, per-session monitor registry, and the recipe-yaml → reference-md restructure are all faithful applications of the doc's principles (lazy-loading via references, domain-skill verify-once gate, tool-affordance shape). Two design-level questions that feel worth a decision rather than a stealth merge: 1.
|
Signed-off-by: Chad Voegele <cvoegele@nvidia.com>
Signed-off-by: Chad Voegele <cvoegele@nvidia.com>
Signed-off-by: Chad Voegele <cvoegele@nvidia.com>
Signed-off-by: Chad Voegele <cvoegele@nvidia.com>
Signed-off-by: Chad Voegele <cvoegele@nvidia.com>
Signed-off-by: Chad Voegele <cvoegele@nvidia.com>
Signed-off-by: Chad Voegele <cvoegele@nvidia.com>
Signed-off-by: Chad Voegele <cvoegele@nvidia.com>
Signed-off-by: Chad Voegele <cvoegele@nvidia.com>
Signed-off-by: Chad Voegele <cvoegele@nvidia.com>
Signed-off-by: Chad Voegele <cvoegele@nvidia.com>
Signed-off-by: Chad Voegele <cvoegele@nvidia.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Chad Voegele <cvoegele@nvidia.com>
Signed-off-by: Chad Voegele <cvoegele@nvidia.com>
Signed-off-by: Chad Voegele <cvoegele@nvidia.com>
Signed-off-by: Chad Voegele <cvoegele@nvidia.com>
Signed-off-by: Chad Voegele <cvoegele@nvidia.com>
Signed-off-by: Chad Voegele <cvoegele@nvidia.com>
Signed-off-by: Chad Voegele <cvoegele@nvidia.com>
Signed-off-by: Chad Voegele <cvoegele@nvidia.com>
Signed-off-by: Chad Voegele <cvoegele@nvidia.com>
Signed-off-by: Chad Voegele <cvoegele@nvidia.com>
Signed-off-by: Chad Voegele <cvoegele@nvidia.com>
Signed-off-by: Chad Voegele <cvoegele@nvidia.com>
chadvoegele
force-pushed
the
cvoegele/agent_evals
branch
from
377e6c9 to
6d39913
Compare
cjluo-nv
left a comment
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review — DM the bot to share feedback.
Re-review focused on the per-task .md files converted from YAML.
What's improved since last pass:
- 💬 The full inline
extract_*_scorePython functions that prompted @cjluo-nv's "qq why we still need these python code here?" have been replaced with short dotted YAML score paths (e.g.results.groups.gpqa.metrics."pass@1[avg-of-N]".scores.symbolic_correct.value). Adding new evals no longer requires hand-writing a parser, which addresses the follow-up "will this make adding new evals harder" concern — good. - 💬
tools/launcher/slurm_config.pynow has a top-of-file comment explaining the nemo_run / PEP-604 incompatibility, as @chadvoegele said he'd add. TheOptional[...]+# ruff: noqa: UP045is now self-documenting. - 💬 scicode "why is gpqa here?" — confirmed gone.
Still warrants a human eye before merge:
- Likely bug in
gpqa.md(n_samples: 32): every other NeMo-Skillsns_*task in this PR (aime2025, ifbench, livecodebench, mmlu_pro, scicode) and the in-repoexamples/pruning/minitron/.../nemo_evaluator.yamlusenum_repeats:for ns_gpqa. The deletedgpqa.yamlalso usednum_repeats: 5, and the companionrecipes/examples/example_eval.yamlin this PR usesnum_repeats: 32. Switching only the.mdfragment ton_samples: 32looks like a copy-paste from the AA-LCR fragment and will silently fall back to the harness default repeat count instead of running 32 repeats. Inline comment below. tau2_bench_telecom.mdScore Extraction is still empty — header line followed by EOF, no content. CodeRabbit flagged this last round and it's unresolved. Inline comment below.- SKILL.md Step 9 references a Python snippet that no longer exists. Line says "use the
Score ExtractionPython snippet from the matching task reference inrecipes/tasks/<task>.md" — but the conversion just removed those Python snippets in favor of YAML paths. Worth updating the SKILL.md wording so the agent looks for the dotted path, not a Python function. Inline comment below. - 💬 PR is still WIP-titled with unchecked PR-body TODOs ("Did you update Changelog: TODO", "Did you get Claude approval on this PR: TODO"); @chadvoegele's own description still says "I'm running the full loop ... iterating on skills". Worth confirming readiness with the author before landing.
No issues with the production code (tools/launcher/{core,slurm_config}.py) — qos plumbing is small, optional, backward-compatible, and matches the existing SlurmConfig shape.
| - name: ns_gpqa | ||
| nemo_evaluator_config: | ||
| config: | ||
| params: |
There was a problem hiding this comment.
Bot comment.
Likely bug: this should be num_repeats: 32, not n_samples: 32. Every other NeMo-Skills ns_* fragment in this PR (aime2025/ifbench/livecodebench/mmlu_pro/scicode) uses num_repeats: under extra:, and the existing in-repo examples/pruning/minitron/NVIDIA-Nemotron-Nano-9B-v2/nemo_evaluator.yaml configures ns_gpqa with num_repeats: 8. The companion recipes/examples/example_eval.yaml in this PR also uses num_repeats: 32 for ns_gpqa, and the deleted gpqa.yaml used num_repeats: 5. As written, the harness will not see a recognized 32-repeats knob here and will silently fall back to its default — and the score-extraction section just below assumes pass@1[avg-of-N] exists, which only shows up when repeats actually ran. (Note: AA-LCR uses n_samples: 3 legitimately because it's a different harness/container; that's not the precedent for ns_gpqa.)
| api_key: USER_API_KEY | ||
| ``` | ||
|
|
||
| ## Score Extraction |
There was a problem hiding this comment.
Bot comment.
## Score Extraction is still an empty section (header followed by EOF). CodeRabbit flagged this last round. SKILL.md Step 9 instructs the agent to harvest scores using the matching task reference's Score Extraction guidance, so leaving this empty defeats the new gating step for tau2_bench_telecom runs. Either fill in the dotted path (e.g. results.groups.tau2_bench_telecom.metrics.pass_1.scores.<...> — please verify against an actual results.yml) or drop the empty header so it doesn't read as a TODO.
| 4. Confirm the number of evaluated/scored samples matches for each task and split. | ||
| 5. Confirm the same accuracy metric/score field is used for the baseline and quantized comparison. | ||
|
|
||
| Report the comparability summary alongside the score: prompt/template status, generation-setting status, sample-count status, reasoning-handling status, and the exact score field used. If any item differs, either rerun with matched settings or label the result as not an apples-to-apples quantization comparison. |
There was a problem hiding this comment.
Bot comment.
This sentence says "use the Score Extraction Python snippet from the matching task reference", but the converted recipes/tasks/*.md files no longer contain Python snippets — they document a dotted YAML path into results.yml. Recommend updating to something like "use the Score Extraction field path documented in the matching task reference in recipes/tasks/<task>.md" so the agent doesn't go hunting for a function that isn't there. (Same wording change probably worth doing wherever else the SKILL refers back to the Python snippet form.)
Signed-off-by: Chad Voegele <cvoegele@nvidia.com>
Signed-off-by: Chad Voegele <cvoegele@nvidia.com>
Signed-off-by: Chad Voegele <cvoegele@nvidia.com>
| @@ -0,0 +1,22 @@ | |||
| --- | |||
There was a problem hiding this comment.
Is this really a skill or just experience cache? If this is an experience cache, can we avoid promoting it to be a skill?
| @@ -0,0 +1,22 @@ | |||
| --- | |||
| name: debugging-playbooks | |||
There was a problem hiding this comment.
this name might be fused with debugger skill exits before this PR
Signed-off-by: Chad Voegele <cvoegele@nvidia.com>
Signed-off-by: Chad Voegele <cvoegele@nvidia.com>
Signed-off-by: Chad Voegele <cvoegele@nvidia.com>
cjluo-nv
left a comment
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review — DM the bot to share feedback.
Re-review. Production code (tools/launcher/{core,slurm_config}.py) is small, optional, backward-compatible, and looks fine. Most prior bot/reviewer items are addressed:
- 💬
gpqa.mdn_samples→num_repeats: 32— fixed (commit ack'd by author). - 💬
scicode.mdstray "gpqa" — fixed ("It's gone now"). - 💬 SKILL.md Step 9 wording — updated to reference the Score Extraction section (dotted YAML path) instead of a Python snippet that no longer exists.
- 💬
slurm_config.pyOptional[str]+# ruff: noqa: UP045— author added a top-of-file comment ("nemo_run's CLI parser cannot introspect PEP 604 optional annotations here"). - 💬 cjluo-nv's "qq why we still need these python code here?" — resolved by replacing inline
extract_*_scorePython with short dotted YAML score paths in eachrecipes/tasks/*.md.
Still warrants a human eye before merge:
- Unresolved design-review questions from @Edwardf0t1 (alignment vs. the ModelOpt Agent Skills Design doc).
debugging-playbooksas a new top-level skill isn't in the shipped-skills list and the doc explicitly flags "reference file sprawl" as a risk; @cjluo-nv separately asked "Is this really a skill or just experience cache?" and noted the name may collide with the existingdebugskill. Neither has been replied to. Picking (a) keep top-level + update design doc, vs. (b) fold intodebug/references/playbooks/, is a design call worth a human sign-off. - Step 10 /
compare-resultsscope vs.evaluationskill scope —compare-results/is added in this PR butevaluation/SKILL.mdalso still grows comparability/Step-9 logic (+143/−48). @Edwardf0t1 flagged that evaluation is drifting past the lazy-loading target. Worth a human decision on the boundary. recipes/tasks/tau2_bench_telecom.mdScore Extraction is still a literalTODO. SKILL.md Step 9 explicitly routes the agent to the matching task reference for canonical score fields, so for tau2_bench_telecom that gating step currently has no content. Author marked it as TODO rather than fixing — acknowledged hole, but a human should decide whether to land the task fragment as TODO or hold it.- PR readiness is ambiguous. Title was previously "WIP:"-prefixed; PR body still has unchecked TODOs ("Did you update Changelog: TODO", "Did you get Claude approval on this PR: TODO"), and @chadvoegele's own description still says "I'm running the full loop... iterating on skills". Worth confirming with the author that this is the intended landing snapshot.
No blocking correctness issues in the production code itself; the open items are scope/design and one explicit TODO in a documentation fragment.
Additional comments (outside the PR diff):
.claude/skills/evaluation/SKILL.md:379— > Bot comment.
@Edwardf0t1 flagged that evaluation/SKILL.md is drifting from the lazy-loading target (~80–120 lines), with NEL timeout/resume + Step 9 verification + comparability detail all inlined. Now that compare-results/ exists in this PR, consider extracting Step 9 / 10 detail into evaluation/references/run-verification.md and routing comparability fully to compare-results, so the dispatch tree in SKILL.md stays small. Not blocking, but worth a human pass before this hardens.
|
|
||
| ## Score Extraction | ||
|
|
||
| TODO |
There was a problem hiding this comment.
Bot comment.
Score Extraction is a literal TODO. SKILL.md Step 9 explicitly routes the agent to the matching task reference for canonical score harvesting, so for tau2_bench_telecom the gating step currently has nothing actionable to point at. Either fill in the dotted path against an actual results.yml (likely under results.groups.tau2_bench_telecom.metrics.pass_1.scores...) before merge, or drop the empty section so it doesn't look like a hole the agent will silently skip.
| @@ -0,0 +1,22 @@ | |||
| --- | |||
There was a problem hiding this comment.
Bot comment.
Two reviewer threads about this skill are unanswered:
- @cjluo-nv: "Is this really a skill or just experience cache? If this is an experience cache, can we avoid promoting it to be a skill?" and "this name might be fused with debugger skill exits before this PR".
- @Edwardf0t1: this skill isn't in the ModelOpt Agent Skills Design doc's shipped-skills list (ptq / deployment / evaluation / monitor / launching-evals / accessing-mlflow / debug + PLANNED compare-results), and the doc explicitly flags "reference file sprawl" as a risk. Suggested either (a) keep top-level + update the design doc, or (b) fold into
debug/references/playbooks/.
Please respond with the design rationale before landing — at minimum a reply explaining why this isn't a debug/references/ entry and how it differs from the existing debug skill.
Signed-off-by: Chad Voegele <cvoegele@nvidia.com>
Signed-off-by: Chad Voegele <cvoegele@nvidia.com>
Edwardf0t1
left a comment
Edwardf0t1
left a comment
There was a problem hiding this comment.
LGTM in general!
One thing we can address in a following PR is about evaluation/SKILL.md lazy-loading drift — extract the NEL timeout/resume section + Step 9 / 10 detail into references; keep SKILL.md as a dispatch tree, which is a Claude skills design best practice.
4 participants
What does this PR do?
Type of change: bug fix
Usage
Ask Claude Code:
Testing
I'm running the full loop with the above prompt, and iterating on skills to resolve undesired agent behavior.
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 (e.g. avoiding hardcoded
trust_remote_code=True,torch.load(..., weights_only=False),pickle, etc.).CONTRIBUTING.md: ✅Additional Information
See trials log for details.
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests