Repository navigation
feat(telemetry): task id, repeat index, reward and health verdict on rollout spans - #5052
Conversation
…rollout spans Signed-off-by: Ingo Guehring <iguehring@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
/review |
|
🌿 Preview your docs: https://nvidia-preview-pr-5052-10a22d932aa1.docs.buildwithfern.com/nemo/gym |
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
SHIP WITH CARE — one RISK in the new regression test’s optional-dependency handling; no production-path correctness issue found.
Reviewed all 10 changed files and directly affected task-identity, verification, health-evaluation, and telemetry paths. Review was static; no tests were executed.
Review generated by codex. This was a full review. Request a full review with /review reset=true.
| health = next(s for s in exporter.get_finished_spans() if s.name == "gym.rollout.health") | ||
| assert health.context.trace_id == parent.trace_id | ||
| assert health.parent.span_id == parent.span_id | ||
| assert health.attributes["verdict"] == "healthy" |
There was a problem hiding this comment.
RISK — Preserve the SDK-only test configuration
Failure / blast radius: With OpenTelemetry SDK installed but nemo-lens absent, this test fails at health.attributes["verdict"]: the fallback setter deliberately drops attributes, so the finished span has no verdict. Span creation and ancestry still work. This introduces a false failure in the configuration this module explicitly supports; the neighboring linked-root test already distinguishes it.
Correction / preservation: Keep the production fallback unchanged and replace only this assertion with the existing conditional pattern:
if LENS_INSTALLED:
assert health.attributes["verdict"] == "healthy"
else:
assert not health.attributesRetain the unconditional trace-ID, parent-span-ID, and current-context assertions. The invariant is that ended-parent linkage works with or without Lens, while attributes are required only when the real setter is available.
Acceptance (proposed, not executed): Run test_record_child_span_joins_a_parent_that_has_already_ended in both an SDK-only environment and an SDK-plus-Lens environment. Both must pass: the former exports an attribute-free child; the latter exports verdict="healthy"; both retain the parent’s trace ID and span ID without attaching a current span. Suggested code is source-traced, not execution evidence; verify before adopting.
Signed-off-by: Ingo Guehring <iguehring@nvidia.com>
|
Addressed the review finding in 10a22d9: |
|
/review |
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
SHIP — LGTM — no reliability concerns in this follow-up.
The prior RISK is resolved. The ended-parent regression test now requires verdict="healthy" only with Lens installed and expects no attributes otherwise, matching the no-op fallback contract. Trace-ID, parent-span-ID, and current-context assertions remain unconditional.
Accounted for all 10 changed files and compared them with the previously reviewed head, 9404a20bddd561d16019412aa4e1bd53c1789412. The subsequent change is assertion-only; production telemetry, documentation, and integration-test wiring are unchanged. No unresolved prior findings or qualifying introduced regressions remain.
Static inspection only; no tests were executed.
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.
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA-NeMo/Gym/actions/runs/40293474270 |
…th test Main's #5052 passes the dispatch span context to _record_rollout_health as a fourth argument. Give this test's stub the same optional parameter as main's other stubs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Yuhe Zhang <yuhez@nvidia.com>
…A-NeMo#3302) * feat(eval): persist outcomes and resume unfinished evaluations Rebase the final NVIDIA-NeMo#3302 implementation onto main after NVIDIA-NeMo#4029 and NVIDIA-NeMo#4178. Preserve cancellation and timing cleanup, remove retired Stirrup migration paths, forward GDPVal retry progress, and allow selected-source judge recovery into separate output. Verify saved rewards reach the live counter exactly once. Signed-off-by: Yuhe Zhang <yuhez@nvidia.com> * fix(eval): preserve retry and scoring compatibility Signed-off-by: Yuhe Zhang <yuhez@nvidia.com> * test(gdpval): isolate HTTP recovery fixtures from core tests Signed-off-by: Yuhe Zhang <yuhez@nvidia.com> * fix(eval): preserve managed recovery through explicit resubmission Signed-off-by: Yuhe Zhang <yuhez@nvidia.com> * test(eval): align tracing fixtures with managed recovery Return a valid scored completion from the incoming telemetry fixture. Verify that resuming a completed run neither sends requests nor emits duplicate rollout dispatch traces. Signed-off-by: Yuhe Zhang <yuhez@nvidia.com> * fix(eval): align recovery health and retry reporting Signed-off-by: Yuhe Zhang <yuhez@nvidia.com> * fix(eval): mark exhausted resume chains incomplete Signed-off-by: Yuhe Zhang <yuhez@nvidia.com> * test(eval): align recovery checks with saved run policy Signed-off-by: Yuhe Zhang <yuhez@nvidia.com> * test(eval): accept the health span context in the native-failure health test Main's NVIDIA-NeMo#5052 passes the dispatch span context to _record_rollout_health as a fourth argument. Give this test's stub the same optional parameter as main's other stubs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Yuhe Zhang <yuhez@nvidia.com> * fix(eval): accept typed judge failures and keep failure paths and legacy classes NVIDIA-NeMo#5072 lets judge_failsafe name the observed cause as its failure_kind (for example judge_rate_limited) while the routing class stays judge_failed, and adds a diagnostic _ng_failure_path. Its base Environment Server also fills in a kind such as provider_unavailable when the producer left it unset. - _episode_record() accepts the judge markers and the path on a result whose class is judge_failed and whose kind is a judge kind, not only judge_failed itself. Such a result stays a retryable judge failure that --judge-failed-only re-judges, instead of a terminal environment_protocol_violation (or an aborted run without sidecar routing). - _sanitize_legacy_result() and the native failure branch of _episode_record() keep _ng_failure_path on the saved row as a diagnostic, outside the typed record. - An Environment Server failure with a kind keeps environment_server_failed as its legacy class, so count_failure_classes_as_zero and the other class selectors still match it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Yuhe Zhang <yuhez@nvidia.com> * test(eval): reply with the dispatched episode identity in the provenance tests Main's provenance tests answered every row with a fixed or malformed episode id. Collection validates a reply against the episode it dispatched, so answer with that identity. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Yuhe Zhang <yuhez@nvidia.com> * fix(eval): keep a legacy link's companions when checking health Health resolved every rollout path to the file it links to, so a legacy run reached through a file link lost the sidecar and inventory kept beside the link. Aggregation and the shard glob keep the link's name in that case (resolve_rollout_path), so a recheck counted less than the aggregation it checks. Resolve health paths with the same reader rule. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Yuhe Zhang <yuhez@nvidia.com> * test(eval): align main's aggregation tests with the run-history merge rules NVIDIA-NeMo#4996's tests assumed main's aggregation, which merges a run onto itself and into a directory holding a run. This branch refuses both, because merged rows would replace rows the run's history indexes. - Merging a run onto itself, directly or through a linked directory, is refused before anything is written; merge_shards=false still recomputes its metrics in place. - A fresh run into an aggregated output drops its shard list; a resume finds no run history there and stops before writing. - Aggregating into a run with history is refused and changes nothing. - A file link's companions are read beside the link when they sit there, otherwise beside the file it resolves to, which owns them, as do a linked output's shard list and reports. - A failed publication leaves the original rows and a shard list that names contents that never landed, so a recheck refuses. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Yuhe Zhang <yuhez@nvidia.com> * test(eval): format the omitted-rollout test parameters Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Yuhe Zhang <yuhez@nvidia.com> * fix(arena): also limit final judge-outage failures With the run history, an outage counts as a final failure once its retries are used up, so the not-final limit NVIDIA-NeMo#5073 set would stop catching it. Each Arena benchmark now sets the final-failure limit to the same share. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Yuhe Zhang <yuhez@nvidia.com> * fix(eval): accept NVIDIA-NeMo#5072's judge failure report on judge_failed results NVIDIA-NeMo#5072's judge_failsafe now attaches a failure report instead of a failure path: `_ng_failure_events`, `_ng_failure_id` and, when events were left out, `_ng_failure_events_omitted`. NVIDIA-NeMo#3302 rejected those as collector-owned keys, so a judge outage either aborted collection or became a terminal environment_protocol_violation that lost the saved answer. A judge_failed result may now carry those three keys. They stay on the row as diagnostics, beside the version-1 failure record and not inside it, and legacy agents keep them too. The unused `_ng_failure_path` handling is removed, and other collector-owned keys stay forbidden. The judge fixtures use the row NVIDIA-NeMo#5072's code produces for a judge 429. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Yuhe Zhang <yuhez@nvidia.com> * refactor(eval): name the saved failure record RolloutFailureRecord The failure-boundary design names the saved envelope RolloutFailureRecord: a run's identity plus the failure data, apart from the runtime exceptions that report failures. The record, its version and its fields are unchanged. `RolloutFailure` stays importable as an alias of the same class. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Yuhe Zhang <yuhez@nvidia.com> * test(eval): mark the judge report's event ids for the secrets detector The event ids are random hex from NVIDIA-NeMo#5072's judge_failsafe output, which detect-secrets reads as high-entropy strings. Name each once with the repository's allowlist pragma. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Yuhe Zhang <yuhez@nvidia.com> * fix(eval): mark incomplete CLI runs as failed in tracking Preserve the final saved-coverage gate after merging NVIDIA-NeMo#5213, and mark its rejected completions as failed for exporters. Keep SIGTERM and retryable/exhausted exit codes unchanged. Cover both CLI paths and tolerated partial runs with a real local MLflow store. Signed-off-by: Yuhe Zhang <yuhez@nvidia.com> * fix(eval): integrate main's health layering and partial-stage checks Move collection health reporting below the CLI layer and pass shutdown state from CLI callers. Register the recovery modules in main's import contract, without adding dependency exceptions, and refresh the import inventory for three private helpers intentionally replaced by the recovery implementation. Cover accepted GDPVal partial stages followed by a still-incomplete stage, including a no-progress resume. Accepted omissions must not be counted twice. Align the older GDPVal test with the stage-aware partial-acceptance report. Validation: 2,707 targeted tests and all-files pre-commit passed. The new GDPVal regression fails under a controlled double-credit mutation. Signed-off-by: Yuhe Zhang <yuhez@nvidia.com> * fix(imports): finish recovery compatibility with package moves Expose the existing ConfigError and ENVIRONMENT_SERVER_FAILED definitions through the package APIs used by the conformance runner and legacy adapter. Update the remaining recovery test and conformance import paths. No failure, scoring, or retry policy changes. Validation: 3,197 targeted tests and all-files pre-commit passed, including package interfaces, import layering, recovery, legacy adapter and conformance. Signed-off-by: Yuhe Zhang <yuhez@nvidia.com> * test(eval): follow verification and runtime package moves Signed-off-by: Yuhe Zhang <yuhez@nvidia.com> * fix(eval): preserve run identity and saved recovery policy Signed-off-by: Yuhe Zhang <yuhez@nvidia.com> * test(swe-pivot): align runtime stub with public imports Signed-off-by: Yuhe Zhang <yuhez@nvidia.com> --------- Signed-off-by: Yuhe Zhang <yuhez@nvidia.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Rollout spans now carry the facts needed to name a task and show its outcome, which so far lived only in
rollouts.jsonlandrollout_verdicts.jsonl.gym.rollout.dispatch:nemo.gym.task.id(task_id,problem_idorinstance_id, elsetask_name; omitted when the row has none) andnemo.gym.rollout.repeat.gym.verify:nemo.gym.rollout.reward, only for a numeric, unmasked reward.gym.rollout.healthspan, a child of the already-ended dispatch span and in the same trace, withnemo.gym.health.verdictand the sorted distinctnemo.gym.health.checks. It is emitted for every traced dispatch, whether or not metrics export is on.The reward is a number, not content. Per-rollout values stay on spans; the bounded
gym.rollout.rewarded_totalcounter is unchanged.Closes #4988.
Validation:
main(test_supervisor_is_the_only_writer_of_the_active_servers_gauge; memory and connection-pool tests).nemo.gym.task.idandnemo.gym.rollout.repeat; everygym.verifyhasnemo.gym.rollout.rewardand the rollout id; every rollout has onegym.rollout.healthchild in the same trace (3 unhealthy with their checks, 7 healthy). 10 spans of each name, so nothing is duplicated; the same attributes read back from ClickHouse.Known limits: in the flat-row path
gym.verifyhas nonemo.gym.rollout.id, so the reward is joined by trace id.🤖 Generated with Claude Code