Repository navigation
feat(core): add model retry controls and Gymnasium rollout options - #3415
Conversation
Signed-off-by: Mahan Fathi <mfathi@nvidia.com>
Signed-off-by: Mahan Fathi <mfathi@nvidia.com>
Signed-off-by: Mahan Fathi <mfathi@nvidia.com>
Signed-off-by: Mahan Fathi <mfathi@nvidia.com>
Resolve conflicts with main's retry and model-server changes: - Keep main's permanent quota and auth trips and its raise_for_status body handling. An explicit max_num_tries=1 still overrides max_http_attempts and never extends for internal rate limits. - Keep main's connection-retry cap in the transport exception path alongside the explicit _max_num_tries cap, counting the same attempts so the lower limit also applies to generic errors. - Keep main's Ray opt-out and provider-extension test in openai_model alongside this branch's upstream retry controls and nullable reasoning_effort_none_replacement. - Accept main's raise_for_status(response, content) in the explicit-cap test double. Signed-off-by: Mahan Fathi <mfathi@nvidia.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Validate known web_search_call actions to the SDK's typed models again, so attribute access, SDK-object construction, and from_attributes keep working, while unknown or missing actions stay opaque and every payload still round-trips unchanged. Guard openai_model's response rewrite against non-dict client results, and append _max_num_tries after request()'s existing parameters so positional callers keep their meaning. Signed-off-by: Mahan Fathi <mfathi@nvidia.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
yuhezhang-ai
left a comment
There was a problem hiding this comment.
The main requests are to stop backoff after a permanent provider failure, preserve error information needed by callers and model-call capture, and guard the retry settings that LiteLLM currently bypasses. I verified these with focused tests and injected failure cases; I did not repeat the live-model rollouts. The reasoning-default and documentation suggestions are nonblocking.
A spent or unauthorized key trips NeMoGymAsyncOpenAI, after which every call fails without reaching the provider. The upstream retry policy still treated that PermanentEndpointError as retryable and waited through its whole backoff schedule on every request. Re-raise it immediately. Signed-off-by: Mahan Fathi <mfathi@nvidia.com>
A propagated upstream status now carries the provider's error body, parsed as JSON when possible, instead of a generic message, so callers that look for codes such as context_length_exceeded still find them. Exhausted retries still fail with HTTP 500, but now raise UpstreamRetriesExhaustedError, a RuntimeError that keeps the final provider status and body for model-call capture. Both endpoints share one helper for the retry policy and status propagation. Signed-off-by: Mahan Fathi <mfathi@nvidia.com>
LiteLLMModelServer inherits the upstream retry, pool-timeout, and status propagation settings, but its own /v1/responses handler called the client directly, so an accepted retry policy silently reduced it to one attempt. Route the handler through the shared helper. With default settings it still makes one call under the same concurrency limit. Signed-off-by: Mahan Fathi <mfathi@nvidia.com>
The pinned OpenAI SDK and NeMoGymResponse both accept reasoning.effort 'none', so rewriting it to 'minimal' by default changed the reported inference setting, even when a caller requested 'none'. Default reasoning_effort_none_replacement to null and keep the rewrite available for configurations whose callers cannot accept 'none'. Signed-off-by: Mahan Fathi <mfathi@nvidia.com>
Replace the stale client-disconnect rationale on the provider slot, and state in the config descriptions, request() docstring, and openai_model reference that request timeouts apply per attempt, connect timeouts include waiting for an aiohttp connection-pool slot, and an explicit attempt count replaces the transport's generic-error retry limit. List the new openai_model options in its configuration reference. Signed-off-by: Mahan Fathi <mfathi@nvidia.com>
The shared Responses streaming wrapper converted every responses() failure into an HTTP 200 response.failed event, so a status that openai_model was configured to propagate never reached streaming clients. Re-raise HTTPException there: like request validation, it fails before the stream is committed. Other failures still end the stream with response.failed. Signed-off-by: Mahan Fathi <mfathi@nvidia.com>
With propagate_upstream_http_status_codes, openai_model and litellm_model return the provider's error body under FastAPI's `detail`. NeMoGymAsyncOpenAI read only a top-level error.code/type, so an internal client that received a propagated 429 insufficient_quota treated it as a transient rate limit and kept extending its retries. When a body has no top-level `error`, read the error object under `detail`, so a spent or invalid key stops a receiving Gym client as it does when that client calls the provider directly. Signed-off-by: Mahan Fathi <mfathi@nvidia.com>
Merge main at 242d468. Verified 835 focused tests, the propagated-quota regression probe, and pre-commit run --all-files. Signed-off-by: Yuhe Zhang <yuhez@nvidia.com>
|
/ok to test 1cb1e69 |
yuhezhang-ai
left a comment
There was a problem hiding this comment.
All review findings are addressed, including the propagated permanent-quota error reaching the receiving Gym client.
Reviewed the clean merge of main at 242d468 into this PR. On 1cb1e69, all 835 focused tests, the bounded quota-error regression probe, and pre-commit run --all-files passed. Permanent quota errors stop after one reply; temporary rate limits still retry and recover.
Approving the code. Full repository CI has been requested and should pass before merge.
|
🌿 Preview your docs: https://nvidia-preview-pr-3415-1cb1e692013a.docs.buildwithfern.com/nemo/gym |
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA-NeMo/Gym/actions/runs/37394585251 |
…o#4285) Pi's SDK cannot recognize context overflow when an explicitly propagated provider error is wrapped as `{"detail":{"error":...}}`: it reports “400 status code (no body)” instead of the provider message. Its HTTP 400/503 integration tests also omitted the propagation setting required by the current OpenAI Model Server. Remove the adapter's extra HTTPException conversion and use the existing shared Model Server error handler. Selected statuses now retain the original provider body, including top-level `error.message` and `error.code`. LiteLLM inherits the same correction. After Chat streaming sends a keepalive, the terminal SSE error also retains the selected provider explanation; unselected provider errors remain generic HTTP 500 messages in the stream. The Pi fixture explicitly enables the statuses it exercises. The default propagation policy remains opt-in; unselected errors and exhausted provider retries retain their HTTP 500 behavior. **Compatibility:** consumers of explicitly propagated OpenAI/LiteLLM errors must read the original provider body rather than the former `detail` wrapper. Capture retains that same served body. Follow-up to NVIDIA-NeMo#3415 and NVIDIA-NeMo#3626; related tracking: [FEA-461](https://linear.app/nvidia/issue/FEA-461). This addresses the separate Pi CI failure exposed while validating NVIDIA-NeMo#4251. Validation on this branch: - **507 passed**: OpenAI and LiteLLM adapter tests, base Model Server/Chat and Responses streaming tests, and pinned Pi 0.80.2 SDK outcome tests. Covers opt-in and default status handling, Chat/Responses, streaming/nonstreaming, capture, exhausted retries, and permanent-error handling. A delayed-error regression fails before the fix; the pinned Pi SDK recognizes HTTP 400 context overflow and HTTP 503 both before and after the first keepalive. - **Two real local HTTP smokes passed:** Pi SDK → Gym OpenAI Model Server → fault-injection provider, HTTP 400 context overflow and HTTP 503 service unavailable. Exactly one upstream request per case; Pi retains each error message and correctly distinguishes context overflow. No generative-model benchmark run: this is an error-response wire-format change. - Full `pre-commit run --all-files` and `git diff --check` passed; DCO-signed. ```bash python -m pytest -o addopts= --import-mode=importlib -q \ responses_api_agents/pi_agent/tests/test_outcome.py \ responses_api_models/openai_model/tests/test_app.py \ responses_api_models/litellm_model/tests/test_app.py \ tests/unit_tests/test_base_responses_api_model.py \ tests/unit_tests/test_responses_api_model_streaming.py \ tests/unit_tests/test_chat_completions_streaming.py ``` --------- Signed-off-by: Giulio Lovisotto <glovisotto@nvidia.com>
## What does this PR do? Follow-up to NVIDIA-NeMo#3632, split out as requested in NVIDIA-NeMo#3632 (comment). In `_request_with_retries()`, the generic `except Exception` branch handles every error except `ServerDisconnectedError` and `ClientOSError`, which have their own branches. This includes aiohttp timeouts. When the caller does not pass `_max_num_tries`, this branch counted tries only for external callers. So an internal caller (`_internal=True`) that set `_max_connection_retries` to 2 or more never reached the cap and retried forever. NVIDIA-NeMo#3415 already counts tries for callers that pass `_max_num_tries`. This PR fixes the remaining path, where `_max_num_tries` is not passed. - Count tries for internal callers too, so `_max_connection_retries` stops them. - Keep everything else the same: - internal callers with no cap still retry with no limit and print no retry message, as NVIDIA-NeMo#90 intended; - external callers still stop at `MAX_NUM_TRIES`; - callers that pass `_max_num_tries` are unchanged. - Update the comment that said internal calls are not counted. It keeps NVIDIA-NeMo#90's reason for not limiting internal calls. - Add three tests: - the cap stops internal callers, with tracing off and on; - internal callers with no cap retry silently past `MAX_NUM_TRIES`; - external callers stop at `MAX_NUM_TRIES` and print one retry message per try. - The cap test, `test_request_retry_cap_bounds_internal_callers`, replaces `test_retry_cap_bounds_internal_callers`, which was removed from NVIDIA-NeMo#3632. - Make the retry tests fail instead of hang when the loop never stops: - The test helper `_mock_global_client` (added in NVIDIA-NeMo#2383) now takes the error to raise. - Its fake `request()` calls `pytest.fail()` once it is called more often than any test needs. `pytest.fail()` raises a `BaseException`, which the loop's `except Exception` does not catch. So the test fails at once, even if the loop never sleeps. - Two NVIDIA-NeMo#3415 tests now use this helper instead of their own mocks: `test_explicit_transport_attempt_cap_disables_request_retry` and `test_request_caps_generic_errors_at_the_lower_attempt_limit`. Before, if the generic branch ignored `_max_num_tries`, the first test hung, sleeping a real 0.5 s per retry. No caller in this repo sets both `_internal=True` and `_max_connection_retries` today: - The internal callers pass no cap: `ServerClient`, the simple agent's direct tool calls to Resources servers (`tool_access`), the Terminus 2 sandboxed agent's model client (`NeMoGymAsyncOpenAI(internal=True)`), the OpenHands capture route, and the LiteLLM version check. - The callers with a cap are not internal: the remote agent and sandbox transport use 1, Tavily search uses 0, and vLLM with `endpoint_file` uses a default of 8. - The OpenAI model server and the Gymnasium agent pass `_max_num_tries` only when `upstream_max_num_tries` or `model_server_transport_max_attempts` is set. Neither passes `_max_connection_retries`, so neither changes. Related: - NVIDIA-NeMo#3553 added the cap check to this branch without counting internal tries. - NVIDIA-NeMo#3415 added `_max_num_tries` and counts tries on that path. - NVIDIA-NeMo#3159 tracks bounding every retry by default, including internal callers with no cap. This PR only makes an explicit cap work for them. - NVIDIA-NeMo#2216 tracks the `ServerDisconnectedError` and `ClientOSError` branches, which this PR does not change. - Open PR NVIDIA-NeMo#2935 adds a timeout branch that copies the old counting, so internal callers there still do not count tries. Whichever PR merges later must count tries for every caller. ## Validation All runs used an activated venv from `uv sync --extra dev --extra sandbox --extra openshell --extra telemetry --extra mlflow --extra wandb` (the extras in `full-test-suite.yml`). Every installed package matches `uv.lock` (192 packages, including aiohttp 3.14.4, nemo-lens 0.2.0+b0f977d, and pytest 9.0.3). - `pytest tests/unit_tests/test_server_utils.py responses_api_agents/remote_agent/tests/test_app.py tests/unit_tests/telemetry/`: 465 passed. - With the old code restored, `test_request_retry_cap_bounds_internal_callers` fails in under a second with `Failed: retry loop did not stop` instead of hanging. It fails the same way when the retry sleep is also removed. - Each change below makes at least one test fail. Each run of `test_server_utils.py` took about 3 s, and none hung: - the cap is ignored; - internal callers are limited by `MAX_NUM_TRIES`; - internal callers print; - external tries are double-counted; - external callers stop printing the retry message; - the `MAX_NUM_TRIES` raise is removed; - the retry sleep after a generic error is skipped; - the cap check ignores `_max_num_tries` attempts; - the generic branch ignores `_max_num_tries`; - the traced path drops `_internal` or `_max_connection_retries`. - `pytest -n 16 tests/unit_tests responses_api_agents/remote_agent/tests`: 8089 passed, 5 skipped. The per-test results match `main` at a3cf403 (8085 passed, 5 skipped). The 4 extra results are the new tests, with the cap test run twice (tracing off and on). - The same focused tests pass on a local merge with the latest `main` (NVIDIA-NeMo#4300), which changes no package versions. - `pre-commit run --all-files`: passed. ## Rollouts N/A. This changes a retry counter in the shared HTTP helper. No caller in this repo changes behavior, and no model, agent, environment, or verifier logic is involved. ## Compatibility and benchmark impact None for callers in this repo. A caller that sets `_internal=True` and `_max_connection_retries` to 2 or more, without `_max_num_tries`, now stops after that many tries on errors that reach the generic `except Exception` branch, including timeouts, instead of retrying forever. This includes code outside this repo that uses `NeMoGymAsyncOpenAI(internal=True, max_connection_retries=N)`. `ServerDisconnectedError` and `ClientOSError` keep their own count, as before. ## Checklist - [x] I have read the [contributing guidelines](https://github.com/NVIDIA-NeMo/Gym/blob/main/CONTRIBUTING.md). - [x] The change is focused; unrelated "drive-by" edits are tracked as separate issues/PRs. - [x] Tests added or updated and pass locally, or N/A for docs-only / non-code changes (so CI unit/server checks pass when applicable). - [x] Documentation added or updated, or N/A with justification. N/A: internal helper behavior with no user-facing docs. - [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. N/A: no new files. - [x] All commits have DCO sign-off (`git commit -s`) (so the DCO check passes). --------- Signed-off-by: Abhishree <athittenaman@nvidia.com>
What does this PR do?
Adds opt-in controls for multi-turn model rollouts. With default settings, existing configurations behave as on main (see Compatibility).
openai_model,litellm_model). Optional per-attempt request and connect timeouts, a bounded wait for a concurrency slot, and a model-server retry policy with configurable backoff (upstream_*options).upstream_max_num_tries=1(andNeMoGymAsyncOpenAI(max_num_tries=1)) makes each provider call a single HTTP attempt, so the policy owns the whole retry schedule. A key that the client recognizes as spent or invalid stops the policy immediately. Exhausted retries raiseUpstreamRetriesExhaustedError, aRuntimeError(HTTP 500 for non-streaming requests) that keeps the final provider status and body for model-call capture.LiteLLMModelServerapplies the same settings on/v1/responses.propagate_upstream_http_status_codesreturns selected provider error statuses with the provider's error body instead of HTTP 500, including for streaming Responses requests.NeMoGymAsyncOpenAIalso reads error codes from that body, so a propagated spent or invalid key stops a receiving Gym client, as a direct provider error does.request()gains a trailing_max_num_triesparameter that caps a call's attempts on every exception path and replaces the default generic-error limit.environment_cleanup_timeout_seconds(default 60 s).web_search_callitems with no action, or an action type the SDK does not know, are accepted and kept opaque. Known actions stay typed SDK models, and payloads round-trip unchanged.reasoning_effort_none_replacementreports a provider'sreasoning.effort: "none"as another value for callers that cannot accept"none". It is off by default.The new
openai_modeloptions are documented in its configuration reference.No tracking issue: this is shared infrastructure for multi-turn evaluation, and each change is self-contained and described above.
Validation
Test suites and differentials are against this branch merged into main
b0e3fa6cf.openai_model45,litellm_model26,gymnasium_agent28 passed.switchyard_model,vllm_model,inference_provider,azure_openai_model,local_vllm_model_proxy, andvllm_model_with_compactionmatch main, as do the otherNeMoGymAsyncOpenAIusers' suites (terminus_2_sandboxed_agent,tau2,browsecomp_agent,verifif,gdpval,aalcr).openai_model10,litellm_model24,gymnasium_agent12, Responses streaming 170,test_openai_utils247,test_responses_converter133,test_server_utils137.test_file_lineage_resolves_across_spawned_worker_processesfailed; that test fails intermittently on both.openai_modelandlitellm_model; Responses, Chat Completions and Messages, streaming and not; capture on and off; 21 provider behaviors. All 1,028 scenarios match main in status, body, provider hits, forwarded requests, and capture records.NeMoGymAsyncOpenAI(internal=True)), 30 scenarios. A propagatedinsufficient_quota429 orinvalid_api_key401 now stops the client after one model-server reply. Previously, the 429 was still being retried after 8 replies. Transient 429s still retry until they succeed. Default configurations, other propagated statuses, and direct provider calls are unchanged.pre-commit run --all-filespasses, and all commits are signed off.Rollouts
Main (
3ef478df1) versus this branch merged into it, same Nemotron 3 Nano vLLM server (gpt-5-nano for theopenai_modelrow). The final head differs from the rolled-out commit in one docstring line and in reading error codes from a body'sdetail, which these default-configuration rollouts never receive.blackjack(Gymnasium agent)vllm_modelexample_multi_turn_gymnasiumvllm_modelexample_single_tool_callvllm_modelexample_single_tool_callopenai_model(gpt-5-nano)Rewards match except blackjack, whose card draws vary between runs.
Compatibility and benchmark impact
Default configurations behave as on main for model-server requests and responses (see the differential above). Other effects:
web_search_callactions no longer gain placeholdernullkeys (for examplequeries,sources). Shapes that were previously rejected are now accepted; on chat-converting backends (vllm_model,inference_provider) they fail the same way validweb_search_callitems already do.max_concurrent_requestsmust be at least 1. Previously, 0 made every request wait forever.supports_explicit_close, error-path cleanup now survives client disconnects and is bounded byenvironment_cleanup_timeout_seconds(default 60 s). On main this affects onlyopenair_congestion.responses()raisesHTTPExceptionduring a streaming Responses request now returns that status instead of aresponse.failedevent, like request validation. Among shipped model servers, onlyopenai_modelandlitellm_modelraise it, and only when propagation is configured.SimpleModelServermust keep aSimpleModelServerConfig-derived config, since the server now reads the new options.NeMoGymAsyncOpenAIreadserror.code/error.typeunderdetailwhen a body has no top-levelerror. A 429 or 401/403 whosedetailcarries a spent-key or invalid-key code now stops the client, as the same top-level code already does.No benchmark results change.
Checklist
pre-commit run --all-files) (so CI lint/format checks pass).git commit -s) (so the DCO check passes).