fix(skills): Update Agent Skills Based on Observed Failures Modes in Trials#1945
Conversation
Signed-off-by: Chad Voegele <cvoegele@nvidia.com>
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR updates internal skill documentation across compare-results, day0-release, evaluation, ptq, and quant-recipe-search. It adds external baseline sanity gating, adjusts SciCode task config values, expands PTQ deployment readiness checks, and documents a layer-position exclusion axis for quantization recipe search. ChangesExternal Baseline Sanity Check Workflow
SciCode Task Configuration Update
PTQ Calibration and Deployment Readiness Gate
Layer-Position Exclusion Axis for Recipe Search
Estimated code review effort: 2 (Simple) | ~12 minutes 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
chadvoegele
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1945 +/- ##
==========================================
- Coverage 77.73% 77.26% -0.48%
==========================================
Files 519 519
Lines 57886 58521 +635
==========================================
+ Hits 45000 45214 +214
- Misses 12886 13307 +421
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 — DM the bot to share feedback.
Documentation-only PR (+200/-40, 9 files) updating existing agent-skill markdown based on the June-5 day-0 trial failures. No code/tests, no licensing changes (Apache-2.0 frontmatter is pre-existing and untouched).
Design gate: The complexity gate fired on directory span, but this isn't an architectural change — it's guidance-text edits to existing skills. The one new concept (the "External Baseline Sanity Check" procedure in run-validation.md, wired into compare-results, day0-release, and quant-recipe-search) is a documented process, not a new code subsystem/DSL/registry, so there's no in-repo system being duplicated. Design protocol effectively N/A.
Correctness / consistency verified:
- SciCode:
num_repeats: 1+ score fieldavg-of-1are internally consistent. - day0-release: MLflow baseline reuse/caching removed from both the checklist and Step 3 text (consistent), new
EXTERNAL_BASELINE_MISMATCHclass added to Triage/Step 5/run-validation consistently, andNEEDS_HUMANremoval leaves no dangling references (confirmed via search). - Factual references check out against the actual code:
nemotron-post-training-v3(dataset_utils.py),--calib_with_images, and the VLM default subsetssparsetables/plotqa_cot/wiki_enexactly matchdefault_subsetsinvlm_dataset_utils.py.
No prompt-injection: the directive-style text in the diff ("Do NOT use…", "Always run a fresh baseline") is the skill content under review, not an attempt to steer this review.
Why nudge (owner should confirm two embedded policy calls):
- Step 3 now forces a fresh baseline eval every run (removing MLflow baseline reuse). This is a deliberate cost/accuracy trade-off worth a human sign-off — it increases per-run compute.
- The External Baseline Sanity Check hardcodes an "~5 percentage points" pass threshold; a human should confirm that heuristic is right for the AA suite.
PR body notes it was not Claude-approved and will be validated by re-running day-0 trials, so a human review before merge is appropriate.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
Actionable comments posted: 3
🤖 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/compare-results/SKILL.md:
- Around line 88-102: Clarify the `externally unverified` report shape in the
comparison schema so it is explicit which fields are null or empty when no
credible external score exists. Update the relevant section in `SKILL.md` that
defines the per-task external reference fields to spell out the expected values
for source URL, known protocol differences, percentage-point difference, and
sanity status when the baseline is externally unverified, so valid output does
not appear malformed.
In @.agents/skills/evaluation/SKILL.md:
- Line 412: The current wording in the run-validation guidance makes the
External Baseline Sanity Check sound like a prerequisite for baseline validation
success; rephrase the instruction in SKILL.md so that `validate the run` and the
baseline’s completed-run checks remain the success criteria, while the `External
Baseline Sanity Check` is clearly described as only gating the handoff to
`compare-results`. Keep the references to `run-validation.md`,
`compare-results`, and the baseline/candidate flow so the separation between
validation and delta comparison is unambiguous.
In @.agents/skills/ptq/references/checkpoint-validation.md:
- Around line 3-10: The checkpoint-validation contract currently mixes the
machine-enforced PTQ gate with the manual serving canary, which makes it look
like readiness is fully enforced when it is not. Update the guidance in the
checkpoint-validation content to separate the canary from the required gate:
either make the canary explicitly a manual post-gate step, or expand the
executable gate so the canary is part of the enforced workflow; keep the core
gate focused on the validation checks described in the checkpoint-validation
section and preserve the inherited workspace/path and compatibility inventory
references.
🪄 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: 3994feff-b5c5-4e08-9939-3fb3f1d57df0
📒 Files selected for processing (9)
.agents/skills/compare-results/SKILL.md.agents/skills/day0-release/SKILL.md.agents/skills/evaluation/SKILL.md.agents/skills/evaluation/recipes/tasks/aa/scicode.md.agents/skills/evaluation/references/run-validation.md.agents/skills/ptq/SKILL.md.agents/skills/ptq/references/checkpoint-validation.md.agents/skills/quant-recipe-search/SKILL.md.agents/skills/quant-recipe-search/references/recipe_iteration.md
| Include: | ||
|
|
||
| - Baseline and candidate identifiers. | ||
| - Per-task metric path, baseline score, candidate score, delta, and stderr if | ||
| available. | ||
| - Per-task external reference score, source URL, known protocol differences, | ||
| percentage-point difference, and sanity status (`verified`, `failed`, or | ||
| `externally unverified`). | ||
| - Comparability status for prompt/template, generation settings, sample counts, | ||
| reasoning handling, judge/simulator setup, and score field. | ||
| - Comparability verdict: comparable, not comparable, or inconclusive. | ||
| - Quantization feasibility verdict: acceptable, not acceptable, or inconclusive. | ||
| Never report `acceptable` when external baseline sanity failed. An externally | ||
| unverified baseline does not block `acceptable`; apply the candidate-delta | ||
| gate and report the missing external corroboration. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '70,120p' .agents/skills/compare-results/SKILL.md | cat -nRepository: NVIDIA/Model-Optimizer
Length of output: 2056
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- SKILL.md excerpt ---\n'
sed -n '80,115p' .agents/skills/compare-results/SKILL.md | cat -n
printf '\n--- external-shape references ---\n'
rg -n "externally unverified|external reference score|known protocol differences|percentage-point difference|sanity status" .agents/skills/compare-results/SKILL.mdRepository: NVIDIA/Model-Optimizer
Length of output: 251
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- SKILL.md surrounding section ---'
sed -n '1,180p' .agents/skills/compare-results/SKILL.md | cat -n
printf '\n%s\n' '--- references to run-validation ---'
rg -n "run-validation|externally unverified|sanity status|external reference score|known protocol differences|percentage-point difference" .agents/skills -SRepository: NVIDIA/Model-Optimizer
Length of output: 9926
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '45,90p' .agents/skills/evaluation/references/run-validation.md | cat -nRepository: NVIDIA/Model-Optimizer
Length of output: 2981
Clarify the externally unverified shape.
When no credible external score exists, spell out which fields are null/empty so a valid report doesn’t look malformed.
Suggested schema tweak
- - Per-task external reference score, source URL, known protocol differences, percentage-point difference, and sanity status (`verified`, `failed`, or `externally unverified`).
+ - Per-task external reference score (`null` if externally unverified), source URL (`null` if externally unverified), known protocol differences (`[]` if none), percentage-point difference (`null` if externally unverified), and sanity status (`verified`, `failed`, or `externally unverified`).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Include: | |
| - Baseline and candidate identifiers. | |
| - Per-task metric path, baseline score, candidate score, delta, and stderr if | |
| available. | |
| - Per-task external reference score, source URL, known protocol differences, | |
| percentage-point difference, and sanity status (`verified`, `failed`, or | |
| `externally unverified`). | |
| - Comparability status for prompt/template, generation settings, sample counts, | |
| reasoning handling, judge/simulator setup, and score field. | |
| - Comparability verdict: comparable, not comparable, or inconclusive. | |
| - Quantization feasibility verdict: acceptable, not acceptable, or inconclusive. | |
| Never report `acceptable` when external baseline sanity failed. An externally | |
| unverified baseline does not block `acceptable`; apply the candidate-delta | |
| gate and report the missing external corroboration. | |
| Include: | |
| - Baseline and candidate identifiers. | |
| - Per-task metric path, baseline score, candidate score, delta, and stderr if | |
| available. | |
| - Per-task external reference score (`null` if externally unverified), source URL (`null` if externally unverified), known protocol differences (`[]` if none), percentage-point difference (`null` if externally unverified), and sanity status (`verified`, `failed`, or `externally unverified`). | |
| - Comparability status for prompt/template, generation settings, sample counts, | |
| reasoning handling, judge/simulator setup, and score field. | |
| - Comparability verdict: comparable, not comparable, or inconclusive. | |
| - Quantization feasibility verdict: acceptable, not acceptable, or inconclusive. | |
| Never report `acceptable` when external baseline sanity failed. An externally | |
| unverified baseline does not block `acceptable`; apply the candidate-delta | |
| gate and report the missing external corroboration. |
🤖 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 @.agents/skills/compare-results/SKILL.md around lines 88 - 102, Clarify the
`externally unverified` report shape in the comparison schema so it is explicit
which fields are null or empty when no credible external score exists. Update
the relevant section in `SKILL.md` that defines the per-task external reference
fields to spell out the expected values for source URL, known protocol
differences, percentage-point difference, and sanity status when the baseline is
externally unverified, so valid output does not appear malformed.
| ### Step 9 — Verify completed run | ||
|
|
||
| Before pulling/reporting scores, validate the run. Read `references/run-validation.md` for NEL timeout/resume behavior, completed-run validation, diagnostics, score harvesting, and the handoff to `compare-results` for baseline-vs-candidate deltas. | ||
| Before pulling/reporting scores, validate the run. Read `references/run-validation.md` for NEL timeout/resume behavior, completed-run validation, diagnostics, and score harvesting. For a baseline that will be compared with a candidate, also perform its **External Baseline Sanity Check** before a success verdict, then hand the validated runs to `compare-results` for baseline-vs-candidate deltas. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant files and inspect the referenced sections.
git ls-files '.agents/skills/evaluation/SKILL.md' 'references/run-validation.md'
echo '--- SKILL.md around line 412 ---'
sed -n '395,430p' .agents/skills/evaluation/SKILL.md
echo '--- run-validation.md relevant excerpts ---'
sed -n '1,260p' references/run-validation.mdRepository: NVIDIA/Model-Optimizer
Length of output: 1539
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# If the previous read is too broad, print just the baseline/validation sections.
rg -n -C 3 'externally unverified|success verdict|External Baseline Sanity Check|compare-results|validation|baseline' .agents/skills/evaluation/SKILL.md references/run-validation.mdRepository: NVIDIA/Model-Optimizer
Length of output: 7203
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the actual validation guidance file and the External Baseline Sanity Check wording.
fd -a 'run-validation.md|*validation*' .
rg -n -C 3 'External Baseline Sanity Check|externally unverified|success verdict|compare-results|baseline' .agents references . 2>/dev/null | head -n 200Repository: NVIDIA/Model-Optimizer
Length of output: 488
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the validation guidance and the exact baseline wording.
git ls-files | rg 'run-validation|validation|SKILL\.md|compare-results|baseline'
echo '--- search for key phrases ---'
rg -n -C 3 'External Baseline Sanity Check|externally unverified|success verdict|compare-results|baseline' . 2>/dev/null | head -n 250Repository: NVIDIA/Model-Optimizer
Length of output: 35639
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the validation guide directly and pull the baseline-related contract.
sed -n '1,260p' .agents/skills/evaluation/references/run-validation.md
echo '--- targeted phrases ---'
rg -n -C 3 'externally unverified|External Baseline Sanity Check|success verdict|compare-results|baseline' .agents/skills/evaluation/references/run-validation.mdRepository: NVIDIA/Model-Optimizer
Length of output: 10469
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# If the document is long, read the specific section around any baseline mention.
rg -n 'External Baseline Sanity Check|externally unverified|success verdict|compare-results|baseline' .agents/skills/evaluation/references/run-validation.mdRepository: NVIDIA/Model-Optimizer
Length of output: 1357
Clarify the baseline sanity-check handoff
The current wording makes the External Baseline Sanity Check sound like part of baseline-validation success. Reword it so the check only gates the compare-results handoff.
Proposed wording fix
- For a baseline that will be compared with a candidate, also perform its External Baseline Sanity Check before a success verdict, then hand the validated runs to compare-results for baseline-vs-candidate deltas.
+ For a baseline that will be compared with a candidate, also perform its External Baseline Sanity Check before handing the validated runs to compare-results for baseline-vs-candidate deltas.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Before pulling/reporting scores, validate the run. Read `references/run-validation.md` for NEL timeout/resume behavior, completed-run validation, diagnostics, and score harvesting. For a baseline that will be compared with a candidate, also perform its **External Baseline Sanity Check** before a success verdict, then hand the validated runs to `compare-results` for baseline-vs-candidate deltas. | |
| Before pulling/reporting scores, validate the run. Read `references/run-validation.md` for NEL timeout/resume behavior, completed-run validation, diagnostics, and score harvesting. For a baseline that will be compared with a candidate, also perform its **External Baseline Sanity Check** before handing the validated runs to `compare-results` for baseline-vs-candidate deltas. |
🤖 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 @.agents/skills/evaluation/SKILL.md at line 412, The current wording in the
run-validation guidance makes the External Baseline Sanity Check sound like a
prerequisite for baseline validation success; rephrase the instruction in
SKILL.md so that `validate the run` and the baseline’s completed-run checks
remain the success criteria, while the `External Baseline Sanity Check` is
clearly described as only gating the handoff to `compare-results`. Keep the
references to `run-validation.md`, `compare-results`, and the baseline/candidate
flow so the separation between validation and delta comparison is unambiguous.
| Before treating an exported checkpoint as ready for deployment/evaluation, verify checkpoint size/bits, quantized-weight coverage, metadata consistency, and serving readiness. This is a gate, not a guideline: do not submit evals, start a production serving job, or mark the checkpoint ready until all required checks, including the serving canary, pass and the validation report is recorded. | ||
|
|
||
| ## Required checks | ||
|
|
||
| 1. The quantized checkpoint is smaller on disk than the baseline/source checkpoint and has lower estimated bits per weight. Record source size, output size, and output/source ratio. A partial-quantization recipe may not shrink every tensor, but it should still match the intended quantization coverage. If the size reduction is small or missing, explain why before proceeding. | ||
| 2. The weights that were actually quantized match what the requested qformat/recipe/config targeted. Record layer precision counts grouped by actual/declarative precision, such as NVFP4, FP8, INT4, BF16/unquantized excluded, unexpected unquantized, and declaration mismatches. Quantization config patterns may silently miss layers if the model uses non-standard naming — this only surfaces later as deployment failures when the serving framework tries to load unquantized weights as quantized. | ||
| 3. Metadata that should not change still matches the baseline/source model. Compare generation settings, tokenizer files, chat template, model architecture fields, max positions/context length, and special tokens; quantization should affect weights and quantization metadata, not silently change prompting or generation behavior. Record every diff and classify it as expected or blocking. | ||
| 4. The checkpoint is ready for downstream deployment and evaluation. Record the exact checkpoint workspace and path that both downstream skills must inherit. Inventory every model-compatibility change made during PTQ that may be required to load or serve the checkpoint: dependency upgrades, source patches, custom code, environment variables, and launcher or container changes. Record each category separately and write `none` for every category with no changes. Then invoke the **deployment** skill and use that same workspace, checkpoint path, and compatibility inventory to serve the checkpoint in the intended deployment environment. Run a canary query such as `What is the capital of France?` and require a valid response (for this example, one that identifies Paris). Record the target environment, serving framework and launch configuration, canary query, and response. Stop the canary service after validation unless the user asked to keep it running. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Separate the manual canary from the required gate.
The day-0 gate still only mirrors checks 1-3, so this now reads like a machine-enforced blocker that the current contract cannot actually verify. Please either mark the serving canary as a manual post-gate step here or wire it into the executable gate; otherwise downstream users can assume deployment readiness is enforced when it is not.
Suggested wording
-4. The checkpoint is ready for downstream deployment and evaluation. Record the exact checkpoint workspace and path that both downstream skills must inherit. Inventory every model-compatibility change made during PTQ that may be required to load or serve the checkpoint: dependency upgrades, source patches, custom code, environment variables, and launcher or container changes. Record each category separately and write `none` for every category with no changes. Then invoke the **deployment** skill and use that same workspace, checkpoint path, and compatibility inventory to serve the checkpoint in the intended deployment environment. Run a canary query such as `What is the capital of France?` and require a valid response (for this example, one that identifies Paris). Record the target environment, serving framework and launch configuration, canary query, and response. Stop the canary service after validation unless the user asked to keep it running.
+4. After checks 1-3 pass, manually run the **deployment** skill with the recorded workspace, checkpoint path, and compatibility inventory, then validate the serving canary before treating the checkpoint as ready for deployment/evaluation.Also applies to: 21-34
🤖 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 @.agents/skills/ptq/references/checkpoint-validation.md around lines 3 - 10,
The checkpoint-validation contract currently mixes the machine-enforced PTQ gate
with the manual serving canary, which makes it look like readiness is fully
enforced when it is not. Update the guidance in the checkpoint-validation
content to separate the canary from the required gate: either make the canary
explicitly a manual post-gate step, or expand the executable gate so the canary
is part of the enforced workflow; keep the core gate focused on the validation
checks described in the checkpoint-validation section and preserve the inherited
workspace/path and compatibility inventory references.
Edwardf0t1
left a comment
Edwardf0t1
left a comment
There was a problem hiding this comment.
Three follow-up notes from the review — all guidance/cost trade-offs rather than defects, non-blocking.
| 1. The quantized checkpoint is smaller on disk than the baseline/source checkpoint and has lower estimated bits per weight. Record source size, output size, and output/source ratio. A partial-quantization recipe may not shrink every tensor, but it should still match the intended quantization coverage. If the size reduction is small or missing, explain why before proceeding. | ||
| 2. The weights that were actually quantized match what the requested qformat/recipe/config targeted. Record layer precision counts grouped by actual/declarative precision, such as NVFP4, FP8, INT4, BF16/unquantized excluded, unexpected unquantized, and declaration mismatches. Quantization config patterns may silently miss layers if the model uses non-standard naming — this only surfaces later as deployment failures when the serving framework tries to load unquantized weights as quantized. | ||
| 3. Metadata that should not change still matches the baseline/source model. Compare generation settings, tokenizer files, chat template, model architecture fields, max positions/context length, and special tokens; quantization should affect weights and quantization metadata, not silently change prompting or generation behavior. Record every diff and classify it as expected or blocking. | ||
| 4. The checkpoint is ready for downstream deployment and evaluation. Record the exact checkpoint workspace and path that both downstream skills must inherit. Inventory every model-compatibility change made during PTQ that may be required to load or serve the checkpoint: dependency upgrades, source patches, custom code, environment variables, and launcher or container changes. Record each category separately and write `none` for every category with no changes. Then invoke the **deployment** skill and use that same workspace, checkpoint path, and compatibility inventory to serve the checkpoint in the intended deployment environment. Run a canary query such as `What is the capital of France?` and require a valid response (for this example, one that identifies Paris). Record the target environment, serving framework and launch configuration, canary query, and response. Stop the canary service after validation unless the user asked to keep it running. |
There was a problem hiding this comment.
Mandatory serving canary → cost multiplier under recipe search. This makes group 4 (invoke deployment, serve, run a canary) a required gate for every PTQ hand-off. But quant-recipe-search delegates PTQ + validation per candidate, so during a sweep this implies a full deployment per candidate — serializing the search on a deploy each time. Consider scoping the mandatory canary to the promoted/day-0 checkpoint and marking it optional during exploratory candidate generation (or stating that intent explicitly), so the recipe-search loop does not pay a deploy per candidate.
There was a problem hiding this comment.
Ya, we should make sure the quantized checkpoint can be deployed, otherwise we need to choose a new recipe.
| exists in MLflow (same model, task set, sampling params), reuse it and skip this | ||
| stage. Otherwise run it via the **evaluation** skill (which deploys the source | ||
| model itself). Gate with `gate_run.py`. | ||
| sampling params. Always run a fresh baseline via the **evaluation** skill, |
There was a problem hiding this comment.
Dropping MLflow baseline reuse increases every run's compute/latency. This flips Step 3 from "look it up first, reuse if cached" to "always run a fresh baseline." Defensible as correctness-over-speed given the trials, but it is a real regression for repeated day-0 runs on the same source model. Worth a one-line rationale here so a future reader does not "re-optimize" the reuse path back in.
There was a problem hiding this comment.
Agree it's more inefficient, but it's what we do in practice anyways to verify correctness of the evals.
| ``` | ||
|
|
||
| Treat a credible comparable result as verified only when the absolute | ||
| difference is approximately 5 percentage points or less. A difference |
There was a problem hiding this comment.
The "~5 percentage-point" external threshold is a hardcoded heuristic that can misfire. For many benchmarks no published external run shares the protocol (template / thinking-mode / sample-count differ), so a legitimate baseline can land >5pp from the only reference and trip failed → ANOMALOUS → block comparison. The section does soften this (prefer externally unverified, which is non-blocking), but consider tightening the wording so the default when protocols are not closely matched is externally unverified, reserving failed for a genuinely comparable protocol that still shows a large gap.
There was a problem hiding this comment.
I took 5 percentage points from product guidance.
Shared metric exists and internal baseline is within +/- 5 pp absolute
| - [ ] Step 1: Setup gate — creds present, cluster reachable | ||
| - [ ] Step 2: PTQ (ptq skill) → gate_ptq.py | ||
| - [ ] Step 3: Baseline eval (evaluation skill, deploys source) → gate_run.py [skip if cached, see below] | ||
| - [ ] Step 3: Baseline eval (evaluation skill, deploys source) → gate_run.py |
There was a problem hiding this comment.
Did the agent actually run skills and gate_run.py in day0-release?
There was a problem hiding this comment.
No, the day0-release and gate_run.py were added to the skills a few days after the first round of agent trials.
There was a problem hiding this comment.
I see, would be interesting to see if agent will use it in the next run.
| extra: | ||
| args: ++prompt_config=eval/scicode/default ++with_background=true | ||
| num_repeats: 8 | ||
| num_repeats: 1 |
There was a problem hiding this comment.
We only need 1 repeat for scicode?
There was a problem hiding this comment.
We need repeats to reduce variance, but when using num_repeats > 1 the evaluation will re-use the same code execution service. If there are any leaked sockets, threads, etc. you might eventually see a Resource temporarily unavailable or similar. It's more likely to be correct using a fresh code sandbox for every repeat by running scicode evaluation multiple times with num_repeats: 1.
There was a problem hiding this comment.
@chadvoegele , does this mean the agent needs to do the averaging itself with multiple runs of num_repeats: 1?
There was a problem hiding this comment.
I see, so I assume we need num_repeats: 1 for all benchmarks?
There was a problem hiding this comment.
No, this one is Scicode specific. See the explanation in #1945 (comment)
| Use when the user asks to "quantize a model", "run PTQ", "post-training | ||
| quantization", "NVFP4 quantization", "FP8 quantization", "INT8 | ||
| quantization", "INT4 AWQ", "quantize LLM", "quantize MoE", "quantize VLM", | ||
| or needs to produce a quantized HuggingFace or TensorRT-LLM checkpoint from a |
There was a problem hiding this comment.
Do we need TensorRT-LLM checkpoint here?
There was a problem hiding this comment.
Looks like that was there before so I didn't change it. I can remove it.
There was a problem hiding this comment.
I think TensorRT-LLM checkpoint is deprecated.
There was a problem hiding this comment.
I removed it from here
| - Delegate checkpoint generation and PTQ validation to `ptq`. | ||
| - Change one major axis at a time: format, calibration algorithm, module | ||
| selection, granularity, exclusions, or calibration data. | ||
| family, layer position, granularity, or calibration data. |
There was a problem hiding this comment.
So the agent went through the quant recipe search skills in our trial run, right?
There was a problem hiding this comment.
No, looks like quant-recipe-search was there when the trials were run but no agent used it. https://gitlab-master.nvidia.com/cvoegele/lab/-/blob/main/wiki/day0_jun5_analysis/skill_usage.md
There was a problem hiding this comment.
I see, I thought the changes in this PR are all related to the agent's behaviors in the first run, would be interesting to see if agent will use it in the next run.
There was a problem hiding this comment.
Ya, I'm hoping the agent can use the text of this skill to improve one of the models. But if the skill is still not being picked up, then we'll have to diagnose further.
Signed-off-by: Chad Voegele <cvoegele@nvidia.com>
| For SciCode, keep `num_repeats: 1` to limit sandbox workload. If variance is a | ||
| concern, run multiple independent matched baseline/candidate pairs instead of | ||
| increasing repeats within one run. |
There was a problem hiding this comment.
Why specific to scicode? and is this for faster recipe iteration?
There was a problem hiding this comment.
No, num_repeats: 1 is to help eliminate accuracy degradation due to code sandbox errors.
I did a little more research on it. For SciCode the architecture is:
nemo-skills container
├─ ns eval process
└─ sandbox server: local_sandbox_server (port 6000)
└─ worker process (lifetime: evaluation run)
Whereas for LiveCodeBench, it's:
nemo-skills container
└─ ns eval process
└─ LiveCodeBench evaluator workers (lifetime: one model generation)
With SciCode then any errors in the sandbox can accumulate during the evaluation, which might eventually lead to failures. Since LiveCodeBench is per generation, we don't have the same failure mode.
The newer additions SWE-bench Verified and Terminal Bench use yet another path of a Harbor eval image + a remote AWS Fargate execution container connected by an SSH tunnel.
| extra: | ||
| args: ++prompt_config=eval/scicode/default ++with_background=true | ||
| num_repeats: 8 | ||
| num_repeats: 1 |
There was a problem hiding this comment.
@chadvoegele , does this mean the agent needs to do the averaging itself with multiple runs of num_repeats: 1?
| exists in MLflow (same model, task set, sampling params), reuse it and skip this | ||
| stage. Otherwise run it via the **evaluation** skill (which deploys the source | ||
| model itself). Gate with `gate_run.py`. | ||
| sampling params. Always run a fresh baseline via the **evaluation** skill, |
There was a problem hiding this comment.
Why do we need a fresh baseline?
There was a problem hiding this comment.
I'm thinking for a few reasons:
- It will re-validate the particular evaluation setup used by the agent, i.e. config is right, NEL version is aligned, evaluation container is same, etc.
- I've not yet seen anyone (human or agent) go to MLFlow to fetch a previous baseline successfully.
- Typically we're doing new models anyways so there isn't a previous baseline.
There was a problem hiding this comment.
@chadvoegele , but for point 3, if the agent has already run a baseline once (not available before), we should be able to use that previous result right, like humans do
There was a problem hiding this comment.
Ya, the agent should re-use within a session.
Across different sessions, I think it would be better to re-validate. If we're re-running so much that it becomes a bottleneck, we can reassess.
|
5 participants
What does this PR do?
Type of change: documentation
Updates the agent skills based on findings from the June 5 Day-0 trials:
parallelism: 8andnum_repeats: 1.Refs:
Usage
N/A — agent skill guidance only.
Testing
git diff --check.I'll test the skill changes by re-running day 0 trials after merging.
Before your PR is "Ready for review"
CONTRIBUTING.md: N/AAdditional Information
The branch contains one signed commit and excludes WAAP state, test-fixture changes, and example changes.
Summary by CodeRabbit