Skip to content

feat(telemetry)!: rl.iteration on every span and the shared nv.dl.* attributes - #4607

Merged
terrykong merged 17 commits into
mainfrom
raj/lens-update1
Oct 16, 2026
Merged

terrykong merged 17 commits into
mainfrom
raj/lens-update1

Conversation

@rrs45

@rrs45 rrs45 commented Oct 8, 2026 •

Copy link
Copy Markdown
Collaborator

What does this PR do ?

Bumps nemo-lens to 4d045c03, nine commits past the v0.3.0 tag, and uses what it adds: every span in a training step is now filterable by step, an RL run reports the shared nv.dl.* attributes a Megatron run does, and a checkpoint's durability is traced separately from its cost.

Five commits carry the change, each standalone; the rest of the branch is review follow-up:

Commit Change
chore(deps) nemo-lens pinned to 4d045c03 (nine commits past the v0.3.0 tag, for the observable instrument kinds)
feat(telemetry)! rl.iteration set once per step, reaching every span inside it; rl.step collapsed into it
feat(telemetry) the shared nv.dl.* resource attributes, taken from nemo.lens.semconv
feat(telemetry) rl.checkpoint.finalize, open until the checkpoint is durable
feat(telemetry) rl.host.memory.*, per-node host memory scraped from Ray

rl.iteration on every span in a step

Only rl.<algo>.step carried the step number. Leaves and checkpointing carried none, so "what else was happening in step 412" could not be asked. Each training loop now wraps its step body in iteration_scope, which uses lens v0.3.0's scoped span attributes — a leaf does not have to be threaded the counter, and spans added later are covered for free.

A span that passes the attribute itself keeps its own value, which evaluate_span relies on: validation also runs once before training and reports iteration 0 there.

rl.step is removed. It meant the same thing as rl.iteration and appeared on two spans; both now carry rl.iteration.

The shared nv.dl.* attributes

An RL run set two of them, nv.dl.rank and nv.dl.world_size, so a backend collecting NeMo-RL alongside Megatron could not compare the two on topology, batch shape, step target or library versions. It now reports 17, all from config RL already resolves, and 7 are names in lens's shared DL_RESOURCE_TYPES vocabulary (previously 2).

Three keys that were string literals now come from nemo.lens.semconv, so a typo is an ImportError rather than silent data loss. Two change spelling in the process — see Breaking changes.

Metric names stay in the rl.* scope deliberately. Lens registers a dl.* series for whichever trainer owns the step, so an RL job publishing dl.grad_norm would write into a co-resident Megatron trainer's series; rl.grad_norm and rl.learning_rate are spelling matches for exactly that reason. Lens's own semconv design doc states this.

rl.checkpoint.finalize

rl.<algo>.checkpointing ends when the training loop is released, which with async save is after D2H staging only — the writers are still going and tmp_step_N has not been renamed. The new span closes when the rename lands, answering "when did step 7 become resumable". No flag distinguishes a sync save from an async one: every begin_finalization caller passes wait_fn whether or not async save is enabled, so the checkpoint manager cannot tell them apart.

It carries no rl.bucket: it overlaps the next step's productive spans, so an overhead tag would charge one stretch of wall clock to two buckets and push the goodput denominator past the wall clock.

rl.host.memory.*

Telemetry reported no memory signal at all. Every raylet already publishes ray_node_mem_used / ray_node_mem_total on its own Prometheus endpoint, and RayGpuMonitorLogger reads them for W&B, so the numbers existed but reached only one backend. HostMemoryMonitor scrapes the same endpoints from the driver and records rl.host.memory.used / rl.host.memory.total as gauges in bytes, one point per live node.

Scraped separately rather than teed out of the GPU monitor, which flattens the node's identity into the metric name (node.3.mem_gb) and drops the hostname, and which is gated on logger.monitor_gpus — reading it would make an OTel series vanish when someone turned off a W&B feature. Collection is gated on telemetry exporting and nothing else; it reuses gpu_monitoring.collection_interval so the two families line up on a dashboard.

host.name is a metric attribute here, not the resource attribute of the same name: the resource one would say "head node" on every point, since the driver reports about hosts other than its own. It prefers Ray's NodeManagerHostname and falls back to the IP when Ray resolved no hostname.

