feat(telemetry): add nemo-lens instrumentation and efficiency buckets - #3655
Conversation
0ad1e1a to
683cd35
Compare
683cd35 to
c2d026e
Compare
saumishr
left a comment
There was a problem hiding this comment.
Code review via review-pr skill. Solid opt-in telemetry design (fallbacks, env-before-Ray, span-group gating, good docs/tests). Three follow-ups below for worker init, shutdown flushing, and idle-bucket wiring.
Telemetry coverage on v2 pathwaysFollow-up from review: the instrumentation in this PR covers the legacy algorithm loops and standard Covered
Running with Not covered
Bottom line: legacy in-memory GRPO gets the full span catalog; TQ sync gets metrics-only; SingleController gets essentially nothing today. Worth tracking as follow-up if v2 paths are the primary deployment target. |
|
/ok to test 784dce1 |
|
/ok to test f8cafca |
@rrs45, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/ |
|
/ok to test f8cafca |
|
/ok to test 74b5dad |
1 similar comment
|
/ok to test 74b5dad |
|
/ok to test aad00c1 |
saumishr
left a comment
There was a problem hiding this comment.
LGTM! Discussed offline that the SingleController path will be instrumented as a follow up.
terrykong
left a comment
There was a problem hiding this comment.
Review: telemetry instrumentation
Thanks for this — it's careful work, and a few things made reviewing it much cheaper. Every nemo-lens API call checks out against the pinned 0.1.0 wheel (I unpacked it and diffed the signatures). The "Not yet wired" table in nemo_rl/telemetry/README.md is an honest, specific gap list of the kind most PRs don't ship. And the AST drift-guard in tests/unit/telemetry/conftest.py, which keeps the restated category lists in lockstep with algorithms/utils.py, is the right answer to a duplication you can't remove — two of the comments below just ask you to point that same tool at two more places.
There are a lot of comments, so here is the shape of them. Most cluster into four themes and several are one-liners that fall out of a single decision:
1. Make nemo-lens a base dependency. If it's always installed, the nine try/except ImportError guards around it have no job, and ~163 lines of parallel no-op implementation go away — including a 46-line stub SpanGroup that reimplements lens's own presets. telemetry.enabled becomes the single switch. The comments on pyproject.toml, _fallbacks.py, the six guard sites, and docker/Dockerfile are all one decision.
2. Make the config the interface. All 12 telemetry: fields are projected 1:1 into NEMO_RL_OTEL_*, so the env layer carries nothing the config doesn't. Documenting both costs provenance — a run's settings should be recoverable from its config, not from someone's shell. Keep the standard OTEL_* vars; they're the OTel interop surface. Related: three constrained fields are typed as bare str, so nothing rejects a bad value at load time.
3. Don't map names centrally — standardize them at the source. _RL_OTEL_METRIC_MAP declares sets of metric names to be equivalent, and it has already drifted inside this PR: 3 of its 8 gauges can never receive a value, and 10 of its 16 candidate names are emitted nowhere. Same theme, different place: 39 of 45 timer.time() / span-name pairs disagree, which means correlating a slow span with its timing metric needs a mapping that exists only in the reader's head.
4. Close two drift gaps with the tool you already have. 47 span-name literals are restated by hand in a 332-line doc table with nothing checking they agree; and an idle/* timer added at a call site but not declared in WALL_CLOCK_EFFICIENCY_CATEGORIES is silently dropped — its time then counts as productive, so the efficiency number reads higher than reality.
Two are straight correctness bugs rather than shape: rl.efficiency.pct is tagged window="step" unconditionally even when the value is run-cumulative, and vllm_native_tracing escapes the enabled master switch because it's exported to env before the switch is checked.
On performance evidence — I don't think an overhead A/B is warranted, and I'd rather say so than leave it hanging. All 84 instrumentation sites are per-step or coarser; there's no per-token, per-microbatch or per-request span anywhere, and the comment at trajectory_collector.py:605-612 shows per-sample spans were explicitly considered and rejected. With telemetry off the residual cost is a dict copy and a ContextVar.get() against a step measured in seconds. The one thing worth adding to the PR description is the new teardown cost, which is bounded but real: up to 15s of ray.get plus a 5s force-flush.
Three things I checked and found clean, so they don't become churn later: the _fallbacks.py no-ops faithfully match the real lens contract (upstream managed_span also yields None for a disabled group, and no call site binds the yielded span); the Ray env propagation genuinely works (virtual_cluster.py:306 snapshots all of os.environ); and tests/unit/telemetry/ is collected by the L0_Unit_Tests_Other shard with nemo-lens actually present in the image, so the lens-installed branches do run in CI.
Caveat on this review's evidence: it was produced on macOS with no GPU against a Linux-only uv.lock, so no tests and no linter were run. Everything is from reading source, plus the unpacked nemo-lens 0.1.0 wheel and the pinned OpenTelemetry SDK. Anything depending on runtime behaviour should be treated as unverified.
8a84ca1 to
22b99ab
Compare
|
/ok to test 3e9783c |
|
/ok to test 53282e5 |
Instrument the training loop, generation backends and policy workers with OpenTelemetry spans and metrics via nemo-lens, and account for the idle and wasted time that sits between the productive phases of a run. - Initialize telemetry on Ray workers, not just the driver, and flush it on shutdown so buffered spans are not lost. Example scripts wrap their training body in try/finally so the driver always flushes. - Classify spans into goodput buckets (productive, overhead, idle, wasted). bucket_scope reclassifies spans opened below a caller that knows the intent the callee cannot see, so validation generation counts as overhead. - Emit all eight efficiency categories as OTel metrics, tagged with rl.efficiency.measurement and rl.efficiency.window so wall-clock, thread- seconds and per-step versus run-cumulative values are not mixed. - Propagate W3C trace context across Ray process and thread boundaries so async collector spans nest under the driver root span in the waterfall. - Make nemo-lens a base dependency, removing the optional extra and its ImportError fallbacks. - Add drift tests that parse source and fail the build when a metric-map candidate stops being emitted or a span name goes undocumented. Signed-off-by: Raj Singh <rajsin@nvidia.com>
ee79afc to
0fc4d28
Compare
|
/ok to test 0fc4d28 |
PR3 (#3591) was SQUASH-merged into main as b3b6713, so none of its commits are ancestors of main while PR4 still carries all of them. Git therefore sees PR3's whole diff as independently added on both sides, which is why all 16 conflicts name b3b6713 and why the PR showed CONFLICTING despite the content being identical. That made the classification, not the content, the work. For each conflicted file: is main's version byte-identical to PR3's head (3d9ce21), and does PR4 add anything beyond it? Three groups fell out. GROUP A -- pure squash artefacts, resolved by taking OURS (10 files) fleet_health.py, collective_weight_synchronizer.py, membership.py, nccl_reshard_weight_synchronizer.py, grpo_sc_generation_shard_recovery.sh, test_watchdog_pump.py, test_membership.py, test_reconcile_communicator.py, test_reshard_rebuild.py, test_weight_synchronizer.py main == PR3 exactly and no other PR touched them, so PR4's side is main's content plus PR4's delta. Taking ours loses nothing. GROUP B -- PR4 contributes nothing, resolved by taking THEIRS (2 files) single_controller_utils/setup.py (#3480, #3727, #3821 on top of PR3) tests/unit/single_controller/test_refit_recovery.py (#3480 on top of PR3) GROUP C -- genuine merges (4 files), one per upstream PR below. The six upstream PRs that contributed real content, and what each needed: #3480 recover replay buffer from native TQ checkpoints single_controller.py: rollout_recovery imports. Kept alongside ours. setup.py, test_refit_recovery.py, L1 harness: group B / additive. #3765 log toolcall and thinktag violation rate single_controller.py: VIOLATION_TAG_KEYS. Auto-merged, verified present. #3727 support non-colocated MInf single_controller.py: MegatronGeneration import, kept alongside ours. L1 harness: grpo_megatron_generation_gym_single_controller.sh entry. #3821 warm-start the value model from a critic-pretrain checkpoint config.py: the max_num_epochs validator. Ours only adds restart_dead_shards to FleetHealthConfig, so both survive; verified the field landed in the right class and the validator is intact. #3655 nemo-lens telemetry vllm_generation.py: the @trace_fn decorator on generate. Ours adds restart_shard in a different region; both kept. #3839 pause generation during in-flight refit vllm_generation.py: pause_generation_for_refit / resume_generation_after_refit. Auto-merged, verified present -- worth knowing it exists, since it pauses engines around a refit and this PR restarts them. Verified after resolving: no conflict markers; all four lint hooks clean (the single pyrefly error is the pre-existing unrelated transfer_queue import); 1122 unit tests pass; both submodule pointers and uv.lock/pyproject byte-identical to main. Both sides' work was checked individually rather than assumed: EngineSupervisor wiring, restart_dead_shards, restart_shard, recreate_worker, desired_membership and the report_refit call on our side; the six items above on main's. Note for anyone reproducing locally: #3655 adds a nemo-lens dependency that the pre-merge container image does not carry, so tests fail at import with ModuleNotFoundError: nemo until the venv is refreshed. Plain upstream/main fails the same way in that image; it is not a merge defect. Signed-off-by: asolergibert <asolergibert@nvidia.com>
main's telemetry module imports nemo.lens at module scope (base dep since #3655); the image-baked driver venv predates it and the campaign runs the driver with --no-sync, so the E7 driver died at import (job 6739202). Install the lock-pinned Lens rev alongside orjson. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Pranav Thombre <pthombre@nvidia.com>
main #3655 made nemo.lens a module-scope import in instrumentation.py and span_groups.py, so any venv predating the dep dies at import even with telemetry disabled (job 6740204: image-baked megatron policy worker venvs). Guard the imports with local no-op fallbacks mirroring nemo.lens.fallbacks (unimportable exactly when needed) and a structural SpanGroup stand-in; lens-free envs get False/None/{} semantics, lens-present behavior unchanged. Verified: import + no-op behavior under a blocked nemo.lens meta-path hook, and unchanged pass-through with lens installed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Pranav Thombre <pthombre@nvidia.com>
The 2026-08-10 image's baked venvs predate nemo-lens (#3655): attempt 3 killed the driver (6739202, patched via NRL_DRIVER_PIP_INSTALL) and attempt 4 the megatron policy workers (6740204, not patchable that way — the campaign only force-rebuilds gym/vLLM venvs). The ccbcd4c image (built 2026-08-30 from main) bakes the lock-pinned lens rev 0.2.0+b85578f into the driver venv and all 18 worker venvs, so the git install of lens at spinup is dropped (orjson stays: still absent from the baked driver venv). Referenced through a colon-free symlink in our own enroot dir — pyxis parses --container-image as IMAGE[:TAG], and the source file's name contains a colon. Gym/vLLM venv force-rebuild list unchanged: the image lock lacks this branch's openai 2.6.1/2.44.0 fork. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Pranav Thombre <pthombre@nvidia.com>
main's telemetry module imports nemo.lens at module scope (base dep since #3655); the image-baked driver venv predates it and the campaign runs the driver with --no-sync, so the E7 driver died at import (job 6739202). Install the lock-pinned Lens rev alongside orjson. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Pranav Thombre <pthombre@nvidia.com>
main #3655 made nemo.lens a module-scope import in instrumentation.py and span_groups.py, so any venv predating the dep dies at import even with telemetry disabled (job 6740204: image-baked megatron policy worker venvs). Guard the imports with local no-op fallbacks mirroring nemo.lens.fallbacks (unimportable exactly when needed) and a structural SpanGroup stand-in; lens-free envs get False/None/{} semantics, lens-present behavior unchanged. Verified: import + no-op behavior under a blocked nemo.lens meta-path hook, and unchanged pass-through with lens installed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Pranav Thombre <pthombre@nvidia.com>
The 2026-08-10 image's baked venvs predate nemo-lens (#3655): attempt 3 killed the driver (6739202, patched via NRL_DRIVER_PIP_INSTALL) and attempt 4 the megatron policy workers (6740204, not patchable that way — the campaign only force-rebuilds gym/vLLM venvs). The ccbcd4c image (built 2026-08-30 from main) bakes the lock-pinned lens rev 0.2.0+b85578f into the driver venv and all 18 worker venvs, so the git install of lens at spinup is dropped (orjson stays: still absent from the baked driver venv). Referenced through a colon-free symlink in our own enroot dir — pyxis parses --container-image as IMAGE[:TAG], and the source file's name contains a colon. Gym/vLLM venv force-rebuild list unchanged: the image lock lacks this branch's openai 2.6.1/2.44.0 fork. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Pranav Thombre <pthombre@nvidia.com>
The lock recorded megatron-bridge requires-dist computed from a stale local submodule state (pre-#3655: no nemo-lens dep, old TransformerEngine rev, no otel extra), which a warm local uv cache kept reproducing — every local 'uv lock --check' passed while CI, resolving the clean d352aceda pin cold, correctly failed. Regenerated in a CI-identical shallow clone with a cold cache (uv 0.11.28, revision 3); verified 'uv lock --check' passes cold under both 0.11.28 (container pin) and 0.12.8 (CI's unpinned latest). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Pranav Thombre <pthombre@nvidia.com>
main's telemetry module imports nemo.lens at module scope (base dep since #3655); the image-baked driver venv predates it and the campaign runs the driver with --no-sync, so the E7 driver died at import (job 6739202). Install the lock-pinned Lens rev alongside orjson. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Pranav Thombre <pthombre@nvidia.com>
main #3655 made nemo.lens a module-scope import in instrumentation.py and span_groups.py, so any venv predating the dep dies at import even with telemetry disabled (job 6740204: image-baked megatron policy worker venvs). Guard the imports with local no-op fallbacks mirroring nemo.lens.fallbacks (unimportable exactly when needed) and a structural SpanGroup stand-in; lens-free envs get False/None/{} semantics, lens-present behavior unchanged. Verified: import + no-op behavior under a blocked nemo.lens meta-path hook, and unchanged pass-through with lens installed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Pranav Thombre <pthombre@nvidia.com>
The 2026-08-10 image's baked venvs predate nemo-lens (#3655): attempt 3 killed the driver (6739202, patched via NRL_DRIVER_PIP_INSTALL) and attempt 4 the megatron policy workers (6740204, not patchable that way — the campaign only force-rebuilds gym/vLLM venvs). The ccbcd4c image (built 2026-08-30 from main) bakes the lock-pinned lens rev 0.2.0+b85578f into the driver venv and all 18 worker venvs, so the git install of lens at spinup is dropped (orjson stays: still absent from the baked driver venv). Referenced through a colon-free symlink in our own enroot dir — pyxis parses --container-image as IMAGE[:TAG], and the source file's name contains a colon. Gym/vLLM venv force-rebuild list unchanged: the image lock lacks this branch's openai 2.6.1/2.44.0 fork. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Pranav Thombre <pthombre@nvidia.com>
The lock recorded megatron-bridge requires-dist computed from a stale local submodule state (pre-#3655: no nemo-lens dep, old TransformerEngine rev, no otel extra), which a warm local uv cache kept reproducing — every local 'uv lock --check' passed while CI, resolving the clean d352aceda pin cold, correctly failed. Regenerated in a CI-identical shallow clone with a cold cache (uv 0.11.28, revision 3); verified 'uv lock --check' passes cold under both 0.11.28 (container pin) and 0.12.8 (CI's unpinned latest). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Pranav Thombre <pthombre@nvidia.com>
What does this PR do ?
Add Nemo Lens instrumentation covering basic components along with efficiency categorization
Usage
Emitting OpenTelemetry data
Telemetry is opt-in and a zero-cost no-op unless (a)
nemo-lensis installed and (b) it's enabled. No code changes are needed to get spans/metrics out of the built-in GRPO/algorithm loops.1. Install the extra
uv sync --extra telemetry # or: uv pip install 'nemo-lens[sdk]'2a. Enable via env vars (quickest)
2b. Or via the
telemetry:config blockRaw
NEMO_RL_OTEL_*/OTEL_EXPORTER_OTLP_*env vars always override the YAML.3. No backend handy? Print spans to the console
4. Instrumenting your own code
Import the wrappers from
nemo_rl.telemetry.instrumentation(not rawnemo-lens) so leaf spans automatically get therl.bucketefficiency tag:Driver/worker lifecycle is already wired in the
examples/run_<algo>.pyentrypoints:init_telemetry_driver(config, algorithm="grpo")runs beforeinit_ray()(soNEMO_RL_OTEL_*propagates to every Ray worker via the runtime env), andshutdown_telemetry()runs at the end.What you get
rl.grpo.step,rl.vllm.generate,rl.grpo.policy_update, …), with leaf spans taggedrl.bucket ∈ {productive, overhead, idle, wasted}for offline goodput rollups.rl.*) mirrored from the trainingLogger.See
docs/observability/for the full config reference, span-group catalog, and OTLP/collector setup.Before your PR is "Ready for review"
Pre checks:
Additional Information