[skill] evaluation: align nel-next TB2.1/SWE-bench with golden toolchain - #2063
Conversation
Terminal-Bench 2.1 configs generated from this skill drifted from the canonical eval-factory config (configs/benchmarks/terminal-bench-2.1/ bench.yaml). The scoring contract (playbook, repeats=8, timeout_strategy, run_timeout, concurrency) already matched; the toolchain and several proxy-level defaults did not. - nel-next.sh: install from the public upstream repo (github.com/NVIDIA-NeMo/Evaluator, default branch -> 0.4.0) instead of PyPI. PyPI nemo-evaluator tops out at 0.3.0 and cannot reach the 0.4.x toolchain golden runs on. NEL_NEXT_SPEC becomes the PyPI escape hatch and now takes precedence when explicitly set; NEL_NEXT_ORIGIN stays overridable from .env so internal mirrors stay out of this repo. - eval_image: document the golden pin 0.5.0.1-harbor (single source of truth: configs/shared/nel_next_containers.yaml) rather than 0.3.1.1-harbor as a floor. The TB2.1 task set is unchanged across those versions -- the vendored registry override has not moved since 2026-06-03 and both score 89 samples -- so this is a toolchain fix, not a benchmark change. - proxy.request_timeout 1800 -> 3600: it must be >= the solver's llm_kwargs.timeout (3600), otherwise the proxy truncates long agent turns the harness is still waiting on. - drop_params: add max_input_tokens_per_task and no_rebuild, which the 0.5.x harbor eval image sends and vLLM rejects unless stripped. - exclude_patterns: add model_traffic.jsonl so captured request bodies stay in the run dir and never reach MLflow. - add the http_pairs_dump interceptor (last in chain) for HTTP diagnostics. - document sharding: max_concurrent/sandbox.concurrency are per shard, so shards:N multiplies both serving capacity and live sandboxes. - .gitignore: broaden .env/.env-* to .env* so secret backups such as .env.bak-tb21 cannot be staged. Verified: nel-next.sh --version reports 0.4.0 built from Evaluator.git@9dcca2ae; both TB2.1 run configs pass `eval run --dry-run` on that CLI with 0.5.0.1-harbor in the generated sbatch. Signed-off-by: Chenjie Luo <chenjiel@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
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:
📝 WalkthroughWalkthroughThe PR updates nemo-evaluator installation defaults, AA-next evaluation settings, benchmark guidance, and ChangesAA-next evaluation configuration
Environment file ignore rules
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2063 +/- ##
==========================================
- Coverage 67.00% 64.76% -2.25%
==========================================
Files 520 522 +2
Lines 59545 64659 +5114
==========================================
+ Hits 39900 41874 +1974
- Misses 19645 22785 +3140
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:
|
Same golden cross-check as the TB2.1 change, against the canonical configs/benchmarks/swe-bench-verified/bench.yaml and a reference oci-hsg run (Qwen3.6-35B-A3B, pass@1 0.7012 over 500 tasks x r5). - fix the stale source-of-truth path (configs/benchmarks/nel_next/ swebench_verified/ -> configs/benchmarks/swe-bench-verified/) and point at the shared eval-image pin. - correct the interceptor ORDER: SWE-bench differs from TB2.1 -- http_pairs_dump is FIRST, and drop_params comes BEFORE consolidate_system. - turn_counter gains position: system_message. - drop_params: add max_input_tokens_per_task and no_rebuild. - exclude_patterns: add model_traffic.jsonl; document proxy.model_traffic.capture_request_body. - document that the instruction_template CONTENT is scoring-relevant, and that the public built-in in nemo_evaluator/templates/ is a DIFFERENT prompt whose results are not comparable to the canonical one. - state plainly that omitting the system_message interceptor is a scoring change, not a simplification. - reasoning_replay.mode is per MODEL, not per benchmark: think_tags (Qwen), native (GLM), omitted (MiniMax). Taking it from another model's config is a silent output-parsing bug. - max_concurrent: bench.yaml sets 15, but per-model leaves override it. - document sharding: concurrency is per shard; the reference run uses shards:10 for 500 x r5 = 2500 trials. Signed-off-by: Chenjie Luo <chenjiel@nvidia.com>
69c56cf to
821ed92
Compare
Follow-up to the two prior commits on this branch. They carried the reasoning behind each value (version history, task-set stability, sample counts) into the skill files. The skill's job is producing a correct config, and that rationale does not change any value it emits -- it belongs in the PR description, not in five files that will drift apart. Also drop the term "golden" throughout. It is internal shorthand for the reference eval-factory configs and means nothing to an external reader of this repo; the files now name the config or state the value directly. - nel-next.sh: 19-line install-source essay -> 5 lines. - references/nel-next.md: keep the value + the rejection rule for stale arch-suffixed tags; drop the version archaeology. - terminal_bench_2_1.md / swebench_verified.md: move settings into the existing task-values tables; keep only the non-obvious constraints (proxy timeout >= llm timeout, per-benchmark interceptor order, per-model reasoning_replay.mode, scoring impact of instruction_template and system_message). - swebench_verified.md: make the snippets use the canonical /configs/prompts/swebench_instruction.md consistently; the table and prose had been updated but the bash/yaml examples still showed the old hyphenated path. Net 128 -> 99 added lines. No config value changed. Signed-off-by: Chenjie Luo <chenjiel@nvidia.com>
Validation: end-to-end eval runs on the aligned configsConfigs generated from these skill changes were run end-to-end on aws-cmh (4x GB300
Both This exercises the changes in this PR specifically: the 0.5.0.1-harbor eval image, the Note: the checklist item "Did you get Claude approval on this PR?" is still open -- |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| NEL_NEXT_SPEC="${NEL_NEXT_SPEC:-nemo-evaluator[harbor,export]==0.3.*}" | ||
| NEL_NEXT_ORIGIN="${NEL_NEXT_ORIGIN:-}" | ||
| NEL_NEXT_SPEC="${NEL_NEXT_SPEC:-}" | ||
| NEL_NEXT_ORIGIN="${NEL_NEXT_ORIGIN:-git+https://github.com/NVIDIA-NeMo/Evaluator.git}" |
There was a problem hiding this comment.
[IMPORTANT Compatibility] The new default install is an unpinned git branch, which is weaker pinning than what it replaces and undercuts this PR's own goal.
Before: NEL_NEXT_SPEC defaulted to nemo-evaluator[harbor,export]==0.3.* — a version-constrained resolve that a user could tighten to an exact 0.3.x.
After: NEL_NEXT_ORIGIN defaults to git+https://github.com/NVIDIA-NeMo/Evaluator.git with NEL_NEXT_REF empty, so INSTALL_SPEC resolves to whatever the default branch HEAD happens to be.
Why it matters for this specific script:
INSTALL_SPECis now a constant string across upstream commits. The header's premise — "uv caches the resolved env and refreshes it when the spec changes" — no longer holds: the spec never changes while HEAD moves. uv caches the resolved git commit, so machine A (first run in June) and machine B (first run in August) silently run different toolchains under an identical spec, and neither run needs--refreshto notice.--versioncannot disambiguate. The PR body notes there is nov0.4.0tag, sonemo_evaluator.__version__prints0.4.0for every commit on the branch. There is no way for a user (or an agent following the skill) to answer "which build produced this score."- The validated toolchain is a specific SHA. The PR's own testing section cites
Evaluator.git@9dcca2aeas the build that matches golden, and the header tells the user to "setNEL_NEXT_REFto a commit SHA to pin" — but the shipped default does not do that, so the default path is the unreproducible one.
For a script whose entire purpose is producing benchmark numbers that are comparable across runs and across the BF16/NVFP4 sides of a comparison, the default should be the reproducible build.
Suggested fix — default NEL_NEXT_REF to the validated SHA, keeping both override paths intact:
NEL_NEXT_SPEC="${NEL_NEXT_SPEC:-}"
NEL_NEXT_ORIGIN="${NEL_NEXT_ORIGIN:-git+https://github.com/NVIDIA-NeMo/Evaluator.git}"
# Pinned to the build golden runs on (0.4.0, no tag exists). Set to a branch name
# (e.g. main) to track upstream, at the cost of reproducibility.
NEL_NEXT_REF="${NEL_NEXT_REF:-9dcca2ae}"and update the header + references/nel-next.md to state the default is pinned rather than instructing the user to pin it themselves. If tracking HEAD is genuinely intended, then --version / --which should surface the resolved commit so a run is at least attributable after the fact.
Secondary, smaller point in the same hunk: precedence between the two variables inverted (NEL_NEXT_ORIGIN used to win when both were set; now NEL_NEXT_SPEC does). That is intentional per the comment, but anyone with both already in their .env flips from a git build to a PyPI build with no signal — worth a line in references/nel-next.md.
| vLLM 400s on them if they aren't stripped. | ||
| - **`exclude_patterns`** = `["shard*", "model_traffic.jsonl"]` — captured request bodies | ||
| stay in the run dir, never MLflow. | ||
| - **`http_pairs_dump`** — `config: {dump_path: $${NEL_OUTPUT_DIR}/http_pairs_metrics.json, |
There was a problem hiding this comment.
[SUGGESTION] This snippet is not valid YAML if copied verbatim. In a flow mapping ({...}), {, }, [, ], and , are indicator characters and cannot appear in a plain (unquoted) scalar — so dump_path: $${NEL_OUTPUT_DIR}/http_pairs_metrics.json inside the braces is a parse error, not just a style nit.
recipes/examples/example_eval_next.yaml:54 and swebench_verified.md:87 both quote it correctly; only this reference line doesn't. Since these skill docs are copied into generated configs, the unquoted form would fail at config load rather than at --dry-run schema validation.
| - **`http_pairs_dump`** — `config: {dump_path: $${NEL_OUTPUT_DIR}/http_pairs_metrics.json, | |
| - **`http_pairs_dump`** — `config: {dump_path: "$${NEL_OUTPUT_DIR}/http_pairs_metrics.json", |
| # SWE-bench (OpenHands, multi-turn) adds turn_counter + consolidate_system + a system_message — see swebench_verified.md | ||
| - name: http_pairs_dump # FEP-1104/1120 diagnostics; canonical LAST in the chain | ||
| config: {dump_path: "$${NEL_OUTPUT_DIR}/http_pairs_metrics.json", first_n: 50} # $$ defers expansion to run time |
There was a problem hiding this comment.
[SUGGESTION] The comment says http_pairs_dump is "canonical LAST in the chain", which is right for TB2.1 (the benchmark this template's benchmarks: block is configured for), but swebench_verified.md:78 documents that SWE-bench needs it first and drop_params before consolidate_system. This template is the stated starting point for both recipes ("Start from recipes/examples/example_eval_next.yaml"), and the adjacent SWE-bench pointer on line 52 only mentions the added interceptors, not that the ordering changes.
Given the PR body calls "per-benchmark interceptor ordering" one of the things this change gets right, making the template say so avoids someone lifting the TB2.1 order into a SWE-bench config. Suggest extending the existing pointer comment:
# SWE-bench (OpenHands, multi-turn) adds turn_counter + consolidate_system + a
# system_message, AND reorders this chain (http_pairs_dump first, drop_params before
# consolidate_system) — see swebench_verified.md
| # Environments | ||
| .env | ||
| .env-* | ||
| .env* |
There was a problem hiding this comment.
[SUGGESTION] .env* is broader than the stated goal ("secret backups such as .env.bak-tb21 cannot be staged") and will also swallow any future .env.example / .env.template at the repo root — the exact files you want tracked. The skill already relies on a committed template (.agents/skills/evaluation/recipes/env.example, referenced from SKILL.md:84 and four task recipes); that path is safe today only because it lacks the leading dot.
Cheap insurance, since a silently-untracked template is a confusing failure mode:
.env*
!.env.example
|
|
||
| **Which file you mount changes the score.** Mount the canonical `swebench_instruction.md` | ||
| (underscore) at `/configs/prompts/swebench_instruction.md`, taken from the reference config or | ||
| run dir. The built-in in the `nemo_evaluator/templates/` venv directory is a **different | ||
| prompt** (`swebench-instruction.md`, hyphen) — it runs, but results are not comparable. Keep | ||
| whichever you use fixed across both sides of a comparison. | ||
|
|
||
| ```bash | ||
| VENV="${NEL_NEXT_VENV:-$HOME/.local/share/nel/venvs/nel-next}" # same default as nel-next.sh (NEL_NEXT_VENV may be unset) | ||
| cp "$VENV/lib/python3.12/site-packages/nemo_evaluator/templates/swebench-instruction.md" /tmp/ | ||
| ssh <login> 'mkdir -p <lustre>/<user>/prompts' && scp /tmp/swebench-instruction.md <login>:<lustre>/<user>/prompts/ | ||
| ssh <login> 'mkdir -p <lustre>/<user>/prompts' |
There was a problem hiding this comment.
[SUGGESTION] The rewritten gotcha loses the one thing that made the old version actionable: where to get the file.
The previous text gave a concrete cp from the venv ($VENV/lib/python3.12/site-packages/nemo_evaluator/templates/swebench-instruction.md). The new text correctly explains that the built-in is a different prompt and not comparable — good, that's a real scoring trap — but then the scp on line 64 sources a bare swebench_instruction.md from the caller's cwd, with the provenance only as "taken from the reference config or run dir." An agent following this recipe has no path to fetch, and the instruction "verify sha256 against the source" names no source to verify against.
The two other source-of-truth pointers in this file are precise (configs/benchmarks/swe-bench-verified/bench.yaml, configs/shared/nel_next_containers.yaml), so this one stands out. Suggest naming the repo-relative path the same way, e.g.:
# canonical prompt: configs/prompts/swebench_instruction.md in nvidia-eval-factory-benchmarking
# (dl/JoC/competitive_evaluation/…) — record its sha256 alongside the score
ssh <login> 'mkdir -p <lustre>/<user>/prompts'
scp swebench_instruction.md <login>:<lustre>/<user>/prompts/Also worth stating explicitly that both sides of a BF16-vs-quantized comparison must mount the same file — the current "Keep whichever you use fixed across both sides of a comparison" implies it, but this is the highest-leverage sentence in the section.
| | `max_concurrent` / `sandbox.concurrency` | `15` | | ||
| | `max_concurrent` / `sandbox.concurrency` | `15` in `bench.yaml`; per-model configs override it (MiniMax-M2.7 uses `20`) | | ||
| | `solver` | `timeout_strategy: max`, `run_timeout: 10800` (3h), `agent_kwargs.llm_kwargs.timeout: 3600` | | ||
| | `sandbox.region` | `us-east-2` | |
There was a problem hiding this comment.
[SUGGESTION] Two small consistency issues in this table row and the block below it.
-
The row now says
15inbench.yamlbut "per-model configs override it (MiniMax-M2.7 uses20)", while the YAML at line 33 still hardcodesmax_concurrent: 15with no note. Sincesandbox.concurrencymust trackmax_concurrent(the TB2.1 example flagskeep == sandbox.concurrency), a reader who bumps one per the table may not bump the other. A trailing comment on line 33 would close it. -
The new Sharding section recommends
shards: 10atconcurrency: 15→ 150 live Fargate sandboxes, whereas TB2.1's parallel section recommendsshards: 4atconcurrency: 50→ 200. Both then say "checkN × concurrencyagainst the Fargate quota" without giving the quota. Naming the actual limit once (inreferences/nel-next.md, since it's account-level rather than per-benchmark) would make both checks performable instead of advisory.
|
|
||
| Default install is public PyPI `nemo-evaluator[harbor]==0.3.*`; set | ||
| `NEL_NEXT_ORIGIN`/`NEL_NEXT_REF` for the internal git build (see script header). | ||
| Default install is a git build from `github.com/NVIDIA-NeMo/Evaluator` via `NEL_NEXT_ORIGIN` |
There was a problem hiding this comment.
[SUGGESTION] This paragraph now correctly says the default resolves to 0.4.0, but the version labels elsewhere still say 0.3.x and were not updated — so the doc contradicts itself a few lines apart:
references/nel-next.md:14— comparison table:package | `nemo-evaluator-launcher` 0.2.6 | `nemo-evaluator[harbor]` 0.3.xreferences/nel-next.md:23— "Installing 0.3.x into the 0.2.6 env clobbersnel"SKILL.md:39— "(nemo-evaluator[harbor]0.3.x)"scripts/nel-next.sh:17,20,23,55— header and_uvxcomment all say "nel 0.3.x" / "0.3.x environment"
The 0.2.6-vs-next separation argument holds regardless of which minor the "next" side is, so nothing is functionally wrong — but "0.3.x" is the value a reader will quote when sanity-checking --version output against the docs, and it now prints 0.4.0. Since the point of this PR is removing exactly this kind of drift, worth relabeling these to 0.3.x/0.4.x (or just "next") in the same pass.
![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: 2
🧹 Nitpick comments (2)
.agents/skills/evaluation/references/nel-next.md (1)
30-33: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAlign the
nel-nextversion contract. The installer uses the upstream default branch whenNEL_NEXT_REFis unset, while the reference and skill files still describe a 0.3.x environment. Pin the installer to the documented 0.3.x release, or update the CLI, schema, and templates to 0.4.0.🤖 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/references/nel-next.md around lines 30 - 33, Align the nel-next version contract across the installer and its documentation: either pin the installer’s default NEL_NEXT_REF to the documented 0.3.x release, or consistently update the CLI, schema, templates, and reference/skill files to 0.4.0. Ensure the default branch is not used implicitly when the documented version is expected..agents/scripts/nel-next.sh (1)
43-51: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPreserve the required extras in
NEL_NEXT_SPEC.If
NEL_NEXT_SPEComits[export],mlflow-pushlacks themlflowdependency. Document the requirednemo-evaluator[harbor,export]syntax and reject incomplete overrides, or install the exporter separately.🤖 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/scripts/nel-next.sh around lines 43 - 51, Update the NEL_NEXT_SPEC override handling in the installation setup to ensure it includes the required nemo-evaluator[harbor,export] extras, rejecting incomplete overrides with a clear message or installing the exporter dependencies separately. Document the required override syntax while preserving valid custom specifications.
🤖 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/tasks/aa_next/swebench_verified.md:
- Around line 92-93: Update the reasoning interceptor examples in the model
configuration section so they are model-specific: retain both reasoning and
reasoning_replay for reasoning models, use reasoning_replay.mode "think_tags"
for Qwen-style models, "native" for GLM models, and omit reasoning_replay for
MiniMax; ensure instruct-model examples include neither interceptor.
- Around line 66-70: Update the NEL schema reference in references/nel-next.md
to document cluster.container_mounts under the cluster configuration, noting
that it mounts files into the evaluator container for NEL SLURM jobs. Do not add
a service-level mount or alter the recipe configuration.
---
Nitpick comments:
In @.agents/scripts/nel-next.sh:
- Around line 43-51: Update the NEL_NEXT_SPEC override handling in the
installation setup to ensure it includes the required
nemo-evaluator[harbor,export] extras, rejecting incomplete overrides with a
clear message or installing the exporter dependencies separately. Document the
required override syntax while preserving valid custom specifications.
In @.agents/skills/evaluation/references/nel-next.md:
- Around line 30-33: Align the nel-next version contract across the installer
and its documentation: either pin the installer’s default NEL_NEXT_REF to the
documented 0.3.x release, or consistently update the CLI, schema, templates, and
reference/skill files to 0.4.0. Ensure the default branch is not used implicitly
when the documented version is expected.
🪄 Autofix
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: 84c9459f-a486-4872-8a1b-e40ebcd7a48d
📒 Files selected for processing (6)
.agents/scripts/nel-next.sh.agents/skills/evaluation/recipes/examples/example_eval_next.yaml.agents/skills/evaluation/recipes/tasks/aa_next/swebench_verified.md.agents/skills/evaluation/recipes/tasks/aa_next/terminal_bench_2_1.md.agents/skills/evaluation/references/nel-next.md.gitignore
| ```yaml | ||
| benchmarks: [{playbook: swebench_verified, instruction_template: /configs/swebench-instruction.md}] | ||
| benchmarks: [{playbook: swebench_verified, instruction_template: /configs/prompts/swebench_instruction.md}] | ||
| cluster: | ||
| container_mounts: ["<lustre>/<user>/prompts/swebench-instruction.md:/configs/swebench-instruction.md:ro"] | ||
| container_mounts: ["<lustre>/<user>/prompts/swebench_instruction.md:/configs/prompts/swebench_instruction.md:ro"] | ||
| ``` |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 \
'container_mounts|^services:|^cluster:' \
.agents/skills/evaluation/references/nel-next.md \
.agents/skills/evaluation/recipes/examples/example_eval_next.yaml \
.agents/skills/evaluation/recipes/tasks/aa_next/swebench_verified.mdRepository: NVIDIA/Model-Optimizer
Length of output: 13010
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- relevant files ---'
git ls-files '.agents/skills/evaluation' | sed -n '1,160p'
printf '%s\n' '--- mount and eval-image references ---'
rg -n -C 4 \
'container_mounts|eval_image|instruction_template|eval.image|mount' \
.agents/skills/evaluation \
--glob '*.md' --glob '*.yaml' --glob '*.yml' --glob '*.py' --glob '*.sh' \
| sed -n '1,260p'
printf '%s\n' '--- recipe context ---'
cat -n .agents/skills/evaluation/recipes/tasks/aa_next/swebench_verified.md | sed -n '45,90p'Repository: NVIDIA/Model-Optimizer
Length of output: 34115
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- NEL schema reference ---'
cat -n .agents/skills/evaluation/references/nel-next.md | sed -n '58,125p'
printf '%s\n' '--- all repository references to cluster.container_mounts and evaluation mounts ---'
rg -n -C 5 \
'cluster\.container_mounts|container_mounts|mounts:|eval_image|evaluation:|eval-image|eval image' \
. \
--glob '!*.lock' --glob '!*.jsonl' \
| sed -n '1,360p'
printf '%s\n' '--- possible schema/parser sources ---'
rg -n -C 3 \
'extra="forbid"|extra.*forbid|class .*Config|container_mounts|eval_image' \
. \
--glob '*.py' --glob '*.pyi' --glob '*.yaml' --glob '*.yml' --glob '*.toml' \
| sed -n '1,360p'Repository: NVIDIA/Model-Optimizer
Length of output: 50380
🌐 Web query:
NEL next configuration schema eval_image container_mounts evaluation container mounts nemo evaluator
💡 Result:
In the NVIDIA NeMo Evaluator (including configurations used with NeMo Evaluator Launcher/NEL), the eval_image and container_mounts parameters are primary configuration options used when deploying evaluation jobs to containerized environments, such as SLURM clusters or Docker-based executors [1][2]. 1. eval_image: This parameter specifies the container image (e.g., Apptainer, Enroot, or Docker) used to execute the evaluation harness [1][2]. It provides the isolated environment where the evaluation code runs [3]. In some advanced workflows, this can be dynamically set via environment variables (e.g., ${NEL_NEXT_EVAL_IMAGE}) to ensure the correct architecture-matched or version-pinned image is used [4]. 2. container_mounts: This parameter accepts a list of host paths to mount into the evaluation container [1][2]. It is essential for providing the containerized environment access to required data on the host machine, such as: - Datasets (if they are not automatically managed by the task's dataset directory mounting feature) [5][6] - Caching directories (e.g., for vLLM or HuggingFace) [4] - Configuration files [4] - Results output directories [7] Usage Contexts: - SLURM Deployment: Both parameters are typically defined under the cluster configuration block (e.g., cluster.eval_image and cluster.container_mounts) [1][2]. - NeMo Run: When using NeMo Run, these are set as properties on the executor object (e.g., executor.container_image and executor.container_mounts) [8]. - Dataset-Specific Mounting: Beyond general container mounts, tasks that require specific local datasets can also use the dataset_dir and optional dataset_mount_path fields within the evaluation task configuration [5][6]. The system automatically handles these mounts and sets the corresponding NEMO_EVALUATOR_DATASET_DIR environment variable [5][6]. For further details on specific schema structures, users should refer to the official documentation for the respective version of the NeMo Evaluator SDK, as configurations may vary between stable releases and development branches [5][9][10].
Citations:
- 1: https://docs.nvidia.com/nemo/evaluator/deployment/slurm
- 2: https://docs.nvidia.com/nemo/evaluator/nightly/deployment/slurm
- 3: https://docs.nvidia.com/nemo/evaluator/architecture
- 4: a05850b
- 5: https://docs.nvidia.com/nemo/evaluator/0.2.5/libraries/nemo-evaluator-launcher/configuration/evaluation/index.html
- 6: https://docs.nvidia.com/nemo/evaluator/0.2.5/deployment/launcher-orchestrated/local.html
- 7: https://docs.nvidia.com/nemo/evaluator/0.2.5/get-started/quickstart/container.html
- 8: https://docs.nvidia.com/nemo/evaluator/latest/tutorials/nemo-fw/nemo-run.html
- 9: https://docs.nvidia.com/nemo/microservices/latest/evaluate/evaluation-configs/schema.html
- 10: https://docs.nvidia.com/nemo/evaluator/latest/libraries/nemo-evaluator-launcher/configuration/executors/slurm.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- official SLURM documentation ---'
for url in \
'https://docs.nvidia.com/nemo/evaluator/deployment/slurm' \
'https://docs.nvidia.com/nemo/evaluator/nightly/deployment/slurm' \
'https://docs.nvidia.com/nemo/evaluator/latest/libraries/nemo-evaluator-launcher/configuration/executors/slurm.html'
do
printf '\nURL: %s\n' "$url"
curl -L --fail --silent --show-error "$url" |
tr '\n' ' ' |
sed 's/<[^>]*>/ /g' |
grep -oE '.{0,180}(container_mounts|eval_image|eval image|mounts).{0,260}' |
head -n 12 || true
done
printf '%s\n' '--- referenced repository commit ---'
if git cat-file -e a05850bffad6f254f265d7e30b46823a6064d8f7^{commit} 2>/dev/null; then
git show --stat --oneline a05850bffad6f254f265d7e30b46823a6064d8f7
git show a05850bffad6f254f265d7e30b46823a6064d8f7 -- \
'.agents/skills/evaluation/references/nel-next.md' \
'.agents/skills/evaluation/recipes/tasks/aa_next/swebench_verified.md' \
| sed -n '1,260p'
else
echo 'commit not present locally'
fiRepository: NVIDIA/Model-Optimizer
Length of output: 18435
Document cluster.container_mounts in the NEL schema reference.
cluster.container_mounts is supported for NEL SLURM jobs and mounts files into the evaluator container. The recipe does not need a service-level mount. Update references/nel-next.md to list this field under cluster and clarify its target.
🤖 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/recipes/tasks/aa_next/swebench_verified.md around
lines 66 - 70, Update the NEL schema reference in references/nel-next.md to
document cluster.container_mounts under the cluster configuration, noting that
it mounts files into the evaluator container for NEL SLURM jobs. Do not add a
service-level mount or alter the recipe configuration.
| - {name: reasoning} # reasoning models: normalize reasoning field … | ||
| - {name: reasoning_replay} # … and replay it across turns (drop both for instruct) | ||
| - {name: reasoning_replay} # … and replay across turns. Drop both for instruct models. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
file='.agents/skills/evaluation/recipes/tasks/aa_next/swebench_verified.md'
printf '%s\n' '--- target excerpt ---'
sed -n '70,115p' "$file"
printf '%s\n' '--- related reasoning terms ---'
rg -n -C 3 'reasoning_replay|think_tags|MiniMax|Qwen|GLM|instruct|native' .agents/skills/evaluation/recipes/tasks/aa_next .agents/skills/evaluation 2>/dev/null | head -n 240Repository: NVIDIA/Model-Optimizer
Length of output: 33100
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- interceptor definitions and usage ---'
rg -n -C 4 'reasoning_replay|name:\s*reasoning|think_tags|reasoning.*mode|native.*GLM' . --glob '!*.md' --glob '!*.lock' --glob '!package-lock.json' 2>/dev/null | head -n 320
printf '%s\n' '--- all likely proxy/config files ---'
git ls-files | rg '(^|/)(bench\.yaml|.*(proxy|interceptor|nel.*config|config).*\.(ya?ml|json|py|ts|js|md))$' | head -n 240Repository: NVIDIA/Model-Optimizer
Length of output: 18764
🌐 Web query:
NeMo Evaluator Launcher reasoning_replay interceptor mode think_tags native MiniMax
💡 Result:
In the NVIDIA NeMo Evaluator, reasoning_replay is a specialized interceptor designed to manage chain-of-thought (CoT) reasoning across multi-turn agentic interactions [1][2]. It addresses the issue where certain agent SDKs drop reasoning_content during replay, which can lead to a loss of CoT context and increased turn counts in agentic rollouts [1][3]. The reasoning_replay interceptor caches reasoning_content from upstream model responses (keyed by tool call ID or content hash) and re-injects it into subsequent outbound requests [1]. Regarding the requested modes and features: 1. Interceptor Mode (Native): The reasoning_replay interceptor supports a native mode [3]. In this mode, it ensures compatibility with backends (such as vLLM versions >=0.19) that may require specific reasoning field representations [3]. Recent updates ensure that in native (and both) modes, the interceptor preserves and writes both reasoning and reasoning_content fields to accommodate differences in how various backends parse chat input [3]. 2. Think Tags: The interceptor integrates with the configured reasoning pipeline to wrap cached hidden chains of thought with configured think tokens (e.g., ...) [3][4]. While NeMo Evaluator generally allows customization of these tokens (e.g., in reasoning interceptor configuration) [4], the reasoning_replay interceptor uses these settings to maintain consistent reasoning formatting during re-injection [3]. 3. MiniMax: There is no direct mention of native MiniMax integration specifically within the NeMo Evaluator reasoning_replay interceptor documentation or commit history [1][3]. NeMo Evaluator interceptors are modular and typically built to be backend-agnostic by using standard OpenAI-compatible interfaces [4][5], meaning they would process reasoning content based on standard fields (like reasoning_content) regardless of the specific underlying model provider, provided the provider follows that protocol. In summary, the reasoning_replay interceptor is a tool to ensure continuity of reasoning in long-running agentic evaluations, with native modes specifically tuned to handle backend-specific field requirements (like vLLM) and standard think-tag wrapping [1][3].
Citations:
- 1: NVIDIA-NeMo/Evaluator@751d1f7
- 2: NVIDIA-NeMo/Evaluator@fce9256
- 3: NVIDIA-NeMo/Evaluator@e4cc20c
- 4: https://docs.nvidia.com/nemo/evaluator/latest/about/concepts/interceptors.html
- 5: https://docs.nvidia.com/nemo/evaluator/latest/libraries/nemo-evaluator/interceptors/index.html
🌐 Web query:
site:github.com/NVIDIA/NeMo-Evaluator-Launcher reasoning_replay think_tags
💡 Result:
I couldn’t find an indexed match for reasoning_replay or think_tags in NVIDIA/NeMo-Evaluator-Launcher.
The relevant repository appears to be NVIDIA-NeMo/Evaluator, whose launcher package is located under packages/nemo-evaluator-launcher/. (github.com)
Note: GitHub’s current repository path is likely:
NVIDIA-NeMo/Evaluator
Citations:
🏁 Script executed:
#!/bin/bash
set -eu
for sha in 751d1f7cc2b078ac90a161e49db361da487d741a fce9256690e99c5905e74ba03fea482f2e9f9a75 e4cc20c94e4dd9dbdcd3c5a2d05b878bbfc22907; do
printf '\n--- commit %s ---\n' "$sha"
curl -fsSL "https://api.github.com/repos/NVIDIA-NeMo/Evaluator/commits/$sha" |
jq -r '.commit.message, (.files[]? | "FILE: \(.filename)\n\(.patch // "")")' |
rg -n -C 5 'reasoning_replay|think_tags|native|MiniMax|mode|reasoning_content' | head -n 240 || true
done
printf '\n--- current upstream interceptor docs ---\n'
curl -fsSL 'https://raw.githubusercontent.com/NVIDIA-NeMo/Evaluator/main/docs/about/concepts/interceptors.md' |
rg -n -C 5 'reasoning_replay|think_tags|native|MiniMax|reasoning_content' | head -n 240 || trueRepository: NVIDIA/Model-Optimizer
Length of output: 570
🌐 Web query:
site:docs.nvidia.com/nemo/evaluator reasoning_replay mode think_tags
💡 Result:
In the context of LLM evaluation frameworks, specifically the NVIDIA NeMo Evaluator SDK, the concepts of reasoning traces and think tags are utilized to process and analyze the step-by-step thinking processes of reasoning-capable models [1][2]. Think tags (e.g., and ) are special tokens used by many modern LLMs to encapsulate their chain-of-thought reasoning process—the intermediate logical steps, calculations, and deliberations performed before arriving at a final answer [1][2]. Reasoning replay or reasoning interception is a functional component within the NeMo Evaluator that manages these traces [2][3]. Because many benchmarks and evaluation pipelines expect only the final answer from a model, the Reasoning Interceptor serves several key roles: - Extraction and Removal: It identifies the reasoning trace within the model's output (using the specified start and end tokens), removes it from the content to ensure the evaluation receives only the final answer, and optionally isolates it for further analysis [1][2][4]. - Statistics and Metrics Tracking: The interceptor tracks metrics related to the reasoning process, such as the number of responses that included reasoning, whether the reasoning completed (i.e., reached the end token), and the average length of the reasoning trace (in tokens or words) [2][4][5]. This data is typically saved to an evaluation metrics file (e.g., eval_factory_metrics.json), allowing developers to analyze model behavior, compute costs, or evaluate the quality of the chain-of-thought [1][4][5]. While "reasoning_replay" is not a singular standard term, it appears in this technical ecosystem as part of the operational workflow for handling, logging, and re-evaluating the reasoning traces generated during model inference [2][6]. Configuration of these interceptors allows users to specify custom tokens if their model uses different markers than the standard tags [2][7].
Citations:
- 1: https://docs.nvidia.com/nemo/evaluator/nightly/evaluation/run-evals/reasoning.html
- 2: https://docs.nvidia.com/nemo/evaluator/nightly/libraries/nemo-evaluator/interceptors/reasoning.html
- 3: https://docs.nvidia.com/nemo/evaluator/0.2.5/deployment/adapters/configuration.html
- 4: https://docs.nvidia.com/nemo/evaluator/0.2.5/tutorials/how-to/reasoning.html
- 5: https://docs.nvidia.com/nemo/evaluator/latest/tutorials/how-to/reasoning.html
- 6: https://docs.nvidia.com/nemo/evaluator/0.2.5/about/concepts/interceptors.html
- 7: https://docs.nvidia.com/nemo/evaluator/0.2.5/_modules/nemo_evaluator/adapters/interceptors/reasoning_interceptor.html
🌐 Web query:
site:github.com/NVIDIA-NeMo/Evaluator "reasoning_replay" "think_tags"
💡 Result:
In the context of the NVIDIA NeMo Evaluator framework, reasoning_replay and think_tags are components designed to manage and preserve Chain-of-Thought (CoT) reasoning processes across multi-turn agent interactions [1][2]. reasoning_replay is an interceptor designed to solve the problem where certain agent SDKs (such as OpenHands or pi-ai) drop reasoning_content during conversation replays, causing the model to lose its reasoning context in subsequent turns [2]. The interceptor functions by caching the reasoning_content from upstream model responses and automatically re-injecting it into the next outbound request [3][2]. This ensures that the model's intermediate "thinking" remains available throughout the session [2]. think_tags refer to the configuration used to wrap this cached reasoning content [3]. Because different model backends and APIs may use different fields or formats for reasoning (e.g., specific tags or normalized fields like reasoning_content), the system uses think_tags to standardize how the hidden chain of thought is presented when re-injected [3]. The interceptor can be configured to operate in different modes (such as native or both), ensuring compatibility across various backends like vLLM and SGLang by reading and writing the appropriate reasoning fields [3].
Citations:
- 1: https://github.com/NVIDIA-NeMo/Evaluator/blob/main/README.md
- 2: NVIDIA-NeMo/Evaluator@751d1f7
- 3: NVIDIA-NeMo/Evaluator@e4cc20c
🌐 Web query:
site:github.com/NVIDIA-NeMo/Evaluator "reasoning_replay" "MiniMax"
💡 Result:
The term reasoning_replay refers to a specific interceptor component within the NVIDIA NeMo Evaluator framework, not a standalone model or feature from MiniMax [1][2]. It was developed to solve the issue where certain agent SDKs (such as OpenHands or older implementations) would drop an LLM's reasoning content—its Chain-of-Thought (CoT)—during multi-turn interactions [1][2]. When agentic workflows replay previous turns, the loss of this reasoning can cause models to lose context, leading to redundant or inefficient rollouts [2]. Key technical aspects of the reasoning_replay interceptor include: Caching and Re-injection: It intercepts model responses to cache reasoning content (typically identified by fields like reasoning_content or reasoning) and re-injects that content into subsequent outbound requests [1][2]. Cross-Backend Compatibility: The interceptor manages differences in how various inference backends (such as vLLM versions or SGLang) represent reasoning fields [2]. For example, it ensures compatibility with vLLM ≥0.19 by writing both reasoning and reasoning_content fields, as different backends rely on different keys [2]. Deterministic Operation: It is designed to compose deterministically with other framework components, such as reasoning normalizers and caching interceptors, ensuring the CoT is preserved consistently across long agentic benchmark runs [1]. While NeMo Evaluator is used to evaluate various models, including those with reasoning capabilities, reasoning_replay is a utility within the evaluation infrastructure designed to maintain data integrity during testing, rather than a feature of any specific model provider like MiniMax [1][3][4].
Citations:
- 1: NVIDIA-NeMo/Evaluator@751d1f7
- 2: NVIDIA-NeMo/Evaluator@e4cc20c
- 3: https://github.com/NVIDIA-NeMo/Evaluator/blob/main/README.md
- 4: https://github.com/NVIDIA-NeMo/Evaluator
Make the reasoning interceptor configuration model-specific.
Reasoning models require reasoning and reasoning_replay; instruct models require neither. Set reasoning_replay.mode per model: think_tags for Qwen-style models, native for GLM models, and omit it for MiniMax. Update the examples at lines 92–93 and 96–100.
🤖 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/recipes/tasks/aa_next/swebench_verified.md around
lines 92 - 93, Update the reasoning interceptor examples in the model
configuration section so they are model-specific: retain both reasoning and
reasoning_replay for reasoning models, use reasoning_replay.mode "think_tags"
for Qwen-style models, "native" for GLM models, and omit reasoning_replay for
MiniMax; ensure instruct-model examples include neither interceptor.
There was a problem hiding this comment.
Claude review — [skill] evaluation: align nel-next TB2.1/SWE-bench with golden toolchain
Scope applied: full review per the standard procedure. The triggering comment supplied end-to-end validation evidence but no scoping instruction, so nothing was narrowed. All 6 changed files reviewed (99 additions / 43 deletions) — no modelopt/ source is touched, so the mode-registration, config-schema, and export-compatibility categories don't apply here; the review focused on the agent-skill contract: are the documented values self-consistent, reproducible, and copy-pasteable.
Note on the diff: a .agents/skills/deployment/references/trtllm.md hunk shows up in the two-dot diff against origin/main. That is main-side drift (the checkout is shallow, so no merge base is available), not part of this PR's file list — I excluded it.
Findings: CRITICAL: 0 · IMPORTANT: 1 · SUGGESTION: 5
The one blocking item
nel-next.sh:44 — the new default install is an unpinned git branch. This is the only finding I'd hold the PR for, because it works against the PR's own thesis.
The change is correct in its diagnosis: PyPI tops out at 0.3.0 and cannot reach the 0.4.x toolchain, so a git install is the right move. But NEL_NEXT_REF defaults to empty, so INSTALL_SPEC resolves to whatever the default branch HEAD happens to be. Three consequences specific to this script:
INSTALL_SPECis now a constant string while HEAD moves, which breaks the header's own stated invariant that "uv caches the resolved env and refreshes it when the spec changes." Two machines first running this in different months get different toolchains under an identical spec, and neither needs--refreshto notice.--versioncan't disambiguate them — the PR body confirms nov0.4.0tag exists, so every commit on the branch prints0.4.0. A score becomes unattributable to a build.- The PR's own testing section names
Evaluator.git@9dcca2aeas the build matching golden, and the new header tells the user to "setNEL_NEXT_REFto a commit SHA to pin" — but the shipped default doesn't, so the default path is the unreproducible one.
Fix is one line (NEL_NEXT_REF="${NEL_NEXT_REF:-9dcca2ae}"), details inline. If tracking HEAD is deliberate, then --version/--which should surface the resolved commit so runs stay attributable after the fact.
Also flagged there: the precedence between NEL_NEXT_SPEC and NEL_NEXT_ORIGIN inverted. That's intentional per the new comment, but anyone with both already in .env silently flips from a git build to a PyPI build — worth a line in the reference doc.
Suggestions (non-blocking)
references/nel-next.md:147— thehttp_pairs_dumpsnippet isn't valid YAML as written: unquoted$${NEL_OUTPUT_DIR}/…inside a flow mapping terminates the plain scalar at{. The two other copies of this snippet (example_eval_next.yaml:54,swebench_verified.md:87) quote it correctly; only the reference doesn't. Since these get copied into generated configs, it'd fail at config load rather than at--dry-run.references/nel-next.md:30— the doc now says the default resolves to 0.4.0, but the version labels four lines up (:14,:23), inSKILL.md:39, and in the script header (:17,20,23,55) still say "0.3.x". Nothing is functionally wrong, but that's the value a reader quotes when checking--versionoutput — and removing exactly this kind of drift is the point of the PR.swebench_verified.md:54-62— the rewritten instruction-template gotcha adds the genuinely valuable warning (the venv built-in is a different prompt and not comparable) but drops the concrete path to fetch the canonical file, so thescpnow sources a bare filename from cwd and "verify sha256 against the source" names no source. The other two source-of-truth pointers in the same file are precise; this one should be too.example_eval_next.yaml:52-54— the template is the documented starting point for both recipes and sayshttp_pairs_dumpis "canonical LAST", which is TB2.1-only; the adjacent SWE-bench pointer mentions the added interceptors but not that the ordering itself changes..gitignore:32—.env*is broader than the stated goal and would also swallow a future root.env.example; a!.env.examplenegation is cheap insurance. (Today's committed template,.agents/skills/evaluation/recipes/env.example, is safe only because it lacks the leading dot.)swebench_verified.md:21— themax_concurrentrow now documents a per-model override (MiniMax uses 20) but the YAML below still hardcodes 15 with no note, andsandbox.concurrencyhas to track it. Separately, the two new sharding sections recommend fan-outs that differ by 25% in live sandboxes (150 vs 200) while both say "check against the Fargate quota" without naming it.
Risk assessment
Low. Docs and one shell script; no library code, no public API, no modelopt_state schema, no export path — the checkpoint/config backward-compatibility concerns in CONTRIBUTING.md don't come into play, and the PR's own N/A answers on tests and CHANGELOG are appropriate.
The substantive value-check items are well supported. I confirmed the internal consistency of the aligned values that carry scoring weight: proxy.request_timeout: 3600 >= llm_kwargs.timeout: 3600 holds in the example, both task recipes, and the reference; the four-param drop_params list is identical across all four sites; exclude_patterns is consistent across all five; and the model_traffic: {capture_request_body: true} addition is correctly paired with its exclude_patterns entry, which is the pairing that keeps captured request bodies out of MLflow. The end-to-end evidence in the triggering comment (TB2.1 delta 0.0000, SWE-bench +0.40%, overlapping CIs on full task sets) is the right validation for a change of this kind and covers the scoring-relevant surface — the interceptor ordering, sampling preset, and instruction template — better than any static review could.
The remaining reproducibility gap is the unpinned ref: that eval run pinned a specific build implicitly, but the config as shipped doesn't let the next person reproduce it. Worth closing before merge, since the whole purpose here is scores that stay comparable across runs and across both sides of a quantization comparison.
meenchen
left a comment
meenchen
left a comment
There was a problem hiding this comment.
Bot review (bedrock-claude-opus-5) — DM the bot to share feedback.
Docs/tooling-only change to the evaluation agent skill (5 md/yaml files + .gitignore + one shell script, +99/-43). No prompt-injection or licensing concerns: no LICENSE/SPDX changes, no vendored third-party code, and nel-next.sh keeps its existing Apache header.
Design review (gate fired on directory span): the PR doesn't introduce a new subsystem — it re-points defaults in the existing NEL_NEXT_SPEC/NEL_NEXT_ORIGIN mechanism and updates skill docs, so there is no second in-repo system being created. The one real design decision (default install source PyPI → upstream git) is justified in the PR body (PyPI nemo-evaluator tops out at 0.3.0, so the 0.4.x toolchain golden runs on is unreachable from PyPI), and the PyPI path is preserved as an escape hatch. That's a reasonable rationale; I'm not blocking on the design question. Correctness/consistency issues below.
- Default git install is an unpinned mutable ref (
git+https://github.com/NVIDIA-NeMo/Evaluator.gitwithNEL_NEXT_REFempty). The docs you added say "pinNEL_NEXT_REFto a commit SHA for reproducibility", but the shipped default doesn't — and sinceINSTALL_SPECis what uv keys its cache on, the string never changes as upstreammainmoves, so two people (or the same person after a cache refresh) can silently get different toolchains for a scored benchmark. You already validatedEvaluator.git@9dcca2ae; defaultingNEL_NEXT_REFto that SHA would make the tested toolchain the default and still allow overrides. It's also a supply-chain step down: default--setup-onlynow builds and executes whatever is on a public default branch. - Duplicate, conflicting
cluster.eval_imagerow interminal_bench_2_1.md— the old row (line 23, "set bymodelopttools:eval-config") and the new row (line 27, "0.5.0.1-harbor") now both exist in the same table. Merge them. http_pairs_dumpoutput isn't excluded from MLflow. You addmodel_traffic.jsonltoexclude_patternsprecisely because captured request bodies shouldn't reach MLflow, but the new interceptor writes$${NEL_OUTPUT_DIR}/http_pairs_metrics.jsoninto the same run dir withcopy_logs: trueand no exclusion. If those paired dumps contain request/response bodies, this reintroduces the leak the exclusion is meant to prevent (or, if it's metrics-only, a one-line note saying so would settle it).- Minor / no action strictly required:
- Several places still label the toolchain "0.3.x" while the default is now 0.4.0:
nel-next.shheader line 17, thenel-next.mdtitle + comparison table ("nemo-evaluator[harbor]0.3.x"), andexample_eval_next.yaml's header ("validate against 0.3.0"). Since the PR's stated goal is aligning docs with the golden toolchain, these read stale. swebench_verified.mdnow says toscp swebench_instruction.md"taken from the reference config or run dir" but drops the only concrete acquisition command (the old venv copy) — a reader without access to the internal repo has no way to obtain the canonical file..gitignorebroadening to.env*is fine today (recipes/env.examplehas no leading dot, and nothing tracked matches), but it also pre-emptively ignores.env.example/.envrc-style templates should the repo ever want one; a!.env.examplenegation would keep that door open.
- Several places still label the toolchain "0.3.x" while the default is now 0.4.0:
| NEL_NEXT_SPEC="${NEL_NEXT_SPEC:-nemo-evaluator[harbor,export]==0.3.*}" | ||
| NEL_NEXT_ORIGIN="${NEL_NEXT_ORIGIN:-}" | ||
| NEL_NEXT_SPEC="${NEL_NEXT_SPEC:-}" | ||
| NEL_NEXT_ORIGIN="${NEL_NEXT_ORIGIN:-git+https://github.com/NVIDIA-NeMo/Evaluator.git}" |
There was a problem hiding this comment.
Bot comment.
The default git origin has no ref, so the default install tracks upstream main. Two problems for a scoring toolchain:
- Reproducibility — the docs you added in this PR say "No
v0.4.0tag exists — pinNEL_NEXT_REFto a commit SHA for reproducibility", but the shipped default doesn't pin.INSTALL_SPECis also the uv cache key, and it never changes as upstream moves, so the resolved environment depends on when the cache was populated — different people can score the same benchmark on different toolchains without noticing. - Supply chain —
--setup-onlynow builds and runs whatever is currently on a public default branch by default.
You already validated Evaluator.git@9dcca2ae; suggest making that the default (NEL_NEXT_REF="${NEL_NEXT_REF:-9dcca2ae...}") so the tested toolchain is what users get, with the env var still available to move forward.
| | `max_concurrent` / `sandbox.concurrency` | `50` (canonical bench.yaml) | | ||
| | timeout_strategy | `max` (canonical bench.yaml) + `agent_kwargs.llm_kwargs.timeout: 3600`; use `task` for leaderboard-comparable | | ||
| | `cluster.eval_image` requirement | **≥ `0.3.1.1-harbor`** — TB 2.1's task set is pinned via a vendored registry override in that image (`${NEL_NEXT_EVAL_IMAGE}`, multi-arch) | | ||
| | `cluster.eval_image` | **`0.5.0.1-harbor`** (`${NEL_NEXT_EVAL_IMAGE}`, multi-arch) | |
There was a problem hiding this comment.
Bot comment.
This table now has two cluster.eval_image rows: line 23 (${NEL_NEXT_EVAL_IMAGE} — set by modelopttools:eval-config) and this one (0.5.0.1-harbor). The diff replaced the old "cluster.eval_image requirement" row but left the earlier plain one, so the table has a duplicated key with two different values. Please merge into a single row.
| config: {params: [max_tokens, max_completion_tokens, max_input_tokens_per_task, no_rebuild]} | ||
| # SWE-bench (OpenHands, multi-turn) adds turn_counter + consolidate_system + a system_message — see swebench_verified.md | ||
| - name: http_pairs_dump # FEP-1104/1120 diagnostics; canonical LAST in the chain | ||
| config: {dump_path: "$${NEL_OUTPUT_DIR}/http_pairs_metrics.json", first_n: 50} # $$ defers expansion to run time |
There was a problem hiding this comment.
Bot comment.
This dump lands in ${NEL_OUTPUT_DIR} while output.export_config.mlflow has copy_logs: true, and http_pairs_metrics.json isn't in exclude_patterns. That's inconsistent with the model_traffic.jsonl exclusion added below, whose stated reason is that captured request bodies must never reach MLflow. If http_pairs_dump records request/response payloads (the SWE-bench recipe calls it "paired HTTP" diagnostics), it should be excluded too; if it's genuinely metrics-only, a short note here saying "no payloads, safe to export" would make the asymmetry intentional rather than an oversight.
…o canaries Two review findings on the nel-next toolchain change: 1. NEL_NEXT_REF defaulted to empty, so the install resolved to whatever NVIDIA-NeMo/Evaluator main pointed at. Baseline and candidate runs submitted days apart (or from a cold uv cache) would install different harness commits, folding a harness change into the pass@1 delta that drives the ACCEPT/REGRESSION call, with no recorded SHA to detect it. Default NEL_NEXT_REF to the current main SHA (0.4.0, verified to install); NEL_NEXT_REF=main still tracks HEAD for dev/canary work. --version now also prints the resolved spec so it can be recorded with scored results. 2. http_pairs_dump was enabled in the shared template and both recipes. first_n caps only successful pairs -- the keep rule is (total_seen <= first_n) or (status != 200) -- so every error pair is held in memory with full request and response bodies for the life of the run, and the whole list is re-serialized on each write. On the very failures it diagnoses (vLLM 400s, sustained 429/5xx) a long agentic run grows the proxy without bound. Comment it out of the template and the SWE-bench chain, and document it as canary/diagnostic-only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Zhiyu Cheng <zhiyuc@nvidia.com>
Edwardf0t1
left a comment
Edwardf0t1
left a comment
There was a problem hiding this comment.
Two smaller doc-accuracy points from the same review pass (the two correctness findings — the unpinned NEL_NEXT_REF and http_pairs_dump's unbounded error-pair retention — are fixed in the commit I just pushed).
| proxy: | ||
| request_timeout: 3600 | ||
| extra_body: {skip_special_tokens: false} # add model-card sampling extras if the card sets them | ||
| model_traffic: {capture_request_body: true} # FEA-224; pair with the exclude_patterns entry |
There was a problem hiding this comment.
The "pair with the exclude_patterns entry" framing reads as if the exclude is only needed because of this SWE-bench opt-in, but the capture is on by default.
In 0.4.0 ModelTrafficCaptureConfig (config/services.py:64-78) defaults:
capture_tool_calls: bool = True
capture_reasoning: bool = True
capture_messages: bool = True
capture_request_body: bool = False # <- the only opt-in
max_content_chars: int = 0 # 0 = no truncationSo model_traffic.jsonl — assistant messages, reasoning content and full tool-call payloads — is written on every nel-next run; capture_request_body: true only adds the upstream request body on top. A TB2.1 user who isn't setting capture_request_body can read this note and conclude the exclude_patterns entry is unnecessary for them, which pushes captured model traffic to MLflow.
Suggest making the exclude unconditional in references/nel-next.md ("model traffic is captured by default, so the exclude is always required") and rewording this to "adds the upstream request body to the traffic capture that is already on by default." max_content_chars is also worth a mention as the truncation knob, since there is currently no documented way to turn the capture down.
| - {name: reasoning_replay} # … and replay across turns. Drop both for instruct models. | ||
| ``` | ||
|
|
||
| **`reasoning_replay.mode` is per model, not per benchmark.** `think_tags` (Qwen-style), |
There was a problem hiding this comment.
omitted isn't a third behavior — it selects think_tags.
reasoning_replay.Interceptor.__init__ (adapters/interceptors/reasoning_replay.py:120) is mode: str = "think_tags", so omitting the key gives exactly the Qwen setting. Since this same sentence warns that the wrong mode is "a silent output-parsing bug", listing MiniMax as omitted invites the reading that replay is neutral or off for it, when it actually gets <think>-tag re-injection.
If think_tags genuinely is right for MiniMax, saying so explicitly ("MiniMax: leave mode unset — the default is think_tags") removes the ambiguity. The valid set is think_tags / native / both.
Builds on a6ddca2 (NEL_NEXT_REF pinning + scoping http_pairs_dump to canaries), which already covered the install-pinning and invalid-YAML findings. These are the review items it did not touch: - reasoning_replay.mode: "omitted" is not a third behaviour. The default is think_tags, so leaving mode unset selects it; the valid set is think_tags/native/both. Listing MiniMax as "omitted" invited the reading that replay is off or neutral for it when it actually gets <think>-tag re-injection. - model_traffic: correct the "pair with the exclude_patterns entry" framing. capture_tool_calls/capture_reasoning/capture_messages all default true; only capture_request_body defaults false, so model_traffic.jsonl is written on every run and the exclude is unconditional rather than a consequence of opting in. - terminal_bench_2_1.md: drop the duplicated cluster.eval_image table row, which listed a second value for the same key. - swebench_verified.md: give the instruction template a concrete provenance -- "verify sha256 against the source" named no source -- and keep max_concurrent visibly coupled to sandbox.concurrency. - version labels: 0.3.x -> 0.4.x across the script header, reference, template and TB2.1 recipe, which contradicted the pinned 0.4.x install. Verified: bash -n clean, template YAML parses, pre-commit clean; a6ddca2's NEL_NEXT_REF pin and commented-out http_pairs_dump are preserved. Signed-off-by: Chenjie Luo <chenjiel@nvidia.com>
…mplate The template is the stated starting point for both recipes, and the pointer comment only mentioned the added interceptors. SWE-bench also reorders the chain (drop_params before consolidate_system), so lifting this order into a SWE-bench config silently diverges from the reference. Signed-off-by: Chenjie Luo <chenjiel@nvidia.com>
Review feedback addressedThanks @meenchen and @Edwardf0t1 — two of these were correctness bugs in the docs, not style. Already covered by a6ddca2 (@Edwardf0t1's push): the unpinned
Verified: all doc flow-mapping snippets parse via Not changed — |
meenchen
left a comment
meenchen
left a comment
There was a problem hiding this comment.
Bot review (bedrock-claude-opus-5) — DM the bot to share feedback.
Re-review of PR #2063 (docs/tooling only for the evaluation agent skill, 6 files +139/-59). Design gate: still not a new subsystem — it re-points defaults inside the existing NEL_NEXT_SPEC/NEL_NEXT_ORIGIN mechanism, and the one real design choice (PyPI → upstream git, because PyPI nemo-evaluator tops out at 0.3.0) is justified in the PR body with the PyPI path kept as an escape hatch. No licensing changes (existing Apache/SPDX header on nel-next.sh untouched), no prompt-injection attempts in the PR content.
Previously flagged, now resolved (dropping these): the unpinned NEL_NEXT_REF (now a hard SHA default plus a rationale comment), the invalid-YAML http_pairs_dump flow mapping, the unbounded error-pair retention / http_pairs_metrics.json MLflow leak (interceptor is now commented out and documented as canary-only in all three sites), the duplicated cluster.eval_image row in terminal_bench_2_1.md, reasoning_replay.mode "omitted" wording, and the "capture is opt-in" framing for model_traffic.
Worth a human look before merge:
- The shipped pin is
4d081325170aababd0c8f27c58bed31a81ce82ac("main @ 2026-08-04"), but the PR body's testing section states the validated build wasEvaluator.git@9dcca2ae. So the default toolchain users get is not the one the end-to-end TB2.1/SWE-bench parity runs exercised (or the body is stale). Since the whole point of the pin is that the scored toolchain is the validated one, please confirm which SHA was validated and update the other. - 💬 Author replied that the 0.3.x → 0.4.x labels were updated "across script header, reference, template and the TB2.1 recipe" — still warrants a look because two entry points remain stale:
recipes/examples/example_eval_next.yaml:12("all blocks validate against 0.3.0", contradicting the 0.4.x header three lines above) and.agents/skills/evaluation/SKILL.md(nel-next branch: "nemo-evaluator[harbor]0.3.x", "Isolated 0.3.x venv", "require nemo-evaluator-next (0.3.x)"). SKILL.md is the file an agent reads first, and removing exactly this drift is the PR's stated goal. references/nel-next.mdno longer mentionsNEL_NEXT_SPECat all, while the script inverted precedence soNEL_NEXT_SPECnow wins. Anyone with the old PyPI spec still in.envsilently gets a 0.3.x install that the same doc says "can't reach 0.4.x" (and lacks the vendored TB2.1 registry override) — one sentence in the venv section would close this.- 💬 Author declined
!.env.examplein.gitignore(nothing shadowed today because the committed template isrecipes/env.example, no leading dot) and offered to add the negation — reasonable, but it's a one-line guard against a confusing future failure mode; owner's call. - 💬 The SWE-bench instruction-template gotcha now names
configs/benchmarks/swe-bench-verified/bench.yamland adds asha256sumcheck — still no concrete acquisition command:scp swebench_instruction.md <login>:…sources a bare filename from cwd, so a reader without the internal repo has no way to obtain the file, and "must match the source" names no fetchable path. - CodeRabbit's request to document
cluster.container_mountsin thereferences/nel-next.mdschema block appears unaddressed (the schema section still omits it while the SWE-bench recipe relies on it). Minor.
No tests expected for an agent-skill docs change; the e2e parity table is the appropriate validation.
…ck YAML
- references/nel-next.md: add `cluster.container_mounts` to the schema block. It was
undocumented despite SWE-bench's instruction_template mount depending on it, and
the target (the EVAL container, not the service) was not stated anywhere.
- Same file: quote `${...}` inside the two flow mappings in that schema block
(`sandbox:` and `container_env:`). This is the defect already fixed for the
http_pairs_dump snippet -- `{`/`}` are YAML indicators, so an unquoted variable
reference in a flow mapping is a parse error. Pre-existing, and the whole block
now round-trips through yaml.safe_load.
Not applied: the review suggestion to document reasoning_replay as "omit it for
MiniMax". Omitting is not a third behaviour -- reasoning_replay's mode parameter
defaults to "think_tags", so omitting selects it. The current wording ("MiniMax:
leave unset (i.e. think_tags)") is deliberate and states the resulting behaviour.
Signed-off-by: Chenjie Luo <chenjiel@nvidia.com>
CodeRabbit findings
While there I fixed a pre-existing instance of the same invalid-YAML defect the review
# nemo_evaluator/adapters/interceptors/reasoning_replay.py
_VALID_MODES = ("think_tags", "native", "both")
def __init__(self, ..., mode: str = "think_tags", ...)The default is The rest of the suggestion (reasoning models need both interceptors, instruct models need |
meenchen
left a comment
meenchen
left a comment
There was a problem hiding this comment.
Bot review (bedrock-claude-opus-5) — DM the bot to share feedback.
Re-review of PR #2063 (evaluation agent skill, docs/tooling only — 6 files +143/-61: 4 md/yaml, .gitignore, one shell script). No library code, no tests expected; the e2e TB2.1/SWE-bench parity table is the right validation for this kind of change.
Design gate (fired on directory span): still not a new subsystem. The PR re-points defaults inside the existing NEL_NEXT_SPEC/NEL_NEXT_ORIGIN/NEL_NEXT_REF mechanism in .agents/scripts/nel-next.sh and updates the skill docs — no second in-repo system is introduced, and the one real design choice (default install source PyPI → upstream git) is justified in the PR body (PyPI nemo-evaluator caps at 0.3.0, so the 0.4.x toolchain the golden runs use is unreachable) with the PyPI path preserved as an escape hatch. Not blocking on design.
Licensing / injection: no LICENSE/SPDX/NOTICE changes, no vendored third-party code (nel-next.sh keeps its existing Apache header), no third-party copying, and no prompt-injection attempts in the PR content.
Previously flagged, now resolved (dropping): unpinned NEL_NEXT_REF (hard SHA default + rationale comment + --version now prints the resolved spec), invalid-YAML http_pairs_dump flow mapping (and the pre-existing unquoted ${...} in the sandbox:/container_env: schema block), the unbounded error-pair retention / http_pairs_metrics.json MLflow leak (interceptor commented out and documented as canary-only in all three sites), duplicated cluster.eval_image row in terminal_bench_2_1.md, the reasoning_replay.mode "omitted" wording, the "capture is opt-in" framing for model_traffic, and CodeRabbit's cluster.container_mounts schema gap (now documented under cluster in references/nel-next.md with the target stated).
Worth a human look before merge:
- The shipped pin is still
NEL_NEXT_REF=4d081325170aababd0c8f27c58bed31a81ce82ac("main @ 2026-08-04"), while the PR body's testing section still states the validated build wasEvaluator.git@9dcca2ae. Since the entire point of the pin is that the default toolchain is the one the scored parity runs exercised, please confirm which SHA was validated and update the other. This is the one item I'd want settled rather than deferred. - 💬 Author replied the 0.3.x → 0.4.x labels were updated "across script header, reference, template and the TB2.1 recipe" — still warrants a look because two entry points remain stale:
recipes/examples/example_eval_next.yaml:12("all blocks validate against 0.3.0", three lines under a 0.4.x header) and.agents/skills/evaluation/SKILL.md(three spots: "nemo-evaluator[harbor]0.3.x", "Isolated 0.3.x venv", "require nemo-evaluator-next (0.3.x)"). SKILL.md isn't in the diff at all, and it's the first file an agent reads; removing exactly this drift is the PR's stated goal. references/nel-next.mdstill never mentionsNEL_NEXT_SPEC, while the script inverted precedence soNEL_NEXT_SPECnow wins over the git origin. Anyone with the old PyPI spec left in.envsilently gets a 0.3.x install that the same doc says "can't reach 0.4.x" (and which lacks the vendored TB2.1 registry override) with no signal. One sentence in the venv section closes this.- 💬 Author declined
!.env.examplein.gitignore(nothing shadowed today because the committed template isrecipes/env.example, no leading dot) and offered to add it — reasonable; a one-line guard against a confusing future failure mode, owner's call. - 💬 The SWE-bench instruction-template gotcha now names
configs/benchmarks/swe-bench-verified/bench.yamland adds asha256sumcheck — still no fetchable acquisition path:scp swebench_instruction.md <login>:…sources a bare filename from cwd, and "must match the source" names nothing retrievable, so a reader without the internal repo can't obtain the canonical prompt whose content the doc itself calls scoring-relevant.
The pinned ref (4d081325) and the PR body's claimed validated build (9dcca2ae) disagreed. Resolved from the run artifacts, not from memory: the scored parity runs record their interpreter as uv archive pbrmhTamqfyfp0P958af2, whose direct_url.json is commit_id 4d081325170aababd0c8f27c58bed31a81ce82ac. 9dcca2ae was a different archive -- the one an earlier `--version` check resolved days before the runs. The pin is correct; the PR body was wrong and is updated. That divergence is the bug this pin exists to prevent, observed in our own work: one unpinned spec resolved two different toolchains a few days apart, and the wrong SHA was reported as validated as a result. - example_eval_next.yaml: "validate against 0.3.0" -> the pinned build. This label was fixed once and lost when the branch was reset onto a6ddca2. - SKILL.md: relabel the three 0.3.x mentions. The file was not in the diff at all, and it is the first file an agent reads -- exactly the drift this PR removes. - references/nel-next.md: document that NEL_NEXT_SPEC overrides the git origin and its pin. The script inverted precedence, so a leftover ==0.3.* in .env silently yields a 0.3.x install that lacks the vendored TB 2.1 registry override, with no signal. - swebench_verified.md: give the instruction template a real acquisition path (from a reference run dir's container_mounts, or the eval-factory bench.yaml), state plainly that the prompt is internal with no public download, and require both sides of a comparison to mount the same file. Signed-off-by: Chenjie Luo <chenjiel@nvidia.com>
All four addressed in 96cfc99 — none were intentional declines1. SHA mismatch — settled: Resolved from artifacts rather than memory. The scored runs record their interpreter: and that archive's {"url": "https://github.com/NVIDIA-NeMo/Evaluator.git",
"vcs_info": {"vcs": "git", "commit_id": "4d081325170aababd0c8f27c58bed31a81ce82ac"}}
Worth naming plainly: this is the exact failure the pin exists to prevent, and it happened 2. Stale 0.3.x labels — fixed, and you were right that the earlier reply overstated it. 3. 4. Instruction template acquisition — fixed. Two concrete routes (a reference run dir's Only |
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: 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/SKILL.md:
- Around line 39-44: Update all nel-next documentation references in SKILL.md,
references/nel-next.md, and the aa_next recipes to match the install contract
used by nel-next.sh: refer to the package as nemo-evaluator with harbor and
export extras, replacing nemo-evaluator-next. Keep nel-next only as the wrapper
or CLI label.
🪄 Autofix
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: 4c66a199-f7c2-415a-92e4-96f3ae38862a
📒 Files selected for processing (4)
.agents/skills/evaluation/SKILL.md.agents/skills/evaluation/recipes/examples/example_eval_next.yaml.agents/skills/evaluation/recipes/tasks/aa_next/swebench_verified.md.agents/skills/evaluation/references/nel-next.md
🚧 Files skipped from review as they are similar to previous changes (3)
- .agents/skills/evaluation/recipes/tasks/aa_next/swebench_verified.md
- .agents/skills/evaluation/recipes/examples/example_eval_next.yaml
- .agents/skills/evaluation/references/nel-next.md
meenchen
left a comment
meenchen
left a comment
There was a problem hiding this comment.
Bot review (bedrock-claude-opus-5) — DM the bot to share feedback.
Re-review of PR #2063 (evaluation agent skill, docs/tooling only — 7 files +163/-65: 5 md/yaml, .gitignore, one shell script). No library code; the e2e TB2.1/SWE-bench parity table remains the right validation for this kind of change, so no unit tests are expected.
Design gate (fired on directory span): still not a new subsystem — the PR re-points defaults inside the existing NEL_NEXT_SPEC/NEL_NEXT_ORIGIN/NEL_NEXT_REF mechanism in .agents/scripts/nel-next.sh and updates skill docs. The one real design decision (default install source PyPI → upstream git) is justified in the PR body (PyPI nemo-evaluator caps at 0.3.0, so the 0.4.x toolchain the golden runs use is unreachable) with the PyPI path preserved as an escape hatch, whose precedence is now documented. Not blocking on design.
Licensing / injection: no LICENSE/SPDX/NOTICE changes, no vendored third-party code (nel-next.sh keeps its existing Apache/SPDX header), no prompt-injection attempts in the PR content.
Previously flagged, now resolved:
- SHA mismatch (the one item I'd held for): the shipped pin
4d081325…and the PR body's testing section now agree, and the body cites the recorded uv archive'sdirect_url.json(commit_id 4d081325…) as the build the scored runs executed;9dcca2aeis explained as a different, earlier archive. Settled with artifact evidence rather than deferred. - Stale 0.3.x labels:
SKILL.mdis now in the diff (all three mentions relabelled), andexample_eval_next.yaml:12no longer says "validate against 0.3.0" — it now points at "the pinned nel-next build". Script header, reference, template and both recipes are consistent at 0.4.x. NEL_NEXT_SPECprecedence: documented in the venv section ofreferences/nel-next.md, including that a leftover==0.3.*in.envsilently overrides the git origin and its pin and loses the vendored TB 2.1 registry override.- Instruction-template acquisition: two concrete routes (reference run dir's
container_mountsentry, or thebench.yamlinstruction_templatepath), an explicit "internal, no public download" statement, andsha256sumon both ends plus the "same file on both sides of a comparison" requirement. - Earlier items confirmed still fixed: hard-SHA pin + rationale comment,
--versionprinting the resolved spec, quoted flow-mapping YAML throughout,http_pairs_dumpcommented out and documented as canary-only in all three sites, mergedcluster.eval_imagerow interminal_bench_2_1.md,reasoning_replay.modedefault wording, unconditionalmodel_trafficexclude framing, andcluster.container_mountsin the schema block.
Minor, left open deliberately (no action needed for approval): !.env.example in .gitignore (nothing shadowed today — the committed template is recipes/env.example, and there is no tracked root .env*); the Fargate quota is still not named at the two "check N × concurrency" sites; a user-supplied NEL_NEXT_SPEC that omits [export] would break mlflow-push, though the documented example spells the extras out; CodeRabbit's nemo-evaluator-next → nemo-evaluator naming nit is cosmetic since the skill uses it as a label, not an install spec.
Complex PR: spans 6 directories (≥ 5). Looping in a human for approval.
Edwardf0t1
left a comment
Edwardf0t1
left a comment
There was a problem hiding this comment.
Two last items from the review pass, both non-blocking — the substantive findings are all addressed now, and the SHA resolution via the run's uv direct_url.json in 96cfc99 is a better answer than the pin I pushed (I picked main's HEAD, you picked the build that actually ran).
|
|
||
| - **`eval_image`** = `${NEL_NEXT_EVAL_IMAGE}`. `0.3.1.1-harbor` is multi-arch and is | ||
| the minimum for **TB 2.1**; older `0.17.x/0.18.x-harbor-<arch>` are arch-suffixed. | ||
| - **`eval_image`** = `${NEL_NEXT_EVAL_IMAGE}` → `0.5.0.1-harbor` (multi-arch). Re-check |
There was a problem hiding this comment.
Worth one sentence recording why this pair is safe, rather than a change.
The login-side CLI is now pinned to nemo-evaluator 0.4.0 (nel-next.sh NEL_NEXT_REF=4d081325), and it renders config_<bench>.yaml from the benchmark model dump; the compute node then parses that file inside eval_image = 0.5.0.1-harbor. All the config models are extra="forbid" and the dump carries non-None defaults, so a field renamed or dropped between 0.4.x and 0.5.x surfaces as a pydantic ValidationError after the nodes are allocated and the checkpoint is in vLLM — and --dry-run can't catch it, since it validates against 0.4.0 on the login node. Before this PR the pair was matched (0.3.x CLI + 0.3.1.1-harbor).
The strong counter-argument is that the TB2.1/SWE-bench parity runs passed on exactly this pair, so the combination is empirically validated even though the version numbers look two minors apart. If that's the reasoning, stating it here ("0.4.0 client + 0.5.0.1 image is the validated combination — the image is on its own release train, not the client's") turns a number mismatch that a future reader will trip over into a documented invariant, and tells them what to re-check when either side moves.
| | `max_concurrent` / `sandbox.concurrency` | `50` (canonical bench.yaml) | | ||
| | timeout_strategy | `max` (canonical bench.yaml) + `agent_kwargs.llm_kwargs.timeout: 3600`; use `task` for leaderboard-comparable | | ||
| | `cluster.eval_image` requirement | **≥ `0.3.1.1-harbor`** — TB 2.1's task set is pinned via a vendored registry override in that image (`${NEL_NEXT_EVAL_IMAGE}`, multi-arch) | | ||
| | `cluster.eval_image` | **`0.5.0.1-harbor`** (`${NEL_NEXT_EVAL_IMAGE}`, multi-arch) | |
There was a problem hiding this comment.
Optional, and squarely the owner's call — but this PR exists partly because the previous shared value (≥ 0.3.1.1-harbor) had to be chased through three files, and the change re-creates that shape one level up.
After this PR 0.5.0.1-harbor appears in 5 places (references/nel-next.md:148, example_eval_next.yaml:82, this row, terminal_bench_2_1.md:58, swebench_verified.md:23) and exclude_patterns: ["shard*", "model_traffic.jsonl"] in 4 (nel-next.md:157, example_eval_next.yaml:110, terminal_bench_2_1.md:29, swebench_verified.md:28). Same for request_timeout: 3600 and the four-entry drop_params list. None of these are per-benchmark deltas — they're shared defaults, and both task files already point at references/nel-next.md.
The failure mode is quiet: the next image bump or drop_params change updates some copies, and a stale one becomes the value someone uses for a scored run without any signal. If the per-benchmark tables are meant as a standalone checklist, keeping them but marking the shared rows ("shared — see references/nel-next.md") would get most of the protection without restructuring.
- references/nel-next.md: document that client 0.4.x + eval_image 0.5.0.1-harbor is
a validated pair, not a mismatch. The image is on its own release train. The login
node renders config_<bench>.yaml from the client's model dump and the compute node
parses it inside the image, and every config model is extra="forbid", so a field
renamed or dropped between the two versions fails as a pydantic ValidationError only
after the nodes are allocated and the checkpoint is loaded -- which client-side
--dry-run cannot catch. Safe because the parity runs executed this pair, not by
construction: re-canary whenever either side moves.
- Mark the shared rows in both task tables ("shared -- see references/nel-next.md").
eval_image, request_timeout, drop_params and exclude_patterns are not per-benchmark
deltas; after this PR each appears in 3-4 files, which re-creates the scattered-value
problem the PR set out to remove. The tables stay usable as standalone checklists,
but a stale copy is now visibly a copy.
- references/nel-next.md: restore the exclude_patterns correction (capture is on by
default, so the exclude is unconditional; http_pairs_metrics.json also holds bodies
and headers). It was applied to swebench_verified.md but lost from the shared
reference when the branch was reset onto a6ddca2.
- Package naming: nemo-evaluator-next is the eval *image* repo; the pip package is
nemo-evaluator[harbor]. Corrected the two spots that meant the package. Image and
repo references are unchanged.
Signed-off-by: Chenjie Luo <chenjiel@nvidia.com>
All three addressed in a5cdf27@Edwardf0t1 — client 0.4.x + image 0.5.0.1-harbor. Documented as an invariant in @Edwardf0t1 — duplicated shared values. Verified the counts: While marking them I found that the @coderabbitai — package naming. Partially applied. The pip package is |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- Candidate files ---'
fd -a -t f '^(nel-next\.md|nel-next\.sh|example_eval_next\.yaml|swebench_verified\.md|terminal_bench_2_1\.md)$' .agents
printf '%s\n' '--- nemo-evaluator occurrences with context ---'
rg -n -C 3 --fixed-strings 'nemo-evaluator' .agents
printf '%s\n' '--- nemo-evaluator-next occurrences with context ---'
rg -n -C 3 --fixed-strings 'nemo-evaluator-next' .agentsLength of output: 40338
You are interacting with an AI system. |
|
4 participants
What does this PR do?
Type of change: Documentation / tooling (agent skill)
Terminal-Bench 2.1 configs generated from the
evaluationskill had drifted from thecanonical eval-factory config (
configs/benchmarks/terminal-bench-2.1/bench.yaml). Thescoring contract already matched the reference configs exactly — playbook,
repeats: 8,timeout_strategy: max,run_timeout: 7200,llm_kwargs.timeout: 3600, concurrency. Whathad drifted was the toolchain and a few proxy-level defaults.
The drift was in the skill, not in individual configs: a config generated fresh from the
skill reproduced every stale value, so patching configs alone would not have held.
nel-next.shinstalls from the public upstream repo(
github.com/NVIDIA-NeMo/Evaluator, default branch →0.4.0) instead of PyPI. PyPInemo-evaluatortops out at0.3.0and cannot reach the 0.4.x toolchain the reference runs use.NEL_NEXT_SPECbecomes the PyPI escape hatch and now takes precedence when explicitly set;NEL_NEXT_ORIGINstays overridable from.envso internal mirrors stay out of this repo.eval_image: document the pinned0.5.0.1-harbor(single source of truth:configs/shared/nel_next_containers.yaml) rather than0.3.1.1-harboras a floor.proxy.request_timeout1800 → 3600 — must be>=the solver'sllm_kwargs.timeout,else the proxy truncates long agent turns the harness is still awaiting.
drop_params: addmax_input_tokens_per_task,no_rebuild— sent by the 0.5.x harboreval image; vLLM returns 400 unless stripped.
exclude_patterns: addmodel_traffic.jsonlso captured request bodies stay in the rundir and never reach MLflow.
http_pairs_dumpinterceptor (last in chain) for HTTP diagnostics.max_concurrent/sandbox.concurrencyare per shard, soshards: Nmultiplies both serving capacity and live sandboxes (N x concurrency)..gitignore: broaden.env/.env-*to.env*so secret backups such as.env.bak-tb21cannot be staged.This does not move the benchmark. The TB2.1 task set is pinned by a vendored registry
override that has not changed since 2026-06-03, and both
0.3.1.1-harborand0.5.0.1-harborscore 89 samples — so the image bump is a toolchain fix and scores staycomparable across it.
Usage
Testing
End-to-end parity runs. Configs generated from these changes were run to completion on
aws-cmh (4x GB300 aarch64, sm_103) with an NVFP4 checkpoint of Qwen3.6-35B-A3B, and compared
against the reference BF16 results for the same base model:
[0.4215, 0.4661][0.4233, 0.4644][0.6920, 0.7104][0.6950, 0.7130]Both
pass@1over the full task sets (89 x r8 = 712 trials; 500 x r5 = 2500 trials), withoverlapping 95% CIs in both cases.
This exercises the changes in this PR directly: the
0.5.0.1-harboreval image, theproxy.request_timeout >= llm_kwargs.timeoutfix, the four-paramdrop_params, theper-benchmark interceptor ordering, and the SWE-bench
system_message/instruction_templaterequirements. A wrong tool-call parser, sampling preset, orinstruction template would each have moved these numbers well outside the intervals.
Also verified:
nel-next.sh --version->0.4.0, built fromEvaluator.git@4d081325— the commit thescored runs above actually executed, confirmed from their recorded uv archive
(
direct_url.json->commit_id 4d081325170aababd0c8f27c58bed31a81ce82ac). This is the SHANEL_NEXT_REFnow defaults to.eval run --dry-runwith no schema errors. These schemas areextra="forbid", sohttp_pairs_dumpand the newdrop_paramsentries would hard-fail ifunsupported by the pinned toolchain.
0.5.0.1-harborresolves in the generatednel_eval.sbatch; the tag is multi-arch(linux/amd64 + linux/arm64) and imported successfully on aarch64 compute nodes.
pre-commit run --files <changed>- all hooks pass, no file modifications.dl/JoC/competitive_evaluation/nvidia-eval-factory-benchmarking@main.Before your PR is "Ready for review"
NEL_NEXT_SPECrestores the previous PyPI install.CONTRIBUTING.md: N/A--dry-run+ pre-commit.Additional Information
Personal run configs under
.agents/skills/evaluation/runs/are deliberately not included:they carry internal cluster hostnames, lustre paths, account names and an AWS account id, which
do not belong in this public repo.
Summary by CodeRabbit
Documentation
nemo-evaluator0.4.x and the0.5.0.1-harborimage.Configuration
.env-prefixed name are now ignored.