rl.worker_groups names the groups resident on the host, resolved driver-side through placement_group_table as each RayWorkerGroup is built — no worker is asked anything. It is plural and comma-joined on purpose: the value it labels is whole-host memory, so under colocation summing by it double-counts, and no worker group owns the raylet, the object store or the driver regardless. Group on it to pick which hosts to look at; read host.name for the number. Documented as such in docs/observability/metrics.md.

Breaking changes

Dashboards filtering on these need repointing:

  • rl.step is removed — use rl.iteration, which is now on every span in the step rather than two.
  • dl.tensor_parallel.size → nv.dl.topology.size.tp and dl.pipeline_parallel.size → nv.dl.topology.size.pp. Both spell the same quantity; the topology keys are the resource-scoped spelling and the dl.* family the span and metric one. Nothing on lens's side picks between them — normalize_resource_attributes runs only over the attributes lens derives from OTEL_RESOURCE_ATTRIBUTES, while the resource_attributes handed in from here are merged as they are — so either spelling would have arrived intact and getting it right is on NeMo-RL. This is a convention fix, not a typing one.

No behaviour change outside telemetry.

Issues

None.

Usage

Unchanged — one switch, as before:

telemetry:
  enabled: true
  exporter: otlp
  span_groups: per_step

Then filter a step's whole subtree in the collector:

rl.iteration = 412

Before your PR is "Ready for review"

Pre checks:

  • Make sure you read and followed Contributor guidelines
  • Did you write any new necessary tests?
  • Did you run the unit tests and functional tests locally? Visit our Testing Guide for how to run tests
  • Did you add or update any necessary documentation? Visit our Document Development Guide for how to write, build and test the docs.

Additional Information

Tests. The telemetry and checkpoint suites pass locally against the pinned lens rev, including every AST drift guard. New coverage: scoped iteration reaching leaf and checkpoint spans, an explicit value winning over the scope, the scope not leaking past the step, the finalize span outliving its step and being filed under the triggering one, a failed finalization reaching both the span and the caller, a telemetry failure not costing the checkpoint, one point per host however many raylets it runs, a label the scrape stops reporting ceasing to be exported, an unparsable page costing only its own point, a non-positive collection interval starting nothing rather than spinning, and the placement-group lookup that resolves a worker group to its nodes.

The lens version is confusing on purpose. The pin is a rev rather than a tag because no release carries the observable instrument kinds yet: 4d045c03 is nine commits past v0.3.0. Lens's package_info.py still declares 0.2.0 there, so the >=0.2.0 floor is deliberately unchanged and uv.lock reads version = "0.2.0+4d045c0". A 0.3.0 floor would not resolve. Both lockfiles pass uv lock --check at CI's pinned uv 0.11.28.

Known gap, not addressed here. NeMo-Gym calls NemoLensConfig.from_env(span_group_cls=...), a parameter lens v0.3.0 removed, and catches only ValueError — so TypeError kills NemoGym._spinup(). This reproduces on main today and is independent of this PR, but it does mean Gym telemetry cannot be enabled until Gym's pin and RL's agree.

Checkpoint behaviour is unchanged. _rename_checkpoint is byte-identical to main. An earlier revision of this branch routed it through a stricter tmp_step_N parser, which turned a malformed staging name from "publish under whatever sits between the second and third underscore" into a ValueError. That is arguably the better behaviour, but it is a change to the checkpoint publication path and does not belong in a telemetry PR, so it was reverted. begin_finalization reads the step the same way _rename_checkpoint does, guarded and dropped to None on anything unexpected, because that value only labels a span. The pre-existing quirk — tmp_step_final publishes as step_final — is therefore still present, and is left for a checkpoint-scoped change.

@copy-pr-bot

copy-pr-bot Bot commented Oct 8, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions github-actions Bot added the Documentation Improvements or additions to documentation label Oct 8, 2026
@rrs45
rrs45 force-pushed the raj/lens-update1 branch from a6865ef to 7bc8e41 Compare October 8, 2026 22:41
@rrs45 rrs45 added the CI:Lfast Runs a fast test suite and re-use nightly `main` container (but sync dependencies to PRs version) label Oct 8, 2026
@rrs45

