Repository navigation
Conversation
|
/ok to test 406b66a |
|
/claude review |
| "assert Version(version('pyjwt')) >= Version('2.9'); " | ||
| "assert Version(version('sqlalchemy')) >= Version('2.0.40'); " | ||
| "assert Version(version('flask')) >= Version('2.2'); " | ||
| "import datasets, wandb" |
There was a problem hiding this comment.
NOTE: the probe's version floors (jinja2>=3.1.3, pyjwt>=2.9, sqlalchemy>=2.0.40, flask>=2.2) are decoupled from what a rebuild actually installs. openhands.sh never pins these — they come from whatever OpenHands' poetry lock resolves. If the lockfile ever resolves any of them below a floor, _existing_setup_is_reusable returns False on every server startup, so the reuse fast-path is silently disabled and each process pays a full rebuild (which resolves the same sub-floor versions and would fail the probe again next time). No infinite loop (setup() doesn't re-probe after rebuild) and no data corruption — the cost is a permanent, silent loss of the reuse optimization. If these floors encode a known-bad OpenHands version, consider pinning them in openhands.sh alongside the wandb/datasets install so the rebuild actually satisfies what the probe demands.
|
SHIP — startup-only reuse gate, well-tested, off the scoring/async hot paths. This adds a runtime probe to the OpenHands harness Blast-radius check: One non-blocking NOTE inline: the probe's version floors are decoupled from what a rebuild installs — if OpenHands' lock ever resolves a probed package below a floor, the reuse fast-path silently disables itself on every startup (permanent rebuild cost, no corruption). Consider pinning those floors in |
| "assert Version(version('pyjwt')) >= Version('2.9'); " | ||
| "assert Version(version('sqlalchemy')) >= Version('2.0.40'); " | ||
| "assert Version(version('flask')) >= Version('2.2'); " | ||
| "import datasets, wandb" |
There was a problem hiding this comment.
NOTE: the probe's version floors (jinja2>=3.1.3, pyjwt>=2.9, sqlalchemy>=2.0.40, flask>=2.2) are decoupled from what a rebuild actually installs. openhands.sh never pins these — they come from whatever OpenHands' poetry lock resolves. If the lockfile ever resolves any of them below a floor, _existing_setup_is_reusable returns False on every server startup, so the reuse fast-path is silently disabled and each process pays a full rebuild (which then resolves the same sub-floor versions and would fail the probe again). No infinite loop (setup() doesn't re-probe after rebuild) and no data corruption — the cost is a permanent, silent loss of the reuse optimization. If these floors encode a known-bad OpenHands version, consider pinning them in openhands.sh alongside the wandb/datasets install so the rebuild actually satisfies what the probe demands.
406b66a to
f389df0
Compare
f389df0 to
bb6fba8
Compare
bb6fba8 to
858a7ad
Compare
858a7ad to
e5178fa
Compare
…e build attempt (NVIDIA-NeMo#3778) <!-- Thanks for contributing to NeMo Gym! Please fill out the sections below. --> ## What does this PR do? Fixes two independent bugs in `setup_scripts/openhands.sh`'s build step, reproduced against current `main`: - `jq` is downloaded as `jq-linux-amd64` unconditionally, so an arm64 host installs a binary it can't run. The install now resolves the release asset from `uname -m`. - The `make build` retry loop wraps every attempt but the last one in `timeout "$MAKE_BUILD_TIMEOUT_SECONDS"`, so a hang on the final attempt blocks indefinitely instead of failing like the earlier attempts do. It now gets the same timeout. This covers the first two items of NVIDIA-NeMo#2993's acceptance criteria. It doesn't remove the `curl` dependency on GitHub (the third item, for air-gapped installs): the comment on this block ("avoid conda solver changing other package versions") is a deliberate reason `jq` isn't installed via the `mamba install` line above it, and changing the install source needs a direction call (vendoring the binary, an internal mirror, or something else) this PR doesn't make on its own. Part of NVIDIA-NeMo#2993 ## Validation No test harness in this repo exercises `setup_scripts/*.sh` (it's a real shell command run from `SWEAgentsEnvironment`, not something the unit suite drives), so this is functional verification rather than a pytest addition: - `bash -n` and `shellcheck` on the changed file: clean, no new warnings. - A small harness mocking `curl`/`make`/`timeout`/`uname`, sourcing just the two changed blocks, run in Docker against both `main` and this branch: on `main` with `uname -m` reporting `aarch64`, `curl` still requests `jq-linux-amd64` and only one `timeout` call fires across both `make build` attempts; on this branch the same input requests `jq-linux-arm64` and both attempts are timeout-wrapped, while `x86_64` still requests `jq-linux-amd64`. - Re-ran the file-level competing-PR search right before opening this: the only other open PR touching this file (NVIDIA-NeMo#2387) changes an unrelated line. I haven't built OpenHands end to end on an arm64 host; no such runner here. ## Rollouts N/A: this changes setup tooling, not training/eval behavior. ## Compatibility and benchmark impact None. Same amd64 behavior as before; arm64 hosts now get a working `jq` install instead of a broken one. ## Checklist - [x] I have read the contributing guidelines. - [x] The change is focused; unrelated "drive-by" edits are tracked as separate issues/PRs. - [ ] Tests added or updated and pass locally, or N/A for docs-only / non-code changes (so CI unit/server checks pass when applicable). No test harness here covers setup scripts; see Validation. - [x] Documentation added or updated, or N/A with justification. - [x] Pre-commit checks pass locally (`pre-commit run --all-files`) (so CI lint/format checks pass). - [x] New source files include the required Apache-2.0 SPDX header. - [x] All commits have DCO sign-off (`git commit -s`) (so the DCO check passes). --------- Signed-off-by: Amir Fathi <amirfathi.me@gmail.com>
Signed-off-by: Teodor-Dumitru Ene <teodord.ene@gmail.com>
Signed-off-by: Teodor-Dumitru Ene <teodord.ene@gmail.com>
e5178fa to
6bc4eb7
Compare
|
/ok to test 6bc4eb7 |
|
@sdevare-nv could you review this one? |
|
I'm not sure whether it will be an issue: the venv probe runs twice, if the first passes and the second fails, can |
The reuse gate for a pre-existing OpenHands setup was 'the venv interpreter file exists', which conflates two very different states with a healthy environment: an interrupted build (a python binary with no packages behind it) and a venv whose packages an episode has since downgraded (episodes run pip inside the sandbox).
Attempting to use pre-existing venv's in either case causes all subsequent episodes to fail and mistakenly return 0 reward.
Nothing ever rebuilds the env on
mainbecause the gate always passes.