Repository navigation
refactor(terminal-bench-4): per-task Compose service overrides in the profile config - #5159
Conversation
… profile config Move the three task-name Compose patches out of Environment.start() into a profile setting, environment.compose_service_overrides: short task name -> Compose service -> JSON merge patch applied to the agent's generated Compose YAML. The Terminal-Bench 4 profile carries medical-claims-processing and payments-pipeline-fix; the Terminal-Bench-Science profile carries protein-active-learning. A task name the manifest lacks stops the server at startup, and a service the task lacks fails the task at seed. The generated YAML for the three pinned packages is byte-identical to main. Signed-off-by: gwarmstrong <gwarmstrong@users.noreply.github.com>
Signed-off-by: gwarmstrong <gwarmstrong@users.noreply.github.com>
|
/ok to test b5eee78 |
|
🌿 Preview your docs: https://nvidia-preview-pr-5159-b5eee7846f82.docs.buildwithfern.com/nemo/gym |
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
SHIP — LGTM — no concerns found.
Reviewed all nine changed files and their directly affected configuration, Compose-consumer, and session-cleanup paths. The override application preserves agent-only scoping and removes inherited users after OCI normalization. The shipped profiles retain the three previous non-root adaptations, and the regression tests retain verifier/unrelated-task isolation and package immutability checks.
Static review only: tests and rollouts were not executed, and the reported byte-identical output on downloaded task packages was not independently reproduced.
Note: formal approval is not available for this request, so this review is posted as a comment only.
Review generated by codex. This was a full review. Request a full review with /review reset=true.
yaoyu-33
left a comment
There was a problem hiding this comment.
Thanks for picking this up. Moving the three non-root patches into the profiles keeps task names out of the shared server, and I found no blockers or risks. One note is inline.
What I checked
- I ran main's hard-coded patches and
merge_patchside by side, with the shipped YAML, for all three services. They produce the same Compose output.x-sandboxis now merged instead of replaced. That only matters if a task's own Compose file setsx-sandbox, and none of the pinned packages do. user: nullreachesEnvironmentConfigasNonethrough Gym's real config loading, for bothminiswe.yamlentry points andconfigs/terminal_bench_4.yaml. Every in-repo config that runs these tasks layers the profile'sresources.yaml.- Test results:
resources_servers/terminal_bench_4/tests: 261 passed, 1 skipped.tests/unit_tests/test_environment_validation.py: 45 passed.- Breaking any one of these makes a test fail: the shipped override blocks, the agent-only gate, the short-name lookup, the startup key check, or the missing-service check.
Other observations (no action needed here)
nemo_gym/sandbox/compose_config.py:522already hasadapt_non_root_sidecars, which the Harbor environment server uses to derive this same adaptation from image metadata. It wouldn't drop in as is:protein-active-learningstill needs itsASSAY_STATUSpatch. So it's at most a follow-up.- CI: the one failing unit test,
tests/unit_tests/test_process_supervisor.py::test_only_detached_services_survive_harness_group_termination[True-timeout], is in the process supervisor and looks unrelated to this change.
PR hygiene (nit): AGENTS.md asks for an imperative title, e.g. refactor(terminal-bench-4): move per-task Compose service overrides into the profile config. The validation list could also include the pre-commit run --all-files result.
| submission["x-sandbox"] = {"hosts": [], "resolve_environment": ["ASSAY_STATUS"]} | ||
| # Use the image's default submission user without an explicit su. | ||
| submission.pop("user", None) | ||
| overrides = self.config.compose_service_overrides.get(self.task.name.split("/")[-1], {}) |
There was a problem hiding this comment.
resources_servers/terminal_bench_4/environment.py:288
These overrides are read only inside if self.uses_compose: (L265). An entry keyed to a manifest task that has no Compose file therefore passes the startup check (app.py:106) and is ignored without an error, so the README's "a service that the task's Compose file lacks fails the task at seed" doesn't hold for that case. Could __init__ raise when the agent role has overrides but no Compose file? It could sit next to the existing "Sidecar artifacts and hooks require Compose" check (L186-L189). Alternatively, startup could reject keys whose manifest category isn't compose. The shipped entries are all Compose tasks, so nothing is affected today.
yaoyu-33
left a comment
There was a problem hiding this comment.
Approving at b5eee7846. The inline note on overrides for tasks without a Compose file is non-blocking and fine as a follow-up. The one failing unit test (test_process_supervisor.py) is unrelated to this change. Thanks!
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA-NeMo/Gym/actions/runs/40730741784 |
What does this PR do?
terminal_bench_4patched three tasks' Compose services in code, keyed by task name: Terminal-Bench 4'smedical-claims-processingandpayments-pipeline-fix, and Terminal-Bench-Science'sprotein-active-learning(#4879). Each has a service that runs as a non-root user, which cannot edit/etc/hostsorsu. So the patch turns off host-file injection, resolves a peer URL throughx-sandboxwhere the service needs one, and drops the service's explicituser. As yaoyu-33 suggested in the #4879 review, this PR moves the patches into the benchmark profiles, so each benchmark carries its own and the server no longer names tasks.The new setting
environment.compose_service_overridesmaps a short task name and a Compose service to a JSON merge patch for that service in the agent's generated Compose YAML: mappings merge, other values replace, andnullremoves a setting. As withverifier_timeout_overridesandtask_images.by_task, a task name that the manifest lacks stops the server at startup. A service that the task's Compose file lacks fails the task at seed. As before, the patches apply only to the agent environment, not to verifiers or to the pinned packages.benchmarks/terminal_bench_4/resources.yamlcarries the two Terminal-Bench 4 entries, andbenchmarks/terminal_bench_science/resources.yamlcarriesprotein-active-learning. The resources server README documents the setting.No issue: follow-up to yaoyu-33's review comment on #4879.
Validation
terminal_bench_4261 passed,tests/unit_tests/test_environment_validation.py45 passed. The two existing tests for these patches keep their expected Compose output and now read the overrides from the shipped profiles. New tests cover the merge rules, a service that the task lacks, and a misspelled task name at startup.Environment.start()writes is byte-identical onmainand on this branch. Each run used its benchmark'sresources.yamlandcompose-images.json, with only the sandbox client mocked.Rollouts
N/A: the generated Compose YAML is unchanged.
Compatibility and benchmark impact
No change for either shipped profile. A custom profile that runs
medical-claims-processing,payments-pipeline-fixorprotein-active-learningonterminal_bench_4without these profiles'resources.yamlmust now add the overrides itself.