rrs45 commented Oct 8, 2026

Copy link
Copy Markdown
Collaborator Author

/ok to test b51c998

@rrs45 rrs45 added CI:L0 Run doctests and unit tests and removed CI:Lfast Runs a fast test suite and re-use nightly `main` container (but sync dependencies to PRs version) labels Oct 8, 2026
@rrs45

rrs45 commented Oct 9, 2026

Copy link
Copy Markdown
Collaborator Author

/ok to test 5551a17

@rrs45

rrs45 commented Oct 10, 2026

Copy link
Copy Markdown
Collaborator Author

/ok to test 97831e3

@rrs45 rrs45 changed the title feat(telemetry)!: carry rl.iteration on every span and report the shared nv.dl.* attributes feat(telemetry)!: rl.iteration on every span and the shared nv.dl.* attributes Oct 10, 2026
@rrs45

rrs45 commented Oct 10, 2026

Copy link
Copy Markdown
Collaborator Author

/ok to test 4386bdb

@rrs45

rrs45 commented Oct 11, 2026

Copy link
Copy Markdown
Collaborator Author

/ok to test 43a538a

@nemo-automation-bot
nemo-automation-bot Bot requested a review from a team October 11, 2026 17:16
@rrs45

rrs45 commented Oct 11, 2026

Copy link
Copy Markdown
Collaborator Author

/ok to test 62647d9

@rrs45

rrs45 commented Oct 12, 2026

Copy link
Copy Markdown
Collaborator Author

/ok to test d94ad85

@rrs45

rrs45 commented Oct 12, 2026

Copy link
Copy Markdown
Collaborator Author

/ok to test 2f6b955

@terrykong terrykong left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this — the main pieces are the right shape:

  • iteration_scope (one ContextVar per step instead of threading the counter into every call site) is the right design.
  • The finalize span sets its iteration explicitly, with a clear reason.
  • parse_memory_samples and ray_nodes can be tested without Ray.
  • Keeping HostMemoryMonitor separate from RayGpuMonitorLogger is right, and the observable-gauge rework keeps HTTP out of the export callbacks.
  • The lens bump also picks up lens 4d045c0, which makes the OTLP log exporter follow OTEL_EXPORTER_OTLP_PROTOCOL, so with http/protobuf logs now really go over HTTP.
  • The new tests are strong: four mutations of the checkpoint feature were each caught.

The biggest finding is the rl.checkpoint.async_save tag, which is true on every checkpoint (inline on checkpoint.py).

PR description fixes:

  • It still says nemo-lens is "pinned to the v0.3.0 tag (7bcc03ce)" and mentions "lens v0.3.0" throughout. The pin is now lens main at 4d045c0 (v0.3.0 + 9 commits); please update those mentions.
  • Breaking changes still says _rename_checkpoint now raises. It is byte-identical to main (commit 9ebdc9e reverted it); the closing paragraph is the true one.
  • "7 land in DL_RESOURCE_TYPES": lens does not type resource_attributes (inline on setup.py), so drop the typing claim; the count does not matter.
  • The commit table lists 5 of the 9 commits; say the other 4 are fix-ups that do not stand alone.
  • Teardown: shutdown_telemetry can now wait up to 11 s in the worst case, when a scrape is stuck on a hung node (stop() joins for _SHUTDOWN_GRACE_S, host_memory.py#L389-L395). Normally it returns at once. Worth one line.

Follow-ups, not this PR:

  • Per-process host memory: rl.host.memory.* is whole-host, so it can't show which actor or task is using the memory. Ray already publishes a per-process breakdown on the same /metrics page. This can be done in a follow-up, tracked in #4681.
  • RayGpuMonitorLogger._collect still scrapes dead nodes; it could reuse host_memory.ray_nodes(), which skips them.

CI: has not run on 2f6b955 yet; it needs /ok to test. On the previous head, lint passed and the only red lane (L0_Unit_Tests_Models_4, in test_stream_weights_via_ipc) was unrelated to telemetry. Nothing here was run on GPU; findings come from reading the source plus pure-Python checks against the pinned lens.

Visual explainer: https://terrykong.github.io/gh-pages-poc/terryk/pr-4607-step-number.html
Visual explainer: https://terrykong.github.io/gh-pages-poc/terryk/pr-4607-async-save-flag.html

Generated by Claude Code

Comment thread nemo_rl/utils/checkpoint.py Resolved
Comment thread nemo_rl/telemetry/instrumentation.py Resolved
Comment thread docs/observability/extending.md Resolved
Comment thread nemo_rl/telemetry/setup.py Resolved
Comment thread nemo_rl/telemetry/setup.py Resolved
Comment thread nemo_rl/telemetry/host_memory.py Resolved
Comment thread nemo_rl/telemetry/host_memory.py Resolved
Comment thread nemo_rl/telemetry/setup.py Resolved
Comment thread nemo_rl/telemetry/setup.py Resolved
Comment thread docs/observability/configuration.md Resolved
@rrs45

rrs45 commented Oct 13, 2026

Copy link
Copy Markdown
Collaborator Author

/ok to test ea31088

@rrs45

rrs45 commented Oct 14, 2026

Copy link
Copy Markdown
Collaborator Author

/ok to test 5c54d25

@rrs45

rrs45 commented Oct 14, 2026

Copy link
Copy Markdown
Collaborator Author

/ok to test 6ee1b22

@rrs45

rrs45 commented Oct 14, 2026

Copy link
Copy Markdown
Collaborator Author

/ok to test ed30dc6

@terrykong

Copy link
Copy Markdown
Collaborator

/ok to test eeebf3f

@copy-pr-bot

copy-pr-bot Bot commented Oct 16, 2026

Copy link
Copy Markdown

/ok to test eeebf3f

@terrykong, there was an error processing your request: E2

See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/

@terrykong

Copy link
Copy Markdown
Collaborator

/ok to test bf2ac32

rrs45 and others added 17 commits October 15, 2026 18:01
The pin was b0f977d, an untagged commit four behind the v0.3.0 tag, so
the pin now names a release instead of a point on main.

Picks up shared attribute encoding and typed reconstruction, the
training resource semconv fields, optional group gating on explicit
span emission, and scoped span attributes.

The >=0.2.0 floor in [dependencies] stays as is: package_info.py still
declares 0.2.0 at the v0.3.0 tag, so raising it to >=0.3.0 would fail
to resolve. Lens' pyproject.toml is byte-identical across the two revs,
so the lockfile change is confined to the rev and version strings.

Signed-off-by: Raj Singh <rajsin@nvidia.com>
rl.step and rl.iteration were the same counter under two names, split by
which span emitted it rather than by meaning: the training loops read
total_steps and tagged rl.iteration, while validate() takes a parameter
named step and tagged rl.step. Both resolve to the same value, so
"everything in training step 412" needed two filters and knowledge of
which spans used which.

Collapse onto rl.iteration and set it once per step with iteration_scope,
built on lens' span_attributes. Leaves, checkpoint spans and the rollout
and generation spans under a step now all carry it without the counter
being threaded to each call site -- which is why checkpoint spans had no
attributes at all, and why a span added later would have missed it again.

Spans left outside the scope are the ones with no single step: rl.<algo>.job
wraps the run, and rl.sc.generate_and_push runs on the rollout pump's clock
and keeps rl.target_step. Worker-side model init is in another process, so
the ContextVar does not reach it.

Also tags the ppo and rm evaluate spans, which had step in scope but never
attached it, and scopes sft_v2 at the loop body since its step span is a
decorator and it checkpoints from the caller.

BREAKING CHANGE: spans no longer carry rl.step. Dashboards and queries
filtering on it should use rl.iteration, which holds the same value.

Signed-off-by: Raj Singh <rajsin@nvidia.com>
An RL run previously set two attributes from lens's shared distributed-learning
namespace, nv.dl.rank and nv.dl.world_size, so a backend collecting NeMo-RL
alongside Megatron or NeMo could not compare the two on topology, batch shape,
step target or library versions -- the facts a reader reaches for first when one
run is slower than another.

Populate the rest of that surface from the config RL already resolves: provider
name, run uuid, local rank, tp/pp/dp sizes, global and micro batch size,
sequence length, optimizer, recompute granularity, the configured step target,
and the torch/CUDA/NCCL/transformer-engine versions as actually loaded.

Three values that were written as string literals now come from
nemo.lens.semconv, so a typo fails as an ImportError rather than becoming
silent data loss. Two of them change spelling in the process:
dl.tensor_parallel.size and dl.pipeline_parallel.size become
nv.dl.topology.size.tp and .pp. Both spell the same quantity, but only the
topology one is in lens's DL_RESOURCE_TYPES registry, which is what
normalize_resource_attributes types a resource attribute against -- the dl.*
family is the span and metric spelling. A dashboard filtering on the old keys
has to be repointed.

Metric names are deliberately left in the rl.* scope. Lens registers a dl.*
series for whichever trainer owns the step, so publishing dl.grad_norm from an
RL job would write into a co-resident Megatron trainer's series; rl.grad_norm
and rl.learning_rate are spelling matches for exactly that reason.

The dp size is derived rather than read, since RL configures none, and is
omitted when generation runs on dedicated GPUs -- the cluster then holds ranks
the policy does not have.

Signed-off-by: Raj Singh <rajsin@nvidia.com>
rl.<algo>.checkpointing ends when the training loop is released, which with
async save is after D2H staging only -- the writer processes are still going
and tmp_step_N has not been renamed. Nothing answered "when did step 7 become
resumable", which is the question asked after a crash.

Add rl.checkpoint.finalize, opened on the thread begin_finalization spawns and
closed when the rename lands. It carries rl.checkpoint.async_save, because both
save shapes emit the same span and a near-zero duration is otherwise ambiguous
between a sync save and an async one that had already finished.

Two properties need stating. Its rl.iteration is the step that triggered the
save rather than the step running at the rename: a bare thread starts with an
empty context, so the enclosing iteration_scope is both unreachable and, by the
time the span closes, wrong. The step is read back from the tmp_step_N name,
which is also now the only parser of that name -- _rename_checkpoint used to
read it a second way, and on an unexpected name that one published the
checkpoint under whatever sat between the second and third underscore.

And it carries no rl.bucket. It overlaps the next step's productive spans, so
an overhead tag would charge one stretch of wall clock to two buckets and push
the goodput denominator past the wall clock. It is a child of the staging span
for navigability, which does mean step 7's span subtree stays open into step 9;
read step cost off rl.<algo>.step rather than off the subtree.

Opening the span is guarded. The work runs inside the try whose exception
finalize_pending re-raises, so an unguarded failure here would abort the run
and skip the rename -- reporting a lens incompatibility as a failed save.
Failures from the wait and the rename still reach both the span and the caller.

Signed-off-by: Raj Singh <rajsin@nvidia.com>
Every example config carries both policy backend blocks fully populated
and only the enabled one describes the run, so reading whichever value is
non-empty picked the disabled block. dpo.yaml reported tp 2 against the
Automodel tp 1 it really uses, and grpo_math_1B.yaml reported optimizer
adam against AdamW -- wrong rather than missing, which is worse. Select
the backend from megatron_cfg.enabled once, then read only that block.

Drop nv.dl.topology.size.dp. It was the one attribute derived rather than
read, and dividing the cluster's GPU count by the model-parallel sizes
assumes the policy owns every GPU. A reward model with its own resources,
distillation teachers, expert parallelism and non-colocated generation
each leave it fewer, and the division came out too high rather than
failing. Read the worker's device mesh if the real number is wanted.

Also:
- sft_v2 reports itself as sft_v2 but keeps its settings under the sft
  section, so it never reported a training target. Map the section name.
- nv.dl.run.uuid carried the raw run id, which is already reported as
  nemo.run.id. A backend joining RL runs to Megatron ones groups on
  lens's derived uuid5, so the raw id there matched nothing.
- Read transformer-engine's version from distribution metadata instead of
  importing it, which loads its PyTorch extension in every process that
  sets up telemetry and raises OSError, not ImportError, when the shared
  library will not load.
- Correct the docs: rl.iteration only stamps driver-process spans, the
  campaign stage is not always RL, local_rank is not a device index, and
  an unregistered resource attribute is passed through untyped rather
  than rejected.

Signed-off-by: Raj Singh <rajsin@nvidia.com>
RayWorkerGroup sets LOCAL_RANK from the worker's bundle index in its
placement group, not from a per-node counter. That is node-local only
when each node has its own placement group, and a generation backend
spanning nodes asks for a single unified one instead -- SGLang always,
vLLM, TRT-LLM and Megatron when their parallelism crosses nodes. The
unified group's bundles are summed across nodes, so the index runs
across all of them and the previous "within its own node" wording would
have had a reader grouping processes by a host that is not implied.

Signed-off-by: Raj Singh <rajsin@nvidia.com>
The resource-attribute summary in span-groups.md carried its own copy of
the claim, which the configuration.md fix did not reach. sft and sft_v2
report SFT and rm reports RM.

Also take derive_nv_dl_run_uuid from nemo.lens rather than reaching into
nemo.lens.resources.slurm for it. The function is re-exported at the top
level and listed in __all__, so the public path is the one that survives
lens moving it.

Signed-off-by: Raj Singh <rajsin@nvidia.com>
Reading the step for the finalize span had grown into a second parser for
tmp_step_N, and pointing _rename_checkpoint at it changed how a checkpoint
gets published: a name the regex rejected raised instead of publishing
under whatever sat between the second and third underscore. That is a
behaviour change to the checkpoint path in a telemetry PR, which is not
where a reviewer looks for one.

Drop the new parser. _rename_checkpoint is byte-identical to main again,
and begin_finalization reads the step the same way it does, guarded and
dropped to None on anything unexpected because the value only labels a
span. The pre-existing quirk -- tmp_step_final publishes as step_final --
is left alone for a checkpoint-scoped change to fix.

Signed-off-by: Raj Singh <rajsin@nvidia.com>
Every raylet publishes ray_node_mem_used / ray_node_mem_total on its own
Prometheus endpoint, and RayGpuMonitorLogger already reads them for W&B.
Nothing carried them into OTel, so a run's telemetry had GPU-side memory
and no host-side memory at all.

HostMemoryMonitor scrapes the same endpoints and records
rl.host.memory.used / rl.host.memory.total (bytes, not the GB the W&B
path reports), one point per live node.

Scraped separately rather than teed out of the GPU monitor: that path
flattens the node's identity into the metric name (node.3.mem_gb) and
discards the hostname, and it is gated on logger.monitor_gpus, which
would make an OTel series disappear when a W&B feature is turned off.

host.name is a metric attribute here rather than the resource attribute
of the same name, because the driver reports about hosts other than its
own. rl.worker_groups names the groups resident on the host, resolved
through placement_group_table as each RayWorkerGroup is built. Plural
and comma-joined on purpose: it labels whole-host memory, so it
describes the box rather than partitioning the value.

The monitor is owned by init_telemetry_driver / shutdown_telemetry
rather than by Logger. That placement is load-bearing: the group-to-node
map is a module global, visible only to the process that wrote it, and
RayWorkerGroup writes it from the driver. Single-controller GRPO and SFT
v2 build their worker groups on the driver but their Logger inside the
controller actor, so a Logger-owned monitor would read an empty map and
label every host as holding no group -- right numbers, wrong label, on
every point.

Also drops a stale assertion left in test_swaps_when_target_exists by
the checkpoint-rename revert in 5551a17, where it is vacuously true.

Signed-off-by: Raj Singh <rajsin@nvidia.com>
Thirteen review threads, grouped by what they change.

Behaviour. parse_memory_samples ran outside the scrape's try, which only
caught requests.RequestException; prometheus_client raises ValueError on
a body that is not the exposition format, so one raylet serving an HTML
error page aborted the whole sweep and left every host's point frozen at
the previous snapshot. It is now guarded on its own, and a bad page costs
only its own point. rl.checkpoint.async_save is gone: all twelve
begin_finalization callers pass wait_fn whether or not async save is on,
so the attribute read true on every checkpoint, sync saves included.

Reasons that were wrong. The observable-gauge rationale claimed the SDK
re-exports a synchronous gauge's last value forever; on the locked 1.44.0
it drops the value at each collect instead, so the cycles between two
samples carry no point at all, and only from 1.45 does it hold the last
value per attribute set. Both make an observable gauge the right choice,
for different reasons, and the comment, metrics.md and the RecordedMetric
docstring now say so. The nv.dl.topology.size.* rename was justified by
lens typing resource_attributes, which it does not -- it merges the
caller's as-is, so the name is a convention. iteration_scope was said to
win against an explicit value because lens skips keys already present;
lens starts the span before applying the caller's attributes, so they
overwrite. The deferred import named nemo_rl.utils.logger as the
importer; it is worker_groups.py, at module scope.

Claims that were too broad. setup.py said only the driver builds worker
groups, but RewardModelEnvironment builds its Policy inside a Ray actor,
so its hosts report rl.worker_groups="none" -- documented rather than
fixed, since completing the label means a named actor and a round trip
per sample. span-groups.md said every algorithm opens iteration_scope;
four loops do not. index.md said every entry point initialises telemetry;
six do not. extending.md told contributors to import rl.* names from
lens, reversing main's rule that RL names its own attributes, so
rl.algorithm is RL's constant again and the rule is back.

Tests. register_worker_group_placement had no coverage despite every
RayWorkerGroup calling it: one table lookup per placement group, a bundle
index the table does not place, and the never-raises guard. The lens
import guards could never skip, since the telemetry conftest imports lens
for every test in the directory, and a handle that comes up not exporting
is the regression the driver test exists to catch rather than a reason to
skip it. The fixture also unfreezes the metric vocabulary it froze.

FALLBACK_COLLECTION_INTERVAL is no longer any parameter's default: it is
reached only when the config carries no logger block at all, so a run
that does configure a cadence cannot silently fall back to it. Read from
GPUMonitoringConfig instead would drag mlflow and wandb into telemetry
startup for one float.

Signed-off-by: Raj Singh <rajsin@nvidia.com>
Five more review threads, none of them behavioural except the parse.

parse_memory_samples ran prometheus_client over the whole raylet page to
keep two series. Dropping the lines that cannot match first is the same
result for ~28x less work: 143 ms to 5 ms on a synthetic 48k-line page in
this venv. The filter is a prefix test, so it keeps near misses like
ray_node_mem_used_percent, which the name lookup then drops -- that
series is now in the test fixture so the two steps stay honest together.

Names typed twice. Ray's own series names were spelled here and again in
RayGpuMonitorLogger, where renaming one side for a Ray bump would leave
this module matching nothing and its series silently empty; they are
RAY_NODE_MEM_USED / RAY_NODE_MEM_TOTAL now, imported by logger.py. The
same for NRL_WORKER_GROUP, which worker_groups.py exports and telemetry
reads back, and for the two efficiency keys algorithms/utils.py writes
and metrics.py reads: all three are public in vocabulary.py, which
imports nothing from nemo_rl and so cannot close a cycle. "init/total"
inside WALL_CLOCK_EFFICIENCY_CATEGORIES stays a literal on purpose: that
list is the canonical declaration, and the telemetry suite parses it as
string literals to check the constants that restate it.

Comments. Eight blocks of five or more lines inside function bodies, cut
to the line that says what breaks: the design reasoning is in metrics.md
and in the docstrings, and a call-site paragraph only drifts from it.

configuration.md now says the software versions are per process, so
transformer_engine missing from a driver, controller or vLLM span does
not mean the run had none, and that SGLang, TRT-LLM and Dynamo workers
report no versions at all.

Signed-off-by: Raj Singh <rajsin@nvidia.com>
_CONFIG_SECTION_BY_ALGORITHM existed for one entry: sft_v2 reports itself
as sft_v2 while reading master_config.sft, so looking its settings up by
algorithm name found nothing. One map entry is cheap; a table of which
section each entry point lays its config out under is not, because every
new entry point has to remember to extend it and nothing fails if it does
not.

Gone, along with _config_section and the test that pinned the mapping.
The lookup is by algorithm name now, so an algorithm whose settings live
under another section reports no training target: _dig returns None on
the missing section and the isinstance check drops it. Checked that this
is the only effect -- sft_v2 omits nv.dl.training.target.train_iters, and
sft, grpo, dpo and rm are unchanged, rm still omitting its -1. Neither
rl.algorithm nor the campaign stage reads the section.

Not replaced with a config_section argument from run_sft_v2.py: that is
API for an attribute that is a placeholder cap in several configs anyway.

Signed-off-by: Raj Singh <rajsin@nvidia.com>
Reading the cadence as `interval if interval is not None` let a configured
`collection_interval: 0` through, where the previous `interval or ...`
had made it fall back. At zero the loop scrapes every raylet as fast as
HTTP allows: the new test takes 14s and spins without the guard.

Refused rather than clamped. Clamping reports a cadence the config did not
ask for, which is harder to notice than no series at all, and the check
sits in start_host_memory_monitoring rather than the constructor so the
documented never-raises contract holds -- the constructor runs before that
function's guard. No config ships a zero today; the GPU monitor spins the
same way on one, which is its own fix.

The telemetry conftest also resets the metric vocabulary's freeze now,
beside the other process globals it already clears. Any test that brings
telemetry up closes the vocabulary for the rest of the process, and a
module whose rows are declared by an import inside a later test then
raises on registering them -- surfacing as a missing series in whichever
test runs next rather than as leaked state. Moved there from the
host-memory fixture, which was the narrower half of the same fix.

Signed-off-by: Raj Singh <rajsin@nvidia.com>
Three review follow-ups, all comment or prose.

nv.dl.training.target.train_iters is the configured cap, not a
prediction: grpo_math_1B ships max_num_steps: 1000000 with
max_num_epochs: 1 and stops on epochs long before the cap, so the row
says so rather than leaving 1,000,000 to be read as a run length. What to
report instead is a follow-up.

The parse guard added back a seven-line comment right after the eight
long ones came out; the reasoning is already in the docstring of
test_an_unparsable_page_costs_only_its_own_point, so two lines do here.

The assert in the driver test justified itself by "the lens guard at the
top of this test", which this branch removed two commits earlier. It
asserts because lens is a core dependency.

Signed-off-by: Raj Singh <rajsin@nvidia.com>
…onale

The autouse fixture in this package's conftest imports nemo.lens
unconditionally, so a missing lens errors in fixture setup before any
pytest.importorskip in a test body can skip. Remove the three this branch
added, and the module-level _HAS_LENS skipif in test_checkpoint_spans.py,
importing lens at the top instead.

test_parallelism_sizes_use_the_resource_scoped_spelling credited
DL_RESOURCE_TYPES and normalize_resource_attributes for the choice of
spelling. normalize_resource_attributes runs only over the attributes lens
derives from OTEL_RESOURCE_ATTRIBUTES, so the resource_attributes handed in
from setup.py are merged untouched and either spelling would arrive intact.
setup.py was corrected already; this test was missed. Reword it to the
convention reason and assert the key is reported rather than asserting
against the converter registry.

Signed-off-by: Raj Singh <rajsin@nvidia.com>
ray.sub (head and workers) and init_ray() no longer unset the
PMI/PMIX/MPI/OMPI/SLURM variables inherited from the srun launcher. The
TRT-LLM generation worker instead sets OMPI_ALLOW_RUN_AS_ROOT(_CONFIRM) and
PRTE_ALLOW_RUN_AS_ROOT(_CONFIRM) unconditionally, since the container runs
as root and Open MPI / PRTE refuse to start as root otherwise.

grpo-qwen2.5-0.5b-1n4g-megatron-trtllm-noncolocated-async runs its 30 steps
with the launcher's SLURM_/OMPI_MCA_/PRTE_MCA_ variables left in place.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: shuyixiong <219646547+shuyixiong@users.noreply.github.com>
(cherry picked from commit 546081f)
The Alloy sidecar is sourced early so it inherits SLURM_* before the
MPI/PMIx/SLURM scrub that used to follow. The preceding commit removed
that scrub from both roles, so the ordering the comment justifies no
longer has anything to order against.

Signed-off-by: Raj Singh <rajsin@nvidia.com>
@terrykong

Copy link
Copy Markdown
Collaborator

/ok to test bc2dbec

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI:L0 Run doctests and unit tests Documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants