Repository navigation
Instrument Megatron-LM with OpenTelemetry traces and metrics - #6438
Conversation
|
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. |
There was a problem hiding this comment.
This file is pretty much deprecated, changes should go in megatron/core/inference/text_generation_server/dynamic_text_gen_server/text_generation_server.py
There was a problem hiding this comment.
Good catch — reverted text_generation_server.py to main in f521a8e, so this PR no longer touches the deprecated module at all.
Porting the GenAI-semconv spans and metrics to dynamic_text_gen_server/text_generation_server.py will come as a follow-up rather than being reworked here, to keep this PR to the training-loop instrumentation.
ahmadki
left a comment
There was a problem hiding this comment.
The entire test suit should be dropped, these tests belong to Lens - some of them are even direct copies
|
/claude review |
| default=None, | ||
| help='Comma-separated span-group spec controlling which OTel ' | ||
| 'instrumentation boundaries are active. Accepts preset keywords ' | ||
| '("default", "per_step", "full", "all") or individual group names ' |
There was a problem hiding this comment.
"full" isn't a valid preset — MegatronSpanGroup._PRESETS defines default, per_step, profiling, all.
| '("default", "per_step", "full", "all") or individual group names ' | |
| '("default", "per_step", "profiling", "all") or individual group names ' |
| print_rank_0(f'> Transformer Engine version ... {get_te_version()}') | ||
|
|
||
| # Optional: the sbatch launch script's own timestamps, if it passed them | ||
| # through env vars (see megatron.startup.launch_script_setup/.container_load |
There was a problem hiding this comment.
Span is named megatron.startup.launch_script (training.py:497), not launch_script_setup — the comment 30 lines below (line 492) already uses the correct name.
| # through env vars (see megatron.startup.launch_script_setup/.container_load | |
| # through env vars (see megatron.startup.launch_script/.container_load |
|
Light review — mostly docs/tests. Two single-line typos left as inline suggestions; the rest below. 1. The observability docs published by this PR describe the pre-refactor span taxonomy
Affected:
2.
|
asolergi-nv
left a comment
There was a problem hiding this comment.
There are no critical issues to address, but I wonder if we should start regulating which comments belong in Python files and which ones should be in the documentation.
| log_factory = ArgumentGroupFactory(LoggerConfig, exclude = ["log_throughput_to_tensorboard", "throughput_window_size", "memory_keys", "log_l2_norm_grad_to_tensorboard", "log_runtime_to_tensorboard", "runtime_time_unit", "filter_warnings", "modules_to_filter", "set_level_for_all_loggers", "save_config_filepath"]) | ||
| group = log_factory.build_group(parser, title="logging") | ||
|
|
||
| otel_group = parser.add_argument_group(title='opentelemetry') |
There was a problem hiding this comment.
@maanug-nv This won't be directly exposed in MBridge, right? Should we include OTel in LoggerConfig?
| # OTel: module-level helpers imported once at startup. | ||
| try: | ||
| from nemo.lens.state import is_span_group_enabled as _otel_sg_enabled | ||
| from nemo.lens.helpers import managed_span as _otel_managed_span | ||
| from nemo.lens.helpers import safe_set_span_attributes as _otel_safe_set_attrs | ||
| from nemo.lens.helpers import trace_fn as _otel_trace_fn | ||
| except ImportError: | ||
| from megatron.core.telemetry.fallbacks import is_span_group_enabled as _otel_sg_enabled | ||
| from megatron.core.telemetry.fallbacks import managed_span as _otel_managed_span | ||
| from megatron.core.telemetry.fallbacks import safe_set_span_attributes as _otel_safe_set_attrs | ||
| from megatron.core.telemetry.fallbacks import trace_fn as _otel_trace_fn | ||
|
|
||
|
|
||
| # OTel: module-level state for the job-level spans -- module level rather than | ||
| # closures local to pretrain(), because train()'s own early-exit path (a few | ||
| # thousand lines away, a different function) needs to reach the same | ||
| # end-spans-then-shutdown sequence. Closures defined inside pretrain() aren't | ||
| # callable from there; these are. | ||
| # | ||
| # Structure (audit section L: NO run-long umbrellas). An incarnation emits a set of | ||
| # SEGMENTED traces rather than one tree, because a run-long root span is dead weight | ||
| # (its children live in their own traces anyway) and is LOST outright when a fault kill | ||
| # arrives before it can be ended and exported. There is deliberately no in-process | ||
| # 'workload' / 'megatron.pretrain' span: the authoritative run envelope is the | ||
| # reckoner's out-of-band slurm.job (from sacct), and every span here carries run | ||
| # identity (run_uuid / slurm.sluid / nvrx.cycle) on its RESOURCE attributes, which is | ||
| # what actually stitches the traces back together. | ||
| # | ||
| # trace A: pre_startup (top-level, fully backdated, CYCLE 0 / non-NVRx only: | ||
| # SLURM_JOB_START_TIME -> launch_script_start, i.e. the | ||
| # queue tail / prolog before our first line. Ends exactly | ||
| # where the agent's nvrx.cold_start begins, so the two | ||
| # tile instead of overlapping -- audit section K.) | ||
| # trace B: megatron.startup (own trace, fresh trace_id via an empty Context()) | ||
| # |- megatron.startup.launch_script (backdated; plain-srun path only) | ||
| # |- megatron.startup.container_load (backdated; plain-srun path only) | ||
| # |- megatron.startup.imports / .arg_parse / .inprocess_setup / | ||
| # | .in_job_setup / .initialize_megatron / .jit_fusion_options (backdated) | ||
| # `- whatever runs live before train()'s loop (weight-hash check, sniff test, | ||
| # cuda-graph setup) -- they attach to the current context | ||
| # trace C_n: megatron.train (one per checkpoint interval, each its own trace -- | ||
| # see _reroot_otel_interval; C_0 Links back to trace B, C_n Links to C_n-1) | ||
| # | ||
| # The launch_script/container_load children are only emitted when this process actually | ||
| # owns that window (the launch script passed presrun through AND we're not under NVRx, | ||
| # where nvrx.cold_start covers it -- see pretrain_hybrid.py). When they are emitted the | ||
| # startup trace is anchored at launch_script_start so they tile inside it; otherwise it | ||
| # is anchored at program_start. |
There was a problem hiding this comment.
Do we need ALL these comments & details here?
Comments used to be for people but now they are for agents. Having the documentation closer to the code seems to work better for them... |
Wires the span groups and metric instruments added in NVIDIA#6415 into the code that produces them: the training loop, checkpointing, evaluation, pipeline schedules and P2P transfers, gradient sync, activation offload, transformer and Mamba layers, the dataset builder, and the inference server. Adds the --otel-enabled, --otel-service-name, and --otel-span-groups flags, the MEGATRON_OTEL_* environment variables, telemetry setup and teardown in global_vars, and the nemo-lens dependency behind an otel extra. Spans are gated per group, so a disabled group costs a frozenset lookup and no span object. Non-exporting ranks resolve to an empty group set. Every lens import falls back to the no-ops in megatron.core.telemetry.fallbacks, so nothing here requires nemo-lens to be installed. Publishes the Observability guide by adding its docs/index.md toctree entry. This replaces NVIDIA#6385, rebased onto main after NVIDIA#6415 merged. Original changes by @rhewett-nv. Signed-off-by: Deepak Narayanan <dnarayanan@nvidia.com>
Bumps the nemo-lens pin to b85578fc in both the dev group and the otel extra,
applying the suggestions from review.
Drops tests/unit_tests/telemetry/{conftest,test_config,test_handle,test_helpers,
test_instrumentation,test_instruments,test_propagation}.py. These import
exclusively from nemo.lens and test the lens library rather than Megatron; some
are direct copies of lens's own tests, so they belong upstream. The four
telemetry test files already on main are untouched and still pass.
Removes the RandomIdGenerator monkeypatch from _set_telemetry. It reached into
opentelemetry.sdk to work around identically-seeded span IDs across
data-parallel ranks, which nemo-lens now handles in its own provider setup.
Reverts megatron/core/inference/text_generation_server/text_generation_server.py
to its state on main. That module is effectively deprecated, so the GenAI-semconv
spans and metrics belong in dynamic_text_gen_server/text_generation_server.py
and will be added separately.
Signed-off-by: Deepak Narayanan <dnarayanan@nvidia.com>
Regenerates uv.lock so it matches pyproject.toml again; without this every job that runs uv sync re-resolves from scratch and fails building mamba-ssm. Widening opentelemetry-api from ~=1.33.1 to >=1.33.1,<2 is what makes the resolution satisfiable at all. The old cap held OpenTelemetry at 1.33.x, which forced opentelemetry-exporter-otlp-proto-grpc <=1.34.1 and therefore protobuf<6.0, while nvidia-resiliency-ext 0.6.0 needs grpcio-tools>=1.76.0 and therefore protobuf>=6.31.1. The two are irreconcilable, so megatron-core[dev] and megatron-core[otel] could not be resolved together. nemo-lens itself asks only for opentelemetry-api>=1.20.0, so the cap, not nemo-lens, was the constraint. This moves the dev environment from OpenTelemetry 1.33.1 to 1.44.0. Generated with uv 0.7.2, the version CI pins in both setup-uv and Dockerfile.ci.dev, and verified there with uv lock --check and uv sync --locked --only-group linting. Signed-off-by: Deepak Narayanan <dnarayanan@nvidia.com>
The inference-server instrumentation was reverted from this PR, so the guide no longer describes anything that ships. Removes the GenAI-semconv span section and attribute table, the inference metrics section and its two Prometheus rows, the text_completion node in the span hierarchy, and the inference row in the what-gets-instrumented table. The inference span group itself stays: MegatronSpanGroup.INFERENCE exists and is part of the default preset, so it is now listed as reserved, matching how data_loading is already described. These sections come back with the port to dynamic_text_gen_server. Signed-off-by: Deepak Narayanan <dnarayanan@nvidia.com>
Moves the commit pin out of the dependency strings and into the uv.sources section, matching how every other git-sourced dependency in this file is declared. The revision now lives in one place instead of two that can drift. uv.lock is unchanged: uv records the same git source entry either way, so this is purely a declaration-style change with no effect on resolution. Verified with uv 0.7.2 in the CI container, where uv lock --check and uv sync --locked --only-group linting both pass. Signed-off-by: Deepak Narayanan <dnarayanan@nvidia.com>
da38988 to
abb7421
Compare
What does this PR do?
Wires the span groups and metric instruments added in #6415 into the code that produces them, and adds the configuration surface to turn it all on.
This replaces #6385, rebased onto current main now that #6415 has merged. Original changes by @rhewett-nv — please close #6385 in favour of this one.
What's instrumented
megatron/training/training.pymegatron/training/checkpointing.py,async_utils.pymegatron/core/pipeline_parallel/schedules.pymegatron/core/pipeline_parallel/p2p_communication.pymegatron/core/distributed/distributed_data_parallel.pymegatron/core/pipeline_parallel/fine_grained_activation_offload.pymegatron/core/transformer/transformer_layer.py,megatron/core/ssm/mamba_layer.pymegatron/core/datasets/blended_megatron_dataset_builder.pyAdds
--otel-enabled,--otel-service-name,--otel-span-groups, theMEGATRON_OTEL_*environment variables, and telemetry setup/teardown inmegatron/training/global_vars.py. Publishes the Observability guide by adding itsdocs/index.mdtoctree entry.Cost when disabled
Spans are gated per group: a disabled group costs a frozenset lookup and no span object is allocated. Non-exporting ranks resolve to an empty group set. Every
nemo.lensimport falls back to the no-ops inmegatron.core.telemetry.fallbacks, so nothing here requires nemo-lens to be installed.Changes since the first review
Thanks @ahmadki and @santhnm2 — all four review threads are addressed.
megatron/core/inference/text_generation_server/text_generation_server.pyis byte-identical to main again; this PR no longer touches that module. Per @santhnm2 it is effectively deprecated, so the GenAI-semconv spans and metrics will be ported todynamic_text_gen_server/text_generation_server.pyas a follow-up rather than reworked here.conftest.py,test_config,test_handle,test_helpers,test_instrumentation,test_instruments,test_propagation). Per @ahmadki they test the Lens library rather than Megatron and belong upstream. They were inherited verbatim from Telemetry/otel lens upstream #6385. The four telemetry test files already on main from Add megatron.core.telemetry base layer and observability guide #6415 are untouched and still pass.RandomIdGeneratormonkeypatch removed from_set_telemetry. It reached intoopentelemetry.sdkprocess-globally to work around identically-seeded span IDs across data-parallel ranks; nemo-lens handles that in its own provider setup.b85578fcin both thedevgroup and theotelextra.distributed_data_parallel.py(kept both main'sPARAM_READY_CALLBACK_ATTRimport and the lenstrace_fnfallback) andtraining.py, where main had removedtorch.cuda.empty_cache()fromsave_checkpoint_and_timein Remove torch cuda empty cache #5678 — adopted that removal rather than resurrecting the call.tools/autoformat.sh, which Telemetry/otel lens upstream #6385 never had; black reformatted 9 files and isort fixed 6.opentelemetry-apipin widened — needs a lookuv.lockhad to be regenerated for the new dependency, and doing so surfaced an unsatisfiable conflict that exists on main today:devpinnedopentelemetry-api~=1.33.1, capping OpenTelemetry at 1.33.x.opentelemetry-exporter-otlp-proto-grpc <=1.34.1, which needsprotobuf>=3.19,<6.0.nvidia-resiliency-ext==0.6.0needsgrpcio-tools>=1.76.0, which needsprotobuf>=6.31.1.megatron-core[dev]andmegatron-core[otel]could not resolve together.nemo-lens is not the constraint; it asks only for
opentelemetry-api>=1.20.0. The cap was. The conflict is invisible on main only because nothing currently pulls theotelextra.Fix:
opentelemetry-api~=1.33.1->>=1.33.1,<2. This moves every dev environment from OpenTelemetry 1.33.1 to 1.44.0, not just telemetry runs, which is the main thing worth reviewing here.Two notes on the lockfile:
setup-uv(cicd-main.yml:396) andDockerfile.ci.dev. Verified in the CI container withuv lock --check(exit 0) anduv sync --locked --only-group linting(exit 0), which is what the linting job runs.revision = 3->revision = 2. Main's lock was generated by a newer uv than CI pins, so 0.7.2 rewrites it in its own format. Pre-existing inconsistency, not introduced here.Issue tracking
Linked issue: replaces #6385
Contribution process
Pre-checks
Verified locally
--fail-on-warning(the flag the docs job uses), including the newly published Observability toctree.megatron/core.Known follow-ups
Extracting the span-lifecycle helpers out of
training.py— immediate follow-up, already planned.This PR adds 1,281 lines to
megatron/training/training.py(now 5,603 lines), and a large share of that is telemetry bookkeeping rather than training logic:_start_otel_job_spans,_end_otel_startup_span,_start_otel_train_span,_reroot_otel_interval,_maybe_reroot_otel_interval,_end_otel_interval_span,_end_otel_train_span,_end_otel_job_spans,_otel_telemetry_active,_otel_mark_goodput,_backdated_otel_span)_otel_*globals holding span handles, context tokens and idempotence flagsRoughly 400-500 lines will move to a dedicated module, leaving
training.pywith imports plus the call-site usage that should have been the only footprint all along: 21with _otel_managed_span(...), 6_otel_sg_enabled(...), and a handful of attribute stamps.Deliberately not done in this PR: it would rewrite the exact
training.pydiff reviewers have already read, and the span lifecycle has no unit-test coverage here (the four telemetry tests on main covermegatron/core/telemetry, not this), so a move touchingglobalmutation ordering and atexit/SIGTERM idempotence would be unguarded. As a standalone follow-up the diff is almost entirely a move, reviewable withgit diff -M, and can land with tests for the lifecycle.Also queued: port the GenAI-semconv inference instrumentation to
dynamic_text_gen_server/, and restore the inference sections of the Observability guide alongside it (they were removed here in f007fd2, since this PR no longer ships that instrumentation).🤖 Generated with Claude Code