Repository navigation
feat(telemetry): full correlation chain, saturation, concurrency, and Pareto-efficiency metrics - #2935
Open
oyilmaz-nvidia wants to merge 9 commits into
Open
feat(telemetry): full correlation chain, saturation, concurrency, and Pareto-efficiency metrics#2935oyilmaz-nvidia wants to merge 9 commits into
oyilmaz-nvidia wants to merge 9 commits into
Conversation
Signed-off-by: Onur Yilmaz <oyilmaz@nvidia.com>
Contributor
|
🌿 Preview your docs: https://nvidia-preview-onur-expand-otel.docs.buildwithfern.com/nemo/gym Here are the markdown pages you've updated: |
Signed-off-by: Onur Yilmaz <oyilmaz@nvidia.com>
Signed-off-by: Onur Yilmaz <oyilmaz@nvidia.com>
Signed-off-by: Onur Yilmaz <oyilmaz@nvidia.com>
Signed-off-by: Onur Yilmaz <oyilmaz@nvidia.com>
oyilmaz-nvidia
marked this pull request as ready for review
September 10, 2026 17:54
Contributor
|
/claude review |
Contributor
|
HOLD — one BLOCKER in the global HTTP-timeout change; the rest of the telemetry work is solid. The bulk of this PR (CPU/GPU/memory sampling, attributed OTel instruments in The one blocker is unrelated to telemetry: the new Everything else: SHIP. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| "Total timeout for one aiohttp request, seconds. 300.0 matches aiohttp's own " | ||
| "implicit default, so this only surfaces the knob -- it does not change behavior " | ||
| "until a caller opts into a different value." | ||
| ), |
Contributor
There was a problem hiding this comment.
BLOCKER — this silently changes behavior and can hang long rollouts.
The docstring's claim "300.0 matches aiohttp's own implicit default, so ... it does not change behavior" is incorrect. The prior code passed ClientTimeout() explicitly (server_utils.py:171 on base), which sets total=None — i.e. no total timeout. aiohttp's 300s DEFAULT_TIMEOUT sentinel only applies when no timeout object is passed to the ClientSession at all; passing a bare ClientTimeout() overrides it to unlimited. So this changes the global client from no total timeout → 300s total, and adds a 30s connect timeout where there was none.
What breaks: every call through the shared client now dies at 300s total. That includes:
- Agent
/runrollouts —ServerClient.request(..., _internal=True)(server_utils.py:486), called with no per-call timeout override by the rollout driver (rollout_collection.pyserver_client.post(url_path="/run", ...)). SWE/terminal/OSWorld agent trajectories routinely run 5–30+ min. - Long model-server generations (
/v1/responsesstreaming), wheretotalcovers the full stream.
Why it's worse than a truncation: the new except asyncio.TimeoutError branch (server_utils.py:410) retries _internal callers indefinitely (the if not _internal: raise is skipped). Before this PR, with total=None, a TimeoutError essentially never fired. Now a healthy rollout exceeding 300s times out, retries the whole /run from scratch, times out at 300s again, and loops forever — the rollout never completes. Blast radius: eval runs and RLHF training data collection hang / silently drop every long trajectory.
Fix: default global_aiohttp_total_timeout_seconds to None to preserve the prior unlimited behavior (surfacing the knob without changing the default), or explicitly justify a finite cap that exceeds the longest expected rollout and confirm the _internal timeout-retry path won't loop forever on a legitimately-slow request. Correct the docstring either way.
Contributor
There was a problem hiding this comment.
@oyilmaz-nvidia Could we address this? This would severely change behaviour for gym as currently the timeout is unbounded and we do have some calls p99 going beyond the 300s mark
I think we'd like to keep the functionality the same unless there was a reason to change this here
Contributor
|
Also curious whether you were able to benchmark overall how much overhead this PR adds for collecting metrics? @oyilmaz-nvidia |
kajalj22
reviewed
Sep 11, 2026
| attributes[MEMORY_TOTAL_MIB_ATTRIBUTE] = total_mib | ||
| record_host_memory_used_mib(used_mib) # still inside `with managed_span` | ||
| record_host_memory_total_mib(total_mib) | ||
| safe_set_span_attributes(span, attributes) |
Contributor
There was a problem hiding this comment.
This looks like very similar logic in 4 places (endpoints.py, rollout_collection.py, sandbox/api.py×2). Could we extract this into one shared helper function? Would be easier to maintain the code
Contributor
|
/ok to test 67b3e9d |
Contributor
Author
|
@kajalj22 Thanks for your review. I'll push more code into this PR and try address you questions in in the next couple of days. |
Signed-off-by: Onur Yilmaz <oyilmaz@nvidia.com>
oyilmaz-nvidia
changed the title
Resolves conflicts from 184 commits of main-branch drift against the telemetry work on this branch (PR #2935). All manual resolutions kept both sides' intent rather than picking one over the other: - nemo_gym/base_responses_api_model.py: kept our TTFB-only early-return path (pure telemetry, no capture) alongside main's new SSE-terminal tracking in `_send_training_only` (capture path) -- they cover disjoint cases. - nemo_gym/rollout_collection.py: main refactored the rollout dispatch subroutine into a shared `_run_examples_with_metadata` helper; ported our outcome-tracking (success/timeout/failure) and `record_rollout_completed` call into that canonical function instead of keeping a second, now-redundant copy in `run_examples` itself. - nemo_gym/server_utils.py: combined our retry-count/telemetry recording with main's new `_max_connection_retries` bound check in the generic exception branch (both already coexist in the sibling ServerDisconnectedError/ClientOSError branches). - nemo_gym/mcp_auto_exposure.py, resources_servers/*, responses_api_agents/*: pure import-block collisions between unrelated features added at the same location; combined both. - Test files: independent new test cases from both sides appended at the same location; kept both. Also fixes a real, pre-existing bug this merge exposed (not caused by it): `_request_with_retries`'s `except asyncio.TimeoutError` branch never checked the caller-supplied `_max_connection_retries` bound, unlike every sibling except-branch. This silently ignored `GymAiohttpTransport`'s `_max_connection_retries=1` contract ("the SDK owns retry semantics, never replay a possibly-already-executed command") for timeouts specifically, retrying up to `MAX_NUM_TRIES=3` instead. Caught by main's new `test_tls_is_set_per_request_and_timeouts_are_not_retried`, which didn't conflict (so silently would have kept failing) -- fixed to match the existing pattern in the other except branches. Verified: full unit test suite green (5632 passed) after resolution, deselecting only two pre-existing, unrelated missing-optional-dependency gaps in this dev venv (httpx_aiohttp for e2b, opensandbox package itself). Signed-off-by: Onur Yilmaz <oyilmaz@nvidia.com>
The previous merge commit (1970b6c) was incomplete: a `git stash`/ `git stash pop` mid-merge (a known git footgun -- stashing during an unresolved merge drops the merge state) left a large share of origin/main's actual file content unstaged after the pop, so `git commit` (index-only, not `-a`) silently committed a merge commit missing most of main's changes across ~360 files, even though the working tree had the correct content on disk. This commit adds exactly that missing content -- verified file-by-file that the working tree already matched origin/main (e.g. `diff <(git show origin/main:README.md) README.md` was clean before staging). Full unit test suite re-run after staging: 5632 passed, same as before this fix, confirming the previously-missing main content introduces no regressions. Signed-off-by: Onur Yilmaz <oyilmaz@nvidia.com>
iguehring-nvidia
added a commit
that referenced
this pull request
Sep 22, 2026
…rs with real buckets nemo_gym/telemetry/metrics.py can only reach nemo-lens's five fixed, undimensioned instruments. Anything that needs an attribute (a sandbox provider, a site) has to be created directly on the lens meter, so this adds gym_metrics.py: a per-meter instrument cache and histogram / counter / up-down-counter recorders that are no-ops without telemetry and never raise. The sandbox lifecycle set lives here: gym.sandbox.active moves in from metrics.py unchanged in behaviour; gym.sandbox.startup_duration_ms, gym.sandbox.exec_duration_ms and gym.sandbox.create_retry_total are added for the next commits to record into. The two histograms declare explicit bucket boundaries up to thirty minutes: the SDK default ends at ten seconds, and a remote sandbox start routinely takes a minute, so every sample would otherwise land in the overflow bucket and a p95 would read "10 s" whatever the truth. The registry helpers and the three duration/retry recorders are taken from Onur Yilmaz's #2935, which this series splits out. Co-authored-by: Onur Yilmaz <oyilmaz@nvidia.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Ingo Guehring <iguehring@nvidia.com>
iguehring-nvidia
added a commit
that referenced
this pull request
Sep 22, 2026
…ans, a stop span AsyncSandbox records gym.sandbox.startup_duration_ms around provider.create and gym.sandbox.exec_duration_ms around each command, per provider, with the explicit bucket boundaries from gym_metrics. The start and exec spans carry nemo.gym.sandbox.id so a trace can be joined to the provider's own logs, and stop() now opens a gym.sandbox.stop span around provider.close, so a trace pairs a sandbox's start with its end. All of it sits under the existing sandbox span group; nothing is recorded with the group off. The duration histograms and the id attribute are taken from Onur Yilmaz's #2935. Co-authored-by: Onur Yilmaz <oyilmaz@nvidia.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Ingo Guehring <iguehring@nvidia.com>
iguehring-nvidia
added a commit
that referenced
this pull request
Sep 22, 2026
Retrying a create is provider-internal, so the facade cannot see it; the two providers that retry record gym.sandbox.create_retry_total from their tenacity before_sleep callback, next to the warning they already log. On a full Terminal-Bench 4 run against a degraded OpenSandbox this is the difference between "39 rollouts dropped" and "101 retries, then 39 dropped" being visible on a board rather than only in the driver log. The Daytona hook is from Onur Yilmaz's #2935. Co-authored-by: Onur Yilmaz <oyilmaz@nvidia.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Ingo Guehring <iguehring@nvidia.com>
iguehring-nvidia
added a commit
that referenced
this pull request
Sep 23, 2026
Retrying a create is provider-internal, so the facade cannot see it; the two providers that retry record gym.sandbox.create_retry_total from their tenacity before_sleep callback, next to the warning they already log. On a full Terminal-Bench 4 run against a degraded OpenSandbox this is the difference between "39 rollouts dropped" and "101 retries, then 39 dropped" being visible on a board rather than only in the driver log. The Daytona hook is from Onur Yilmaz's #2935. Co-authored-by: Onur Yilmaz <oyilmaz@nvidia.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Ingo Guehring <iguehring@nvidia.com>
iguehring-nvidia
added a commit
that referenced
this pull request
Sep 23, 2026
The driver decides a rollout's fate in its result-writing loop: scored (persisted to the main output) or dropped (routed to the failures sidecar, or kill-shaped and not persisted). gym.rollout.completed_total is recorded there, once per rollout, with nemo.gym.rollout.outcome and, for dropped rollouts, Gym's failure class and the exception name when the reason is a bare identifier. Recording at the verdict rather than around the /run HTTP call matters: an agent that answers 200 carrying an infrastructure error is a dropped rollout, not a success. Replaces the outcome counter from #2935, which was recorded around the HTTP call with success/timeout/failure, and the earlier gym.rollout.failures proposal, with one instrument at one site. Co-authored-by: Onur Yilmaz <oyilmaz@nvidia.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Ingo Guehring <iguehring@nvidia.com>
iguehring-nvidia
added a commit
that referenced
this pull request
Sep 23, 2026
…rs with real buckets nemo_gym/telemetry/metrics.py can only reach nemo-lens's five fixed, undimensioned instruments. Anything that needs an attribute (a sandbox provider, a site) has to be created directly on the lens meter, so this adds gym_metrics.py: a per-meter instrument cache and histogram / counter / up-down-counter recorders that are no-ops without telemetry and never raise. The sandbox lifecycle set lives here: gym.sandbox.active moves in from metrics.py unchanged in behaviour; gym.sandbox.startup_duration_ms, gym.sandbox.exec_duration_ms and gym.sandbox.create_retry_total are added for the next commits to record into. The two histograms declare explicit bucket boundaries up to thirty minutes: the SDK default ends at ten seconds, and a remote sandbox start routinely takes a minute, so every sample would otherwise land in the overflow bucket and a p95 would read "10 s" whatever the truth. The registry helpers and the three duration/retry recorders are taken from Onur Yilmaz's #2935, which this series splits out. Co-authored-by: Onur Yilmaz <oyilmaz@nvidia.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Ingo Guehring <iguehring@nvidia.com>
iguehring-nvidia
added a commit
that referenced
this pull request
Sep 23, 2026
…ans, a stop span AsyncSandbox records gym.sandbox.startup_duration_ms around provider.create and gym.sandbox.exec_duration_ms around each command, per provider, with the explicit bucket boundaries from gym_metrics. The start and exec spans carry nemo.gym.sandbox.id so a trace can be joined to the provider's own logs, and stop() now opens a gym.sandbox.stop span around provider.close, so a trace pairs a sandbox's start with its end. All of it sits under the existing sandbox span group; nothing is recorded with the group off. The duration histograms and the id attribute are taken from Onur Yilmaz's #2935. Co-authored-by: Onur Yilmaz <oyilmaz@nvidia.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Ingo Guehring <iguehring@nvidia.com>
iguehring-nvidia
added a commit
that referenced
this pull request
Sep 23, 2026
Retrying a create is provider-internal, so the facade cannot see it; the two providers that retry record gym.sandbox.create_retry_total from their tenacity before_sleep callback, next to the warning they already log. On a full Terminal-Bench 4 run against a degraded OpenSandbox this is the difference between "39 rollouts dropped" and "101 retries, then 39 dropped" being visible on a board rather than only in the driver log. The Daytona hook is from Onur Yilmaz's #2935. Co-authored-by: Onur Yilmaz <oyilmaz@nvidia.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Ingo Guehring <iguehring@nvidia.com>
iguehring-nvidia
added a commit
that referenced
this pull request
Sep 23, 2026
The driver decides a rollout's fate in its result-writing loop: scored (persisted to the main output) or dropped (routed to the failures sidecar, or kill-shaped and not persisted). gym.rollout.completed_total is recorded there, once per rollout, with nemo.gym.rollout.outcome and, for dropped rollouts, Gym's failure class and the exception name when the reason is a bare identifier. Recording at the verdict rather than around the /run HTTP call matters: an agent that answers 200 carrying an infrastructure error is a dropped rollout, not a success. Replaces the outcome counter from #2935, which was recorded around the HTTP call with success/timeout/failure, and the earlier gym.rollout.failures proposal, with one instrument at one site. Co-authored-by: Onur Yilmaz <oyilmaz@nvidia.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Ingo Guehring <iguehring@nvidia.com>
iguehring-nvidia
added a commit
that referenced
this pull request
Sep 23, 2026
…rs with real buckets nemo_gym/telemetry/metrics.py can only reach nemo-lens's five fixed, undimensioned instruments. Anything that needs an attribute (a sandbox provider, a site) has to be created directly on the lens meter, so this adds gym_metrics.py: a per-meter instrument cache and histogram / counter / up-down-counter recorders that are no-ops without telemetry and never raise. The sandbox lifecycle set lives here: gym.sandbox.active moves in from metrics.py unchanged in behaviour; gym.sandbox.startup_duration_ms, gym.sandbox.exec_duration_ms and gym.sandbox.create_retry_total are added for the next commits to record into. The two histograms declare explicit bucket boundaries up to thirty minutes: the SDK default ends at ten seconds, and a remote sandbox start routinely takes a minute, so every sample would otherwise land in the overflow bucket and a p95 would read "10 s" whatever the truth. The registry helpers and the three duration/retry recorders are taken from Onur Yilmaz's #2935, which this series splits out. Co-authored-by: Onur Yilmaz <oyilmaz@nvidia.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Ingo Guehring <iguehring@nvidia.com>
iguehring-nvidia
added a commit
that referenced
this pull request
Sep 23, 2026
…ans, a stop span AsyncSandbox records gym.sandbox.startup_duration_ms around provider.create and gym.sandbox.exec_duration_ms around each command, per provider, with the explicit bucket boundaries from gym_metrics. The start and exec spans carry nemo.gym.sandbox.id so a trace can be joined to the provider's own logs, and stop() now opens a gym.sandbox.stop span around provider.close, so a trace pairs a sandbox's start with its end. All of it sits under the existing sandbox span group; nothing is recorded with the group off. The duration histograms and the id attribute are taken from Onur Yilmaz's #2935. Co-authored-by: Onur Yilmaz <oyilmaz@nvidia.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Ingo Guehring <iguehring@nvidia.com>
iguehring-nvidia
added a commit
that referenced
this pull request
Sep 23, 2026
Retrying a create is provider-internal, so the facade cannot see it; the two providers that retry record gym.sandbox.create_retry_total from their tenacity before_sleep callback, next to the warning they already log. On a full Terminal-Bench 4 run against a degraded OpenSandbox this is the difference between "39 rollouts dropped" and "101 retries, then 39 dropped" being visible on a board rather than only in the driver log. The Daytona hook is from Onur Yilmaz's #2935. Co-authored-by: Onur Yilmaz <oyilmaz@nvidia.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Ingo Guehring <iguehring@nvidia.com>
iguehring-nvidia
added a commit
that referenced
this pull request
Sep 23, 2026
The driver decides a rollout's fate in its result-writing loop: scored (persisted to the main output) or dropped (routed to the failures sidecar, or kill-shaped and not persisted). gym.rollout.completed_total is recorded there, once per rollout, with nemo.gym.rollout.outcome and, for dropped rollouts, Gym's failure class and the exception name when the reason is a bare identifier. Recording at the verdict rather than around the /run HTTP call matters: an agent that answers 200 carrying an infrastructure error is a dropped rollout, not a success. Replaces the outcome counter from #2935, which was recorded around the HTTP call with success/timeout/failure, and the earlier gym.rollout.failures proposal, with one instrument at one site. Co-authored-by: Onur Yilmaz <oyilmaz@nvidia.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Ingo Guehring <iguehring@nvidia.com>
iguehring-nvidia
added a commit
that referenced
this pull request
Sep 23, 2026
The driver decides a rollout's fate in its result-writing loop: scored (persisted to the main output) or dropped (routed to the failures sidecar, or kill-shaped and not persisted). gym.rollout.completed_total is recorded there, once per rollout, with nemo.gym.rollout.outcome and, for dropped rollouts, Gym's failure class and the exception name when the reason is a bare identifier. Recording at the verdict rather than around the /run HTTP call matters: an agent that answers 200 carrying an infrastructure error is a dropped rollout, not a success. Replaces the outcome counter from #2935, which was recorded around the HTTP call with success/timeout/failure, and the earlier gym.rollout.failures proposal, with one instrument at one site. Co-authored-by: Onur Yilmaz <oyilmaz@nvidia.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Ingo Guehring <iguehring@nvidia.com>
JeffPengCoder
pushed a commit
to JeffPengCoder/Gym
that referenced
this pull request
Sep 23, 2026
…nd a stop span (NVIDIA-NeMo#3633) Supersedes NVIDIA-NeMo#3581 (its two commits are the first two here) and takes the sandbox part of NVIDIA-NeMo#2935, with @oyilmaz-nvidia as co-author on the hunks from there. Agreed with Onur as the split of NVIDIA-NeMo#2935 that this side owns. ## What One coherent "sandbox lifecycle telemetry" set in `AsyncSandbox` and the providers, all under the existing `sandbox` span group, all attributed by `nemo.gym.sandbox.provider`: | Instrument | Kind | Where | Origin | |---|---|---|---| | `gym.sandbox.active` | up-down counter: +1 when a provider hands back a handle, −1 when Gym releases it | facade `start()` / `stop()` | this PR (was NVIDIA-NeMo#3581) | | `gym.sandbox.startup_duration_ms` | histogram | facade `start()` | NVIDIA-NeMo#2935 | | `gym.sandbox.exec_duration_ms` | histogram | facade `exec()` | NVIDIA-NeMo#2935 | | `gym.sandbox.create_retry_total` | counter, one per retried create attempt | OpenSandbox and Daytona retry callbacks | NVIDIA-NeMo#2935 (Daytona), new (OpenSandbox) | | `nemo.gym.sandbox.id` | span attribute on start / exec / stop | facade | NVIDIA-NeMo#2935 | | `gym.sandbox.stop` | span around `provider.close` | facade `stop()` | new | Supporting change: `nemo_gym/telemetry/gym_metrics.py`, a small registry for attributed instruments (per-meter cache, histogram / counter / up-down-counter recorders, no-op without telemetry, never raises), taken from NVIDIA-NeMo#2935. `record_sandbox_active` lives there; `telemetry/metrics.py` keeps forwarding to nemo-lens's five fixed instruments and points here for anything attributed. ## Why Gym's sandbox API emitted `gym.sandbox.start` and `gym.sandbox.exec` spans and nothing else: no stop span, no metric. Three questions a dashboard could not answer: how many sandboxes exist right now (dashboards approximated it from in-flight `/run` requests, which undercounts by more than two times on Terminal-Bench 4, where a session holds an agent sandbox, a shared-logs helper and later a verifier sandbox); how long provisioning and commands take as a metric rather than only as spans, which need a span-metrics connector somewhere to become one; and how often creation has to be retried. On a degraded provider the last one is the difference between "39 rollouts dropped" and "101 retries, then 39 dropped". Design points: - **Up-down counter, not a gauge**, for `gym.sandbox.active`: several server processes each hold a share of the sandboxes; a backend sums counters correctly across processes, while a gauge shows whichever process wrote last. −1 happens once the provider has actually released the sandbox: a `close()` the provider rejected has released nothing, so the sandbox stays started and counted and a later `stop()` retries it (the compose group depends on that retry). A `connect()`ed sandbox is neither counted nor subtracted, since the starting process counted it. - **Explicit bucket boundaries up to thirty minutes** on both duration histograms. The SDK default ends at 10 s; a remote sandbox start takes about a minute, so every sample would otherwise land in the overflow bucket and a p95 would read "10 s" whatever the truth. (NVIDIA-NeMo#2935 used the defaults; fixed while consolidating.) - **Retry counting sits in the providers** because retrying is provider-internal: the facade calls `create()` once. The two providers that retry (OpenSandbox, Daytona) record from their tenacity `before_sleep` callback, next to the warning they already log, so the counter can never disagree with the log. - **The stop span** wraps only `provider.close`; a failing close yields an error-status stop span and leaves the count unchanged until the retry succeeds. Deliberately not in this PR (they belong to other splits of NVIDIA-NeMo#2935): the CPU / memory / GPU sampling at span close, and the `TimedSemaphore` swap in the providers. ## Validation - `pytest tests/unit_tests/telemetry tests/unit_tests/test_sandbox.py tests/unit_tests/test_opensandbox_provider.py`: pass locally (nemo-lens-gated span tests skip locally and run on the cluster venv). Tests, all against a real in-memory OTel reader: - `test_sandbox_active.py` (10): start then stop moves 1 → 0 and a second stop stays 0; failed create counts nothing; failed initial upload releases the count; a failed close keeps the sandbox counted until a retried stop releases it; group off records nothing; `connect()`ed sandbox not subtracted; no-op without telemetry; errors never reach the caller; non-monotonic and dimensioned by provider; one instrument per meter. - `test_sandbox_lifecycle_metrics.py` (4): one startup sample per start and one exec sample per command with the provider attribute; explicit bounds end at 1,800,000 ms; failed create records no startup sample; group off records nothing. - `test_sandbox_create_retry.py` (3): both providers' callbacks count by provider when the group is on, silent when off. - `test_call_sites.py` (+1 test, +3 assertions): `nemo.gym.sandbox.id` on start and exec spans; a `gym.sandbox.stop` span with provider and id. - `pre-commit run --files ...` on the changed files: pass. ## Rollout evidence Slurm cluster, Terminal-Bench 4 (mini-SWE), OpenSandbox provider, `sandbox` span group on, metrics exported through an OTLP collector that also derives span metrics, so every Gym-emitted number has an independent counterpart to check against. **8-task run (concurrency 8, 24 min, all 8 rollouts scored, collector shut down cleanly):** | Instrument | Gym-emitted | Independent source | Result | |---|---|---|---| | `gym.sandbox.create_retry_total{provider="opensandbox"}` | 10 | 10 "Retrying OpenSandbox sandbox create" lines in the driver log | exact match | | `gym.sandbox.startup_duration_ms` | 25 samples, mean 91.6 s, p50 ≤ 60 s, p95 ≤ 300 s | span-derived `gym.sandbox.start` duration: 25 samples, mean 91.6 s, same p50 / p95 | identical | | `gym.sandbox.exec_duration_ms` | 580 samples, mean 2.4 s, p95 ≤ 5 s | span-derived `gym.sandbox.exec`: 581 successful + 1 error span, p95 ≤ 5 s | agree (see note) | | `gym.sandbox.stop` span | 25 spans, all OK | 25 starts | every sandbox released | | `gym.sandbox.active` | 0 → peak 14 → 0 | 8 sessions, each holding an agent sandbox, a shared-logs helper and later a verifier sandbox | consistent, no leak | With the SDK's default buckets, the startup histogram (mean 92 s) would have placed every sample in the overflow bucket and reported "10 s" for any percentile; with the explicit boundaries the p50 / p95 land in the 60 s / 300 s buckets, matching the span-derived series bucket for bucket. Note on the exec count: the duration histograms are recorded after the span closes, so an operation that raises never reaches the record and is absent from the histogram; it does appear in the span-derived series with `STATUS_CODE_ERROR`. The one error exec accounts for the gap. This is deliberate: a failure's duration is not a latency, and the failure itself is counted where the status lives. **66-task run at concurrency 16 (earlier, counter only):** `gym.sandbox.active` rose from 8 to a peak of 39 while the in-flight-rollouts proxy reported 16, and returned to 0 through 6 `SANDBOX_NOT_FOUND` rebuilds and 3 failed starts. ## Compatibility Additive. Nothing is recorded unless telemetry is on and the `sandbox` span group is enabled. New module `telemetry/gym_metrics.py`; no public API changes. `AsyncSandbox.stop()` semantics are unchanged from main: a failed `close()` leaves the sandbox started so the call can be retried (an earlier revision of this PR broke that and failed `test_docker_compose`; fixed in the last commit). --------- Signed-off-by: Ingo Guehring <iguehring@nvidia.com> Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> Co-authored-by: Onur Yilmaz <oyilmaz@nvidia.com>
raveedturing
pushed a commit
to turing-rlgym/Gym
that referenced
this pull request
Sep 23, 2026
…#3584) ## What changed Add centralized, opt-in memory profiling to NeMo Gym's existing Lens/OpenTelemetry integration. One periodic orchestrator sampler reports: - host used, available, total, and sampled-peak memory; - current RSS/PSS and sampled peaks for every logical Gym server and its local descendants; - the number of processes currently attributed to each server. Users can enable the profiler through YAML or environment variables. Environment-only enablement supports `gym eval submit` and other orchestrated runs without editing benchmark configs: ```bash NEMO_GYM_OTEL_ENABLED=1 \ NEMO_GYM_OTEL_MEMORY_PROFILING_ENABLED=1 \ NEMO_GYM_OTEL_MEMORY_PROFILING_INTERVAL_SECONDS=1 ``` Disabled runs create no sampler thread and perform no memory reads. Detached/reparented processes and remote Ray actors are explicitly outside per-server accounting; host metrics still expose their contribution to node pressure. Related to NVIDIA-NeMo#3155. This implements the centralized memory portion; runtime GC, event-loop, and request-cycle telemetry remain out of scope. ## Relationship to NVIDIA-NeMo#2935 PR NVIDIA-NeMo#2935 also introduces host and process-tree memory reads, sampled inline when spans close. This focused PR centralizes that memory contract behind one periodic sampler so idle periods remain visible and request completion does not trigger repeated process-tree walks. It adds PSS, logical-server identity, process count, available memory, and sampled peaks. The broader correlation/concurrency work in NVIDIA-NeMo#2935 can consume these latest readings for span attributes when rebased, while dropping its overlapping memory module. ## Local runtime evidence - Uvicorn worker scaling for the policy proxy: - 1 worker / 1 process: 171 MB PSS - 2 workers / 4 processes: 427 MB PSS - 4 workers / 6 processes: 680 MB PSS - 8 workers / 10 processes: 1,191 MB PSS - Stateless resources server, 10,000 requests at client concurrency 512 with 64 KiB payloads: 0 errors; PSS rose from 308 MB idle to a 546 MB sampled peak. - Stateless resources server, 20,000 requests at client concurrency 1,024 with 64 KiB payloads: 0 errors; 656 requests/s; 833 MB sampled PSS peak. - Controlled subprocess allocation: PSS rose from 26 MB to 197 MB and returned to 26 MB; `gym.server.memory.sampled_peak` retained 206 MB. - Environment-only smoke emitted host and per-server metrics without a telemetry YAML block. - Combined host plus 64-process sampling at a one-second interval: 9.24 ms mean, 8.15 ms median, 14.15 ms p95. ## Validation - `uv run pytest tests/unit_tests/telemetry/ tests/unit_tests/test_cli.py tests/unit_tests/test_server_utils.py -q` — 354 passed. - `uv run pytest tests/unit_tests/telemetry/test_memory.py --cov=nemo_gym.telemetry.memory --cov-report=term-missing --cov-fail-under=0 -q` — 15 passed, 98.20% profiler coverage. - `uv run pre-commit run --all-files` — passed. - `cd fern && npm run check` — 0 errors, 1 existing warning. - `uv run gym dev test` on the prior upstream base — 5,667 passed and 2 unrelated CLI output-wrapping assertions failed locally; both reproduced in isolation because Rich wraps long temporary paths despite the tests setting `COLUMNS=1000`. ## Rollout evidence N/A for model rollout behavior: this change affects application telemetry only and does not alter agents, verifiers, datasets, rewards, or model requests. The real multi-worker and high-concurrency HTTP smoke tests above exercise the changed runtime path directly. ## Compatibility Telemetry and memory profiling remain disabled by default. Server PSS requires Linux `smaps_rollup`; when unavailable, Gym emits RSS from `statm` and omits PSS. Host usage is calculated as total minus available. Sampled peaks can miss allocations shorter than the configured interval and are documented as sampled rather than exact cgroup lifetime peaks. Signed-off-by: Ananth Subramaniam <ansubramania@nvidia.com>
2 tasks done
7 tasks done
iguehring-nvidia
added a commit
that referenced
this pull request
Oct 5, 2026
…ind and type One counter, `gym.rollout.completed_total`, incremented in the driver after each record is written where it belongs: `scored` (main output, masked results included), `dropped` (failures sidecar) or `omitted` (kill-shaped or drained, written nowhere). It counts attempts: a failed attempt followed by a successful retry contributes one of each; coverage comes from the persisted records. Dropped and omitted attempts carry three bounded attributes: the driver's `_ng_failure_class`, the registered `failure_kind` from nemo_gym.failure_kinds when the producer set one, and `_ng_failure_type`, the exception class name. Free-text failure reasons are never attributes. Supersedes the recording around the `/run` HTTP call in #2935: a 200 whose body carries an infrastructure error is a dropped attempt, which only the driver's verdict knows. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Ingo Guehring <iguehring@nvidia.com>
iguehring-nvidia
added a commit
that referenced
this pull request
Oct 5, 2026
…ind and type One counter, `gym.rollout.completed_total`, incremented in the driver after each record is written where it belongs: `scored` (main output, masked results included), `dropped` (failures sidecar) or `omitted` (kill-shaped or drained, written nowhere). It counts attempts: a failed attempt followed by a successful retry contributes one of each; coverage comes from the persisted records. Dropped and omitted attempts carry three bounded attributes: the driver's `_ng_failure_class`, the registered `failure_kind` from nemo_gym.failure_kinds when the producer set one, and `_ng_failure_type`, the exception class name. Free-text failure reasons are never attributes. Supersedes the recording around the `/run` HTTP call in #2935: a 200 whose body carries an infrastructure error is a dropped attempt, which only the driver's verdict knows. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Ingo Guehring <iguehring@nvidia.com>
iguehring-nvidia
added a commit
that referenced
this pull request
Oct 5, 2026
…ind and type One counter, `gym.rollout.completed_total`, incremented in the driver right after the RolloutStore recorded the attempt: `scored` after a success is persisted, `dropped` after a failure is persisted to the sidecar, `omitted` after an intentional omission. It counts attempts: a failed attempt followed by a successful retry contributes one of each; coverage comes from the store. Dropped and omitted attempts carry three bounded attributes: the driver's `_ng_failure_class`, the registered `failure_kind` from nemo_gym.failure_kinds when the producer set one, and `_ng_failure_type`, the exception class name. Free-text failure reasons are never attributes. Supersedes the recording around the `/run` HTTP call in #2935: a 200 whose body carries an infrastructure error is a dropped attempt, which only the driver's verdict knows. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Ingo Guehring <iguehring@nvidia.com>
iguehring-nvidia
added a commit
that referenced
this pull request
Oct 5, 2026
…ind and type One counter, `gym.rollout.completed_total`, incremented in the driver right after the RolloutStore recorded the attempt: `scored` after a success is persisted, `dropped` after a failure is persisted to the sidecar, `omitted` after an intentional omission. It counts attempts: a failed attempt followed by a successful retry contributes one of each; coverage comes from the store. Dropped and omitted attempts carry three bounded attributes: the driver's `_ng_failure_class`, the registered `failure_kind` from nemo_gym.failure_kinds when the producer set one, and `_ng_failure_type`, the exception class name. Free-text failure reasons are never attributes. Supersedes the recording around the `/run` HTTP call in #2935: a 200 whose body carries an infrastructure error is a dropped attempt, which only the driver's verdict knows. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Ingo Guehring <iguehring@nvidia.com>
iguehring-nvidia
added a commit
that referenced
this pull request
Oct 6, 2026
…ind and type One counter, `gym.rollout.completed_total`, incremented in the driver right after it wrote the attempt: `scored` after a success reaches the main output, `dropped` after a failure reaches the failures sidecar, `omitted` for a no-persist result such as a kill-shaped failure. Rows drained by the dispatch budget never ran and are not counted. It counts attempts: a failed attempt followed by a successful retry contributes one of each; coverage comes from the output files. Dropped and omitted attempts carry three bounded attributes: the driver's `_ng_failure_class`, the registered `failure_kind` from nemo_gym.failure_kinds when the record carries one, and `_ng_failure_type`, the exception class name. Free-text failure reasons are never attributes. Supersedes the recording around the `/run` HTTP call in #2935: a 200 whose body carries an infrastructure error is a dropped attempt, which only the driver's verdict knows. Signed-off-by: Ingo Guehring <iguehring@nvidia.com>
iguehring-nvidia
added a commit
that referenced
this pull request
Oct 10, 2026
…ind and type One counter, `gym.rollout.completed_total`, incremented in the collection driver right after it wrote the attempt or decided to omit it: `scored` after a success reaches the main output, `failed` after a failure reaches the failures sidecar, `omitted` for a no-persist result such as a kill-shaped failure. Rows drained by the dispatch budget never ran and are not counted. It counts attempts: a failed attempt followed by a successful retry on a later resume run contributes one of each; coverage comes from the output files. Every point carries the dispatch name. Attempts with a failure class also carry three bounded attributes: the driver's `_ng_failure_class`, the `failure_kind` when registered or namespaced (any other value is recorded as `unregistered`), and `_ng_failure_type`, the exception class name. Failure fields on a scored row are ignored. Free-text failure reasons are never attributes. Terminal Bench 4 now sets `_ng_failure_type` from the trial's exception type when the official reward is missing, so its infrastructure failures keep the type label. The free-text termination detail stays out of it. Supersedes the recording around the `/run` HTTP call in #2935: a 200 whose body carries an infrastructure error is a failed attempt, which only the driver's verdict knows. Signed-off-by: Ingo Guehring <iguehring@nvidia.com>
iguehring-nvidia
added a commit
that referenced
this pull request
Oct 10, 2026
…ind and type One counter, `gym.rollout.completed_total`, incremented in the collection driver right after it wrote the attempt or decided to omit it: `scored` after a success reaches the main output, `failed` after a failure reaches the failures sidecar, `omitted` for a no-persist result such as a kill-shaped failure. Rows drained by the dispatch budget never ran and are not counted. It counts attempts: a failed attempt followed by a successful retry on a later resume run contributes one of each; coverage comes from the output files. Every point carries the dispatch name. Attempts with a failure class also carry three bounded attributes: the driver's `_ng_failure_class`, the `failure_kind` when registered or namespaced (any other value is recorded as `unregistered`), and `_ng_failure_type`, the exception class name. Failure fields on a scored row are ignored. Free-text failure reasons are never attributes. Terminal Bench 4 now sets `_ng_failure_type` from the trial's exception type when the official reward is missing, so its infrastructure failures keep the type label. The free-text termination detail stays out of it. Supersedes the recording around the `/run` HTTP call in #2935: a 200 whose body carries an infrastructure error is a failed attempt, which only the driver's verdict knows. Signed-off-by: Ingo Guehring <iguehring@nvidia.com>
iguehring-nvidia
added a commit
that referenced
this pull request
Oct 10, 2026
…ind and type One counter, `gym.rollout.completed_total`, incremented in the collection driver right after it wrote the attempt or decided to omit it: `scored` after a success reaches the main output, `failed` after a failure reaches the failures sidecar, `omitted` for a no-persist result such as a kill-shaped failure. Rows drained by the dispatch budget never ran and are not counted. It counts attempts: a failed attempt followed by a successful retry on a later resume run contributes one of each; coverage comes from the output files. Every point carries the dispatch name. Attempts with a failure class also carry three bounded attributes: the driver's `_ng_failure_class`, the `failure_kind` when registered or namespaced (any other value is recorded as `unregistered`), and `_ng_failure_type`, the exception class name. Failure fields on a scored row are ignored. Free-text failure reasons are never attributes. Terminal Bench 4 now sets `_ng_failure_type` from the trial's exception type when the official reward is missing, so its infrastructure failures keep the type label. The free-text termination detail stays out of it. Supersedes the recording around the `/run` HTTP call in #2935: a 200 whose body carries an infrastructure error is a failed attempt, which only the driver's verdict knows. Signed-off-by: Ingo Guehring <iguehring@nvidia.com>
Ashish-Soni08
pushed a commit
to Ashish-Soni08/Gym
that referenced
this pull request
Oct 10, 2026
…NVIDIA-NeMo#3638) Second split of NVIDIA-NeMo#2935 agreed with @oyilmaz-nvidia: the rollout outcome counter, consolidated with the failure-class counter from the orchestrator branch. Reworked after review by @yuhezhang-ai and @ananthsub. ## What One counter, `gym.rollout.completed_total`, incremented in the collection driver's result-writing loop right after it writes the attempt or decides to omit it: | Outcome (`nemo.gym.rollout.outcome`) | Recorded after | |---|---| | `scored` | the result was written to the main output. Includes masked and forced-zero results, which are written but not necessarily scored. | | `failed` | the failure was written to the failures sidecar | | `omitted` | a no-persist result (e.g. kill-shaped) was written to neither file | Rows drained by the dispatch budget never ran and are not counted. This differs from the progress metrics, whose `dropped` groups `failed` with an `omitted` that includes drained rows. Attributes, all bounded: | Attribute | Set on | Source | |---|---|---| | `nemo.gym.rollout.dispatch.name` | every point | the environment server, or the agent when rows name none | | `nemo.gym.failure_class` | attempts with a failure class | the driver's `_ng_failure_class` (`infrastructure_error`, `agent_request_failed`, `agent_run_error`, `judge_failed`, ...) | | `nemo.gym.failure_kind` | attempts with a failure class | the record's `failure_kind` if registered in `nemo_gym.failure_kinds` or `<server>:<kind>` namespaced; any other value is recorded as `unregistered`. The shared validator is unchanged. | | `nemo.gym.failure_type` | attempts with a failure class | `_ng_failure_type`, the exception class name | Failure fields on a scored row are ignored, so a value echoed back through a verifier can't label it. Free-text failure reasons never become attributes. Terminal Bench 4 now sets `_ng_failure_type` from the trial's `exception_info.exception_type` (or `MissingOfficialReward`) when the official reward is missing. The infrastructure-error branch carries free text from the termination detail and stays without a type. **Attempt semantics and scope.** An attempt is one dispatch of one row in one collection run: a retry on a later resume run is another attempt, and an HTTP-level re-send of `/run` inside one dispatch is not. Coverage comes from the output files, not this counter. Only `run_from_config` (`gym eval run` with serving) records it; `--no-serve`, `run_examples` callers, reverification and the GDPVal multistage driver do not. A failed `/run` is a `failed` attempt with class `agent_request_failed` only with `route_failures_to_sidecar` on; otherwise it aborts the run. Gated by the `rollout` span group, which is in Gym's default set. No-op without telemetry. Documented in `observability/metrics.mdx`. ## Why Dashboards could not show "how many rollouts did this run lose, and to what": the agent answers `/run` with 200 whether or not the rollout scored, the `gym.rollout` span carries no status, and the driver's classification went only to the `rollouts_failures.jsonl` sidecar and a log line. On a degraded sandbox provider a full Terminal-Bench 4 run lost 60 of 66 rollouts to `SandboxApiException`; that number should be a panel, not a grep. **Why at the verdict and not around the HTTP call.** NVIDIA-NeMo#2935 recorded `gym.rollout.completed_total{outcome=success|timeout|failure}` in a `finally` around the `/run` request. That measures the transport: a `/run` that returns 200 carrying an infrastructure-error row counts as a success there. In the run above, all 60 sandbox failures came back as HTTP 200. Recording where the driver writes its verdict sees the real outcome. Not done here, and worth a follow-up in Gym's telemetry code: marking the `gym.rollout` span itself with an error status for failed rollouts, so they are findable in the trace backend too. The span lives in the agent server; the verdict is taken in the driver, so it needs a small protocol addition. ## Validation - `tests/unit_tests/telemetry/test_rollout_outcomes.py`, against a real in-memory OTel reader: outcomes and bounded attributes; an unregistered kind is bucketed while registered and namespaced kinds pass; failure fields on a scored row are ignored; the helper is gated on the `rollout` span group specifically. - `tests/unit_tests/test_rollout_collection.py`: through the real dispatch loop, one omitted, one failed (HTTP 200 carrying the environment server's class and exception type) and one scored attempt, read back from the meter; the sidecar test asserts the transport failure's class and type; the dispatch-budget test asserts drained rows are not counted. - `resources_servers/terminal_bench_4/tests/test_verified_response.py`: the type is set from the exception type and for a missing reward, and never from the free-text detail. - Mutation checks: removing the `omitted` call, recording it as `failed`, gating on another span group, dropping the `unregistered` bucket, or reading the type on scored rows each fails a test. - Rollout collection, the telemetry test file and Terminal Bench 4 tests: 552 passed. `pre-commit` on the changed files: pass. ## Rollout evidence **This revision** (job-side Gym at this PR's code), Slurm cluster, Terminal-Bench 4 (mini-SWE) on OpenSandbox, Nano 3.5 on one node, metrics exported through the per-job OTLP collector. The counter's final values are compared with the driver's own output files, which are the ground truth, and with the values in the metrics backend. | Run | Series | Counter (job dir) | Backend | Files | |---|---|---|---|---| | 8 tasks | `scored` | 7 | 7 | 7 lines in `rollouts.jsonl` (all reward 0: a wrong answer is still `scored`) | | 8 tasks | `failed`, `infrastructure_error` / `VerifierTimeoutError` | 1 | 1 | 1 line in `rollouts_failures.jsonl`, same class and type | | 2 tasks, invalid sandbox key | `failed`, `infrastructure_error` / `SandboxApiException` | 2 | 2 | 2 lines in `rollouts_failures.jsonl`, same class and type | Every point carries `nemo.gym.rollout.dispatch.name="terminal_bench_4_miniswe"`. No `failure_kind` appears because Terminal Bench 4 sets none. Both types come from Terminal Bench 4's own trial result, which this revision now carries as `_ng_failure_type`; before it, these points had no type. An earlier revision of this PR (labels `outcome` / `failure_class` / `failure_reason`, same recording point) matched the files exactly on a 66-task run (60 scored, 6 dropped across `SandboxTimeoutException`, `VerifierTimeoutError` and `RewardFileNotFoundError`) and an 8-task run (8 scored). ## Compatibility Additive; one new instrument, no config change. Supersedes the `gym.rollout.completed_total` in NVIDIA-NeMo#2935 (different recording site and attribute set; drops out on its rebase) and the never-opened `gym.rollout.failures` proposal. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Ingo Guehring <iguehring@nvidia.com>
khazic
pushed a commit
to khazic/Gym-lao
that referenced
this pull request
Oct 13, 2026
## 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>
svcnvidia-nemo-ci
added
area:core
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes the remaining gaps between Gym's OTel instrumentation and the telemetry-dashboard
MVP: end-to-end correlation, saturation metrics that reflect actual job workload (not
just the Gym PID), live concurrency state, tool-call/agent-turn metrics, and a
resource-efficiency (Pareto) analysis joining experiment telemetry (reward/tokens) with
system telemetry (CPU/GPU-seconds) by rollout id.
End-to-end correlation
run_id,benchmark, andrepeat_indexnow live onTrajectoryRecord(JSONL output),not just as OTel resource/span attributes — closing the join-key gap between traces and
trajectory data.
nemo.gym.run.id,nemo.gym.task.id,nemo.gym.repeat.index(decoded from the rollout id),
nemo.gym.model_request.id,nemo.gym.sandbox.id.(avoids cardinality blowup on high-frequency instruments).
Saturation: process-tree aware, not just the Gym PID
gym.process_tree.cpu.percent/gym.process_tree.memory_used_mib— this process plusevery recursive child (Enroot, OpenClaw, unsquashfs, sandbox-runtime processes), summed.
Sits alongside the existing PID-only metrics rather than replacing them.
no new
nvidia-smicalls added to the hot path) so it can be joined per-rollout.Live concurrency state
TimedSemaphorenow exportsgym.concurrency.limit/.active/.available/.queue_depthas live gauges (previously only after-the-fact wait duration existed),plus
.admission_cancelled_totalfor waits cancelled before admission.Throughput / outcome
gym.rollout.completed_total'soutcomeattribute is now three-way(
success/timeout/failure, was binary). Addedtelemetry.metrics_export_interval_sto make the metrics-export cadence a documented, configurable parameter.
Tool-call metrics (new span group)
Added
GymSpanGroup.TOOL_CALLandToolCallTelemetryMiddlewareon resources servers,giving
gym.tool.call_duration_ms/.call_cpu_percent/.call_memory_used_mibdimensionedby actual tool name — previously only visible as an undimensioned generic HTTP span.
Correctly excluded from
mcp_auto_exposure.py's middleware guard, with the coveragetradeoff documented (a tool invoked via direct MCP dispatch bypasses HTTP, and this
middleware with it).
Agent-turn metrics
gym.agent.turn_count/.turn_duration_ms, derived post-hoc from the assembledtrajectory at rollout completion (no agent harness has a live per-turn span today).
Resource-efficiency / Pareto analysis
New
nemo_gym/pareto_analysis.py: joins rollout JSONL (reward, tokens) with Tempo tracedata (CPU-seconds, GPU-seconds) by
rollout_id, flags Pareto-efficient configurations,and computes tool-calls-per-successful-task. CLI:
python -m nemo_gym.pareto_analysis --input <rollouts.jsonl> --tempo-url <url>.Dashboards + offline tooling
docker/observability/: full local stack (OTel Collector, Prometheus, Tempo, Grafana)plus a 24-panel "NeMo Gym — All Metrics" dashboard covering every metric above.
docker/observability/offline/{record,replay}.sh: record telemetry to disk with noGrafana/Prometheus/Tempo running, replay it into the live stack later.
Known limitations (documented in code, not silently glossed over)
polling every ~10s, not a fresh read per span, and shared across co-located processes.
reject-on-full/timeout policy today, so there's nothing to distinguish from inside
TimedSemaphore.acquire().nemo_gym.sandbox.api.AsyncSandbox;mini_swe_agent_2's coding sandbox (driven by the third-partyminisweagentlibrary)bypasses it.
psutil.Process.children(recursive=True))twiceper sample tick (once each in
cpu.py/memory.py) — measured ~7-8ms per call on aquiet host; a future pass could share one child-process snapshot between them.