Fix AA-next OpenHands timeout guidance - #2094
Conversation
Signed-off-by: Zhiyu Cheng <zhiyuc@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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughUpdated evaluation guidance to use the ChangesEvaluation guidance
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 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 #2094 +/- ##
==========================================
- Coverage 67.15% 66.81% -0.35%
==========================================
Files 521 522 +1
Lines 59857 61312 +1455
==========================================
+ Hits 40199 40965 +766
- Misses 19658 20347 +689
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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
cjluo-nv
left a comment
cjluo-nv
left a comment
There was a problem hiding this comment.
Bot review (bedrock-claude-opus-5) — DM the bot to share feedback.
Docs/runbook-only change (4 files, +33/-9) under .agents/skills/evaluation; no library code or tests affected, so the "no new tests" justification is reasonable. The substance — replacing the "≥ 0.3.1.1-harbor" floor with "current source of truth: 0.5.0.1-harbor" and adding a SWE-bench canary gate that rejects an active OpenHands timeout: 300 — is consistent across all four files, and I confirmed no stale 0.3.1.1-harbor "minimum" wording is left (the remaining mentions are deliberate "do not use" callouts). No licensing-relevant changes, and I found no prompt-injection content in the PR text.
Three actionable points, all documentation-consistency rather than blocking correctness:
- Possible version skew: the eval image is bumped to
0.5.0.1-harborwhile.agents/scripts/nel-next.shstill pins the submitting CLI tonemo-evaluator[harbor,export]==0.3.*andreferences/nel-next.mdis titled/structured around "nemo-evaluator 0.3.x". If the harbor image tag tracks thenemo-evaluatorversion, the runbook now tells users to combine a 0.3.x launcher with a 0.5.x runtime with no note about whether that's supported. - The new canary paragraph in
references/nel-next.mdsays "for the recipe below,3600", but there is no recipe below in that file — the 3600 value lives inrecipes/tasks/aa_next/swebench_verified.md. - The
≥ 0.3.1.1-harbor — FEP-1085 reasoning fixrationale was dropped from the SWE-bench table; keeping both required fixes (reasoning + timeout propagation) documented helps anyone who has to pick a different image tag.
| - **`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. | ||
| Private gitlab-master image → cluster needs enroot creds (SKILL Step 7.5). | ||
| - **`eval_image`** = `${NEL_NEXT_EVAL_IMAGE}`. The current internal source of |
There was a problem hiding this comment.
Bot comment.
Compatibility check: this now pins the eval runtime to 0.5.0.1-harbor, but .agents/scripts/nel-next.sh still installs the submitting CLI as nemo-evaluator[harbor,export]==0.3.*, and this file's header ("nel-next (nemo-evaluator 0.3.x)") plus the "Separate venv" section describe 0.3.x. If the harbor image tag tracks the nemo-evaluator version, users would be running a 0.3.x launcher against a 0.5.0.1 runtime. Since the config schemas are extra="forbid", a config that validates under --dry-run (client side, 0.3.x) could hard-fail inside the container, or vice versa. Please either (a) note explicitly that the image tag is independent of the client package version, or (b) bump/loosen NEL_NEXT_SPEC and the surrounding 0.3.x prose in the same PR.
|
|
||
| For a SWE-bench canary, inspect the OpenHands startup/agent log before scaling | ||
| out. The configured `agent_kwargs.llm_kwargs.timeout` must appear as the active | ||
| LLM timeout (for the recipe below, `3600`); `timeout: 300` means the eval image |
There was a problem hiding this comment.
Bot comment.
"for the recipe below, 3600" — there is no recipe below in this file (the next paragraph is the eval run / SLURM description); the agent_kwargs.llm_kwargs.timeout: 3600 value lives in recipes/tasks/aa_next/swebench_verified.md. Suggest rewording to e.g. "the value configured in the SWE-bench recipe (recipes/tasks/aa_next/swebench_verified.md, currently 3600)". Also, this paragraph largely restates the new "Canary gate" section in swebench_verified.md; consider keeping the detail in one place and cross-linking so the two can't drift.
| | `sandbox.region` | `us-east-2` | | ||
| | `sandbox.ecr_repository` | `${HARBOR_SWEBENCH_ECR_REPOSITORY}` (dedicated `harbor-swebench` repo, **us-west-2**, regardless of sandbox region) | | ||
| | `cluster.eval_image` | `${NEL_NEXT_EVAL_IMAGE}` (needs **≥ `0.3.1.1-harbor`** — FEP-1085 reasoning fix) | | ||
| | `cluster.eval_image` | `${NEL_NEXT_EVAL_IMAGE}` (current internal source of truth: **`0.5.0.1-harbor`**) | |
There was a problem hiding this comment.
Bot comment.
This drops the previous rationale (FEP-1085 reasoning fix) for the image floor. 0.5.0.1-harbor presumably includes it, but readers who need to substitute a different tag now have no record of that constraint — and the tag ordering here isn't obviously monotonic (the reference file describes 0.17.x/0.18.x-harbor as older than 0.3.1.1-harbor), so "newer tag ⇒ includes the fix" isn't safe to infer. Suggest listing both required fixes, e.g. "must include the FEP-1085 reasoning fix and the OpenHands timeout propagation fix (Evaluator#1083)".
cjluo-nv
left a comment
cjluo-nv
left a comment
There was a problem hiding this comment.
@Edwardf0t1 I'm working on PR to upgrade these two benches to the latest harbor 0.5 which had a lot overlap on this PR and it also covers other configs to make the generated config similar with the golden VPR config. Maybe we can wait a bit and I will share the PR once the test runs pass.
2 participants
What does this PR do?
Type of change: Bug fix
Updates the AA-next evaluation runbook to require the current
0.5.0.1-harborruntime for Terminal-Bench 2.1 and SWE-bench, and adds aSWE-bench canary gate that rejects an active OpenHands
timeout: 300.The prior
0.3.1.1-harborguidance acceptedsolver.agent_kwargs.llm_kwargs.timeout: 3600, but that runtime did not passthe value to the OpenHands LLM client. OpenHands therefore retained its
300-second default and failed slow model generations with
LLMTimeoutError.NVIDIA-NeMo/Evaluator#1083 fixed the propagation path;
0.5.0.1-harboris thecurrent internal benchmark source of truth and includes that fix.
The source paths in the benchmark recipes are also updated to the current
nvidia-eval-factory-benchmarkinglayout.Companion internal config-writer MR:
https://gitlab-master.nvidia.com/omniml/Model-Optimizer-Internal/-/merge_requests/111
Usage
Keep
agent_kwargs.llm_kwargs.timeout: 3600in the SWE-bench solver config anduse the
NEL_NEXT_EVAL_IMAGEwritten by the currentmodelopttools:eval-configskill. During the first canary, confirm the OpenHands agent reports an active
timeout of
3600; do not scale out a canary that reportstimeout: 300.Testing
pre-commit run --fileson all four changed filesgit diff --checkconfigs/shared/nel_next_containers.yamlin the internal benchmark source oftruth
Before your PR is "Ready for review"
CONTRIBUTING.md: N/AAdditional Information
Upstream propagation fix:
NVIDIA-NeMo/Evaluator#1083
Summary by CodeRabbit
0.5.0.1-harbor.