Repository navigation
Conversation
Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
HOLD — one BLOCKER and one RISK.
Flavor selection can silently retain the original harness, and a failed parallel fan-out can outlive session close. Reviewed all 30 changed files and their affected session, configuration, and resource-verification paths. Static review only; tests and the reported model smoke runs were not independently executed.
Note: formal approval is not available for this request, so this review is posted as a comment only.
Inline comments (escaped v1):
-
responses_api_agents/langgraph_agent/configs/orchestrator_agent.yamlline 6: BLOCKER — Preserve the public flavor-selection workflowFailure: Selecting
gym eval run --benchmark aime24 --agent-type langgraph_agent/orchestrator_agentnow leaves the benchmark's SimpleAgent in place instead of selecting the graph. The previous template marked itself as a swap source withresources_server.name: ???. Withnull,_is_unbound_agentandcompose_unbound_agentfind no source. The AIME agent and dataset therefore remain unchanged, while an unrelated orchestrator instance is added. The CLI selector only loads this file; it provides no replacement mechanism. A completed evaluation can consequently measure a different harness from the one explicitly requested. The parallel-thinking and ReWOO templates lose the same swap marker.Correction: Could you preserve explicit harness selection while retaining resource-independent session templates? Update the selector/composer contract—particularly
_translate_agent_type, source selection incompose_unbound_agent, and the corresponding flavor configuration—so an explicitly selected flavor replaces the workload agent, inherits its resource/model/dataset bindings, and retargets its existing Environment Server. The invariant is that a successful explicit harness selection cannot leave the original workload agent running. Do not globally classify every null-bound agent as a swap source: directly referenced session agents must remain independent. The exact implementation needs that distinction, so I am not suggesting an unverified global parser patch.Preservation and acceptance — proposed, not executed: Parse AIME24 with each flavor through the real selector. Assert exactly one replacement LangGraph workload agent, the requested entrypoint, inherited AIME resource/model/dataset bindings, and the existing Environment Server pointing to it; assert no orphan flavor instance or extra relay remains. Retain the direct session-composition assertions: null resource binding, one worker, and exactly one explicitly supplied session environment. Also retain the twelve pairing checks and ensure unrelated self-contained agents are not swapped.
-
responses_api_agents/langgraph_agent/parallel_thinking_agent.pyline 121: RISK — Drain sibling model calls when one parallel path failsFailure: With two paths, let both model calls enter, then make one raise while the other remains pending. This
asyncio.gatherpropagates the exception without cancelling or awaiting the unfinished sibling. The activation can therefore finish exceptionally while a model request still runs. The new stateless close hook drains only the activation ledger; the ledger close sees an already-failed activation and returns. Session close can thus acknowledge cleanup while the sibling continues consuming model resources or writing capture evidence. Although this gather predates the PR, the newly added session lifecycle now relies on activation completion to establish that execution has drained.Correction: Could you give
parallel_thinkstructured ownership of all its path tasks? Replace this line with the following suggested code, verifying it before adoption:async with asyncio.TaskGroup() as group: tasks = [group.create_task(_single_path(p)) for p in prompts] results = [task.result() for task in tasks]The invariant is that the node cannot terminate, successfully or exceptionally, while any path task is still executing or finalizing.
TaskGrouppropagates grouped failures, so check the resulting error presentation as well; this suggestion was not executed.Preservation: Keep the calls concurrent, retain results in configured prompt order rather than completion order, aggregate every successful path's outputs and usage, and keep selecting the last configured path's cookies for synthesis. Close during an otherwise healthy fan-out must still cancel and drain every path.
Acceptance — proposed, not executed: Add a two-path session regression where one call raises and the sibling records cancellation, then waits in its finalizer. Assert neither activation completion nor successful close precedes release of that finalizer; afterward assert the sibling stopped, synthesis was never called, and identical retries launch no additional model calls. Separately close during two pending healthy paths and assert both finalizers finish. Retain the successful aggregation assertions: three model calls and input/output/total usage of 33/63/96. Add reversed completion order and verify synthesis still receives paths in configured order and returns its final output.
Review generated by codex. This was a full review. Request a full review with /review reset=true.
Resolve conflicts with main's module moves (nemo_gym.client, nemo_gym.server_runtime, nemo_gym.metrics, nemo_gym.trajectory), keep report_rollout_stop beside the new StatelessResponsesAPIAgent, and list langgraph_agent in the central AGENT_SESSION_AGENT_TYPES set so swaps and the legacy-relay script treat it as a session agent. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
Merge current main and replace host graph execution with sandbox runtime installation, supervised runners, retained agent sessions, and explicit owned or borrowed sandbox lifecycle. Preserve existing prompts and model capture routes. Export shared agent utilities and update their existing consumer imports to satisfy the public API contract. Validation: 102 LangGraph tests; 242 core tests (3 platform skips); 309 public API facade tests; remaining import checks passed in the preceding run. Full pre-commit hooks passed after formatting; unrelated SWE-bench import-order autofix discarded. Fern check: 0 errors, 1 warning. Real EOS validation in progress; PR remains draft. Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
Retain remote merge 47729f6 in ancestry. Its import relocations and central session registry are already present; sandbox sessions supersede the stateless helper and its inheritance-specific test. No tree changes relative to the validated sandbox implementation. Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
Restore nineteen out-of-scope files exactly to main 0c7ac51. The sandbox implementation, configs and required public utility imports are unchanged. Exact final tree passes pre-commit run --all-files with no modifications. Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
HOLD — one unresolved BLOCKER.
Explicit flavor selection can still leave the benchmark's original harness running. The prior parallel-sibling RISK is fixed: parallel_think now owns its branches through a TaskGroup, and the regression test asserts that the pending sibling has stopped after another branch fails.
This follow-up checked both prior findings and subsequent sandbox-execution changes against the previously reviewed tree. No additional findings met the historical impact-escalation threshold. Static review only; I didn't execute tests or independently reproduce the reported smoke runs.
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 findings-only review. Request a full review with /review reset=true.
| sandbox_config: | ||
| image: ubuntu:24.04 | ||
| workdir: /workspace | ||
| resources_server: null |
There was a problem hiding this comment.
BLOCKER — Preserve explicit flavor selection (unresolved prior finding)
Failure: With valid model/provider configuration, selecting gym eval run --benchmark aime24 --agent-type langgraph_agent/orchestrator_agent still leaves the benchmark's SimpleAgent selected. This is the same failure reported in review 5717622551: the previously reviewed flavor also used a null resource binding. The selector only loads this file. Null isn't a resource reference, so source detection and composition find no unbound agent and return without replacing anything. The AIME workload, dataset and Environment Server reference remain on SimpleAgent; the unrelated flavor instance instead receives a generated relay. Evaluations can therefore measure a different harness from the one explicitly requested. Parallel-thinking and ReWOO retain the same failure.
Correction: Could you carry explicit harness-selection intent from _translate_agent_type into composer source selection, independently of resources_server: null? Update compose_unbound_agent and its source-selection helper _composed_instance_names consistently, retaining held overrides, inherited resource/model/dataset bindings, and Environment Server retargeting. The invariant is that successful explicit selection replaces the workload harness, rather than adding an unused instance. Don't classify every null-bound agent as a swap source. The composed model-only LangGraph pairing also needs an explicit empty tool-transport list, since the adapter rejects required grants; don't strip grants from unrelated harnesses. This needs a selector/composer contract distinction, so a one-line YAML or global null-detection patch isn't a safe complete fix.
Preservation and acceptance — proposed, not executed: Through the real CLI selector, resolve AIME24 with each flavor. Assert one replacement LangGraph workload agent with the requested entrypoint and inherited AIME resource/model/dataset bindings; the existing Environment Server must reference it, use no tool grants, and leave no SimpleAgent or orphan flavor relay. Retain direct session composition: directly referenced null-bound flavors remain independent, with one worker and exactly the supplied session environment. Retain the twelve pairing checks, including empty grants and matching resource references, and check that unrelated self-contained agents aren't swapped.
Address the review of the sandboxed LangGraph adapter: - Send only caller-set request fields from the host and from each graph model call (exclude_unset), as ServerClient did. Explicit nulls were overriding Model Server extra_body sampling defaults. - Return an incomplete response when the supervisor stops a graph at its deadline, instead of a 500 the Environment Server retries and never verifies. Wire rollout stop to the session, and raise the runner ceiling default to 21600 s so the episode budget, not a new 600 s cap, decides. - Keep the activation's trace context in the sandbox model URL. - Pass env/user to the shared SandboxSession launch only when set, so other adapters' exec-only test fakes keep working. - Constrain the sandbox install to the Agent Server's resolved dependency versions, and pin the graph runtime in requirements.txt so CI runs the workflow and runner tests. - Document that --agent-type no longer swaps the flavors in, add the sandbox provider config to the environment READMEs, and list LangGraph among the adapters that still install with uv in the sandbox. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
|
/ok to test 9cb8e2f |
|
🌿 Preview your docs: https://nvidia-preview-pr-5377-d3a6380e0361.docs.buildwithfern.com/nemo/gym |
Resolve the OpenClaw import conflict by keeping main's agent-outcome import alongside the branch's public agent-utils imports. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
|
/ok to test b077542 |
Keep the managed pyproject.toml and uv.lock layout from main while retaining LangGraph 1.2.18 and langchain-core 1.7.3. Update the installation note to name the managed dependency files. Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
|
/ok to test de14559 |
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA-NeMo/Gym/actions/runs/41896731910 |
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA-NeMo/Gym/actions/runs/41897945593 |
Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
Signed-off-by: yaoyu-33 <yaoyu.094@gmail.com>
|
/ok to test d3a6380 |
LangGraph's reflection, parallel-thinking, orchestrator and ReWOO workflows now support agent sessions while executing directly in the Agent Server. These graphs only call the Gym Model Server, so task sandboxes, per-task runtime installation and sandbox runners are removed.
Identical activations join/replay one graph; close cancels and drains it before verification. Capture routes, sampling defaults, aggregate usage, resource cookies and verifier metadata are preserved. Parallel branch failures cancel and drain their siblings. The twelve existing pairings use
single_agent_turn_legacy, one agent worker and no tool grants.Explicit flavor selection now works:
gym eval run --benchmark aime24 --agent-type langgraph_agent/orchestrator_agent --model-type vllm_modelreplaces the benchmark's agent and retains its model, resources and dataset bindings. Only environments retargeted to the model-only graph lose tool grants. Loading a flavor with--configpreserves custom null-bound session wiring.This is the internally tracked LangGraph session migration; no separate public issue is required.
Compatibility:
/runremain available.restart_only. Native invocation/turn evidence, checkpoint continuation and training qualification are not claimed.Validation on
830a4bc1bd4d1fb3f655a2c11aac1a9f055e6ff9, including main23abf8264:python -m pytest tests/unit_tests/test_cli_main.py tests/unit_tests/config/test_global_config.py tests/unit_tests/test_add_legacy_agent_environment_servers.py tests/unit_tests/test_public_api.py -q --disable-warnings --maxfail=2: 719 passed, no skips.python -m pytest responses_api_agents/langgraph_agent/tests -q --disable-warnings --maxfail=2: 45 passed, no skips, in its isolated invocation.pre-commit run --all-files: passed; both new commits have DCO sign-off.ab0a6611; Fern reported 0 errors and one unavailable authenticated-redirect warning.Real-model smoke:
ab0a6611, one unchanged checked-in reasoning-gym sample per flavor completed with reward 1, using NIMnvidia/nemotron-3.5-bolt-e4b, temperature 0, 2048 output tokens per call and thinking disabled. Reflection/parallel/orchestrator/ReWOO made 1/5/6/5 captured model calls, all HTTP 200. Usage sums matched; close preceded verification; all task processes exited.830a4bc1, stock end-to-endgym eval run --resources-server reasoning_gym --agent-type langgraph_agent/reflection_agent --model-type vllm_model --split trainran one checked-in sample through a task-local dataset override. Only the requested LangGraph agent was present. Collection exited 0 with a completed response, five captured HTTP-200 model calls, matching usage, close before verification and no remaining processes. All 33 changed paths matched the committed source. Reward was 0: the existing reasoning-gym verifier scored the full five-message reflection transcript; rescoring the final answer alone yields 1. The recorded outcome is retained. Initial smoke-driver setup errors occurred before inference; its reward-1 assertion also failed after the successful collection.HTTP statuses above are captured Gym Model Server responses; a separate provider HTTP status was not recorded. These are execution smokes, not benchmark accuracy, parity or training qualification.
CI failure repairs after merging main
74b307063d116827bf42537d5d9f533f318e2a16:opencode_searchcomponent manifest and lock are fixed upstream by build(opencode_search): add the missing component lock files #5629, included here unchanged. This resolves both component-lock jobs and the five dependency-policy failures in core shard 3; the same failures were present on main.ng_agent_outcome, including the upstream status. Startup, timeout, MCP readiness and cleanup failures still raise; credential redaction is retained. The previous behavior translated the conformance model's HTTP 400 into an agent HTTP 500.Validation on
1f653c1a9(macOS):python scripts/component_locks.py: 268 projects passed, using uv 0.13.7.pytest -o addopts= --import-mode=importlib -q tests/unit_tests/tooling/test_component_dependencies.py tests/unit_tests/tooling/test_add_legacy_agent_environment_servers.py tests/unit_tests/agents/test_sandbox_session.py: 213 passed, 3 skipped.pytest -o addopts= --import-mode=importlib -q responses_api_agents/kilocode_agent/tests: 240 passed, 19 skipped. Seven HTTP-error regressions failed against the previous adapter and passed after the fix; runtime-failure checks also passed.pytest -o addopts= --import-mode=importlib -q -n 4 responses_api_agents/openclaw_agent/tests scripts/harness_conformance/tests tests/unit_tests/rollouts/harness_capabilities: 907 passed, 47 skipped, outside the local sandbox to permit loopback servers and process inspection.pytest -o addopts= --import-mode=importlib -q resources_servers/opencode_search/tests tests/unit_tests/agents/test_sandbox_supervisor_client.py: 45 passed, 3 skipped.-k langgraph: 6 passed (all four flavors plus overrides and custom config wiring).pre-commit run --all-files: passed. Merge and repair commits carry DCO sign-off.1f653c1a91a2fe702bc801c86057a7345c8e5130source; details below. The LangGraph real-model evidence above predates this main merge. The repository-wide CI matrix has not been rerun at this head.Linux follow-up on
1f653c1a91a2fe702bc801c86057a7345c8e5130(Python 3.13.14, uv 0.13.4, isolated checkout):pytest -o addopts= --import-mode=importlib -q -n 4 responses_api_agents/openclaw_agent/tests responses_api_agents/kilocode_agent/tests scripts/harness_conformance/tests tests/unit_tests/rollouts/harness_capabilities tests/unit_tests/agents/test_process_supervisor.py tests/unit_tests/agents/test_sandbox_supervisor_client.py tests/unit_tests/agents/test_sandbox_supervisor_bootstrap.py tests/unit_tests/agents/test_sandbox_session.py tests/unit_tests/tooling/test_component_dependencies.py resources_servers/opencode_search/tests: 1,389 passed, 18 skipped. The prior OpenClaw failures and all five dependency-policy failures are covered.test_exec_transport.py: 9 passed in each of three additional consecutive runs. The original tests also passed once on this host, so the reported short-deadline CI flake was not reproduced deterministically.python scripts/run_harness_conformance.py --harness kilocode --output <fresh-directory>: all 9 scenarios fulfilled, exit 0. Inmodel_error, the Environment Server receivedstatus=failed, the original HTTP 400 message andng_agent_outcome={"failure_kind":"model_request_error","status_code":400}; setup and close both completed.nvidia/nemotron-3.5-bolt-e4b, temperature 0, thinking disabled, 1024 output-token limit per call: reward 1, completed response, two HTTP-200 model calls and two bash tool calls writing/reading a task-local marker. Aggregate usage matched the captured calls (18,699 input / 76 output tokens). The existing Environment Server, agent lifecycle and verifier ran normally; a task-local smoke driver supplied the live model endpoint and task. This is execution evidence, not benchmark accuracy or parity.Final synchronization with main
653846f949ad08a29df19c7de2ee4166d1c1bab3:d3a6380e03617cda91571ff784b2f899ca0712b7resolves the duplicate OpenCode lock-file additions by keeping the upstream files from build(opencode_search): add the missing component lock files #5629 unchanged. The final PR has noopencode_searchdiff against main.1f653c1a9, onlyresources_servers/opencode_search/pyproject.tomland its lock changed. All agent/runtime code, tests, conformance tooling, root dependencies and Kilocode/OpenClaw locks remain identical.pre-commit run --all-filespassed. The merge commit carries DCO sign-off.1f653c1a9; the new revision starts fresh checks. The full repository CI matrix still requires the normal maintainer authorization.