mirror of
https://github.com/NVIDIA/Model-Optimizer.git
synced 2026-10-02 03:14:52 +08:00
Chenjie LuoandClaude Opus 5.5
80e04b8816
### What does this PR do? Type of change: documentation (agent skill) + template bug fixes Aligns the `evaluation` skill's three upstream-tracked benchmarks (Terminal-Bench 2.1, SWE-bench Verified, MRCR) with the current `nvidia-eval-factory-benchmarking` configs, and fixes guidance that turned out to be wrong when a full three-benchmark campaign was run with the skill end to end. Rebased on #2499. The first commit is the alignment. The rest address review and a fresh config-generation test: MRCR serving scoped per variant, `limit_samples` canary guidance, the template's serve command passing `--gpu-memory-utilization` (replacing `command:` had silently dropped it, so the 1M golden's 0.95 never reached vLLM), upstream's 128K values, and regression tests for the `++limit` gate and the serve command. The skill text keeps only the rules; the evidence behind them is below. GDPVal is out of scope. It was removed from this skill in #2470, and this PR replaces #2464. **Alignment with upstream** - Sandbox region via `HARBOR_ECS_REGION`. TB2.1's ECR repo name tracks the region; SWE-bench's stays in us-west-2. - One interceptor order for both harbor benchmarks, with `http_pairs_dump` **last**. Upstream is split on its position, which changes only what the dump records, never the score. Interceptor lists replace wholesale on merge, so a leaf must restate the whole chain. - `capture_request_body` goes on the service. A shared block injects an alias-only entry with no `type`. - MLflow tags gain `task_name` and `nemo-evaluator-next-version`. - TB2.1 `max_concurrent`: 50 for nano-class models, 15 for larger ones (all upstream non-nano leaves override it to 15). - `proxy.request_timeout` must always be set explicitly. Otherwise it inherits the model fragment's serving value, which ranges from 3600 to 36000 upstream, and TB2.1 has no benchmark key for it. - MRCR: - `parallelism` is 512, deliberately above server capacity, so `--max-num-seqs` must no longer be derived from it. - `limit_samples` now reaches the gym through a gated `++limit`. - Observability capture is on. - 128K is its own upstream benchmark on the condensed gym schema. **Corrections found by running it** | what the skill said | what actually happens | |---|---| | `username: ${oc.env:USER}` | nel-next only expands `${VAR}` / `${VAR:-default}`. The config passes `--dry-run` and fails at `--submit` with *"remote username contains invalid characters"*. | | MRCR canary via `++limit` edited into `collect_rollout_params` | `limit_samples` is now gated through. Under the 0.2.6 launcher the `-o` path is `++evaluation.nemo_evaluator_config.config.params.limit_samples`. `++config.params…` creates a bogus top-level key. | | the condensed gym schema's bootstrap is in the runtime image | It lives in upstream `configs/models/gym_eval_command.yaml`, which is composed in. A standalone config must carry the `command:` block. | | `mean/prefix_matched ~0.55 is healthy` | That value is calibrated to the 1M golden. A 128K run at `pass@1` ≈ 95 measured ≈ 1.0. The signal is a collapse toward 0. | | NVFP4 MoE `VLLM_*` env vars as reliable knobs | They are build-dependent: one vLLM build logged them as unknown and ignored them. Check the server log once per image. | **Rules the skill lacked** - **MRCR variant.** Pick the largest variant within the checkpoint's trained context. On a 262K-context model, 1M needs `VLLM_ALLOW_LONG_MAX_MODEL_LEN` and measures extrapolation, which a quantization comparison would then entangle with quantization damage. The serving setup follows the variant: 128K serves at the trained context without the override. - **SWE-bench `reasoning_effort`.** openhands-sdk sends `reasoning_effort: high` on every call, and canonical `bench.yaml` doesn't strip it. A server whose accepted set excludes `high` returns HTTP 400 on the first call of every trial, so `pass@1` is 0. The fix is to overwrite it with the server default via `proxy.extra_body`. terminus-2 (TB2.1) and Gym's `simple_agent` (MRCR) never send the key (46/46 and 110/110 requests checked). - **Reasoning toggles** go in `extra_body.chat_template_kwargs`. Don't write out no-op sampling defaults; `top_k` is *not* one (vLLM's default is `-1`). - **Upstream model fragment.** Consult `configs/models/<model>/` when it exists, for serving flags, the thinking toggle and `reasoning_replay.mode`. ### Usage No API change. Regenerating a config from the skill now yields the aligned values: ```yaml # recipes/examples/example_eval_next.yaml services: model: proxy: request_timeout: 3600 # always explicit benchmarks: - max_concurrent: 50 # nano-class; larger models 15 sandbox: region: ${HARBOR_ECS_REGION:-us-east-1} cluster: username: ${USER} # NOT ${oc.env:USER} ``` ### Testing - `python -m pytest plugins/modelopt/skills/ -o addopts=""`: 7/7 pass, including the new `tests/test_example_mrcr.py`. It checks that `example_mrcr.yaml` emits `++limit=N` only when `limit_samples` is set, and that the folded vLLM serve command shell-parses with every flag intact, including `--gpu-memory-utilization`. Each check fails when its defect is reintroduced. - `markdownlint-cli2` on the changed Markdown: 0 errors. Both example YAMLs parse. The full `pre-commit` suite was not run after the squash, because the sandbox could not fetch hook repos. The pre-squash commits passed it. - **Exercised end to end.** Configs built from this skill ran a BF16 campaign for a 262K-context MoE reasoning model on an internal cluster to completion: - MRCR-128K: 1470/1470 rollouts - Terminal-Bench 2.1: 712/712 trials - SWE-bench Verified: 2500/2500 trials Each correction above is a defect that campaign surfaced. - **Differential check.** Terminal-Bench configs generated from the pre- and post-change skill, from the same brief and in isolation, differ on: - interceptor chain - concurrency - `top_k` - region interpolation - MLflow tags Not run: a scored evaluation of this PR itself. ### Before your PR is "*Ready for review*" - Is this change backward compatible?: ✅ <!-- Docs + a template fix; no ModelOpt API surface touched. --> - If you copied code from any other sources or added a new PIP dependency, did you follow guidance in `CONTRIBUTING.md`: N/A - Did you write any new necessary tests?: N/A <!-- Documentation and config-template values. --> - Did you update [Changelog](https://github.com/NVIDIA/Model-Optimizer/blob/main/CHANGELOG.rst)?: N/A <!-- Agent skill docs, not a user-facing ModelOpt feature/breaking change/deprecation. --> - Did you get Claude approval on this PR?: ❌ <!-- Not run. --> ### Additional Information The companion internal `eval-config` change now carries only the internal values: the ECR URLs, the region default and the cluster image notes. It points here for the generic rules. **Known gaps not fixed here**, worth a follow-up: - `references/nel-next.md:136` and `references/launcher-workflow.md:207` derive `--max-num-seqs` from `parallelism / DP`. nel-next has no `parallelism` field (its analogue is `max_concurrent`), and MRCR's 512 is deliberately not a server cap. - `references/launcher-workflow.md:200-201` makes `--max-num-batched-tokens` and `--enable-chunked-prefill` always-include defaults, but `example_eval_next.yaml` omits both. - `references/launcher-workflow.md:27` says `sbatch_comment` belongs under `execution:` and is otherwise inert, yet all three shipped examples put it under `cluster:`. - MoE detection (`--enable-expert-parallel`) is unresolvable from the facts the skill asks for when the model handle has no `-A*B` suffix. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Signed-off-by: Chenjie Luo <chenjiel@nvidia.com> Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>