Repository navigation
Add maestro framework - #2702
x41lakazam wants to merge 7 commits into
Conversation
|
Hi @x41lakazam! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Hey @jansel @williamwen42 @bobrenjc93, is there any missing step to move this PR to review ? |
…indings pin pip install --no-build-isolation skips installing [build-system].requires, so hatchling (maestro's build backend) was never present and the build failed with BackendUnavailable. Also override the base image's PIP_CONSTRAINT pin on cuda-bindings/cuda-python for the maestro install, since cupti-python needs newer versions than that pin allows. Verified by building deploy/Dockerfile end to end against nvcr.io/nvidia/pytorch:25.06-py3 on swx-ucc-build01; pip install completes and `pip show maestro` confirms a clean install. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Hi @d4l3k @dcci @bobrenjc93 |
|
@d4l3k @dcci @bobrenjc93 could you please review? |
Brings userbenchmark/maestro/maestro up to date with the nccl-multi-test maestro engine, now made sync-ready on the refractor-for-pytorch branch: - core/orchestrator.py: generalize NCCL mem-pool registration to multiple axis process groups with explicit deregistration on destroy(); drop the is_root()-gated result collection so run_all_patterns() returns the full DataFrame on every rank. - core/axis.py, core/config_parser.py: per-axis environment variables via a new `envs` config key, threaded through to Axis/TorchAxis. - core/utils/distributed.py: add dist_breakpoint(); drop the standalone is_root() helper (inlined at call sites). - core/profilers/cupti.py: drop dead commented-out ActivityKind lines. - main.py, __main__.py: split run() into a thin CLI command plus _run_benchmark() returning the results DataFrame; switch to a relative package import. Also drops Megatron support to match the upstream engine, which removed it on the same branch: deletes blocks/megatron/, the megatron-core dependency, the Dockerfile's Megatron-LM install step, the megatron_moe_layer.yaml example, and the related doc mentions. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…overrides Pulled README.md and docs/doc.md content updates from nccl-multi-test/maestro (source of truth), and bumped pyproject.toml version/description to match. Re-applied this repo's layout-specific overrides on top: README.md keeps the docs/doc.md link and run.py usage snippet, pyproject.toml keeps the build-system/wheel packaging config needed for this repo's maestro/ layout. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
d4l3k
left a comment
There was a problem hiding this comment.
Note
This review was generated by an AI agent (Claude) using PyTorch's pr-review skill against head b1ab47c, and posted at the request of @d4l3k. Findings were checked against the source but not run on GPUs; please verify before acting on them.
PR Review: #2702
Summary
Adds userbenchmark/maestro, a YAML-driven benchmark that runs collectives/GEMMs concurrently on separate process groups and reports per-block latency/BW from CUPTI. No REVIEW.md applies. Several measurement-correctness bugs mean the reported numbers are wrong (warmup included, GEMM shapes halved, start gate no-op, aliased buffers), the TorchBench entrypoint as documented doesn't run, and rank-local errors can hang the job.
Code Quality
core/orchestrator.py:118-124,core/profilers/base.py:92:profiler.reset()only clearsself._results; CUPTI buffers (10MB,cupti.py:40) are only delivered onactivity_flush_allin_exit. Warmup activities are therefore still buffered at reset and come back with the measured ones._measure_blocks_performancesorts bystart_nsandpop(0)s, so it consumes the warmup iterations first; withwarmup_iters >= itersthe report is 100% warmup. Leftover activities are dropped silently. Fix:cupti.activity_flush_all(1)before clearing inreset()(or separate profiler contexts), and assert every mapped stream's activity list is empty after consumption.core/orchestrator.py:396-400: axeswait_event(start_event)beforestart_eventis ever recorded (recorded at :427, after enqueue).torch.cuda.Eventis lazily created andCUDAEvent::block()is a no-op when!is_created_, so nothing gates the launch; blocks start in CPU enqueue order and inter-axis start skew biases overlap/shared-BW. Gate on the GPU before enqueue (e.g.torch.cuda._sleepon a gate stream, record, then have axes wait).core/config_parser.py:134,138: GEMM dims go throughparse_size(dim) // dtype_size, somat_a_shape: [4096, 4096]becomes 2048x2048 (8x fewer FLOPs). Dimensions are element counts; don't divide.ops_presets/moe.py:56,67,75emits element counts that are then halved the same way, and ignores its declaredk/expert_capacity_factor.core/block.py:197-198, 246-247,core/orchestrator.py:438-456: every block slices the singlemainpool from offset 0. Within a block src/dst overlap (all_gather_into_tensorsrc=dst[:n/team]is only NCCL's valid in-place layout on team rank 0;reduce_scatter_tensorlikewise;all_to_all_single/send-recv ring use identical src/dst). Across blocks, concurrent blocks on different axes and GEMM operands read/write the same memory simultaneously. Size the pool as the sum of per-block regions and give each block disjoint src/dst (optionally an explicit, correctly-offset in-place mode).core/orchestrator.py:263-268, 312-333: per block,start_nsis min over ranks andduration_nsmax over ranks, then overlap usesstart + duration— start and duration come from different ranks, and CUPTI timestamps are per-host clocks not comparable across nodes. Compute overlap per rank from that rank's own start/end, then aggregate.core/orchestrator.py:303-304: shared BW divides bymax(duration_i)rather than the first-start-to-last-end span, so fully serialized blocks still report ~sum of individual BWs.core/orchestrator.py:205-217, 362-392: block↔activity mapping assumes each block launches exactly the same number of activities per stream as in one isolated run; any variation (cuBLAS split-K, NCCL memcpy-based paths) silently misattributes activities to other blocks/iterations. Only underflow is detected.core/orchestrator.py:145-148:latency_precision(documented indocs/doc.md:61, plumbed throughmain.py) is never used; output is hardcoded:.1fms, so anything under 50µs prints0.0.core/config_parser.py:31-47,core/axis.py:65: uneven teams (e.g.[[0,1,2],[3]]) are accepted, butteam_size()returnslen(groups[0]), giving wrong buffer sizes/BW factors on other teams. Require equal team sizes or uselen(my_group_ranks).blocks/torch/comm.pyTorchReduceScatter/TorchAllToAll: no divisibility check onvector_sizevs team size (TorchAllGatherhas one); failure surfaces as a raw torch shape error at first run.core/block.py:101:_enrich_api_params_schemais never called, soapi_params_schema/defaults are dead and missing keys raise bareKeyErrorfrom_prepare_tensors_pool, outsidemain.py's config error handling. The "Validate - all the axes referenced in the blocks exist" comment atconfig_parser.py:140has no code.core/utils/format.pyparse_size: accepts negatives (-4M→[:-N]slicing), rejects1.5M/1MiB, and uses 1024-based units while the table header says "Size (B)" and BW uses 1e9.core/orchestrator.py:438,core/block.py:195,242: dtype hardcoded fp16 (fallback path allocates fp32); pool initialized withtorch.ones, so repeated in-place fp16 sum reductions overflow to inf within the warmup, and (due to aliasing) GEMM operands become inf/NaN.core/profilers/torch.py:TorchProfiler.__init__raisesNotImplementedErrorandmain.pyonly acceptscupti; remove it.core/axis.py:30-31:_temporary_environclears and rebuilds all ofos.environwhile process-group background threads may callgetenv; save/restore only the keys inenvs.core/axis.py:53-60: a rank not in any team leavesmy_group_ranksunset →AttributeErrorinstead of the intended error.core/utils/distributed.py:184, 220-226:traffic_classhandling is unreachable (create_axisis called withoutparams).core/orchestrator.py:94: docstring says non-root ranks returnNone; every rank returns the full DataFrame, makingrun.py:25dead.
Infrastructure
run.py:15,18,README.md:11:python run_benchmark.py maestro-benchmark <config.yaml>fails —run_benchmark.pydispatches on the directory name (maestro). There's also no mention that it must be launched undertorchrun/srun.maestro/maestro/__init__.py:8-10:core/,core/utils/,blocks/torch/lack__init__.pyand internal modules import top-levelcore,blocks,main. A namespace package loses to any regularcore/blocksmodule anywhere onsys.pathregardless of insertion order, and the permanentsys.pathinsert leaks these generic names to the whole process. Additionallysetup.py'sfind_packages(include=["torchbenchmark*","userbenchmark*"])skips the namespace dirs, so a non-editablepip install .of TorchBench ships an unimportable maestro. Use package-relative imports, add the__init__.pyfiles, drop thesys.pathhack.run.py:28,37: metrics JSON puts everything under a new top-levelresultskey with an emptymetricsdict;ADDING_USERBENCHMARKS.mdand TorchBench consumers (e.g.abtest.py) only readmetrics: {name: float}. Flatten to e.g.f"{pattern}-{block}-avg_latency_ms".- No
install.py/requirements entry forclick,cupti-python;pyproject.tomlis not used on the TorchBench path, andrun_benchmark.py'sexcept ImportErrorturns the resulting failures into exit 0. deploy/Dockerfile:1,7,13:git clone <REPO>is a shell redirect and fails; cloning pytorch/benchmark wouldpip installTorchBench'ssetup.py, not maestro; emptyPYTORCH_VERSIONdefault yields an invalid image ref;PIP_CONSTRAINT=drops NGC constraints for all deps. UseCOPYand installuserbenchmark/maestro.examples/execution_scripts/slurm.sbatch:82: runs/PATH/TO/maestro/src/main.py(nosrc/exists) and bypasses TorchBench;--results-diris validated but unused;SLURM_GPUS_PER_NODEis unset/typed under--gres;$CONFIG_FILEunquoted insidebash -c.
Examples
examples/config_files/collectives_1D/*.yaml:save_results_to: "/path/to/results/..."fails at parse time withPermissionError(not inmain.py's caught set);all_reduce_sweep.yaml/reduce_sweep.yamlrequire 16 ranks without saying so;reduce_sweep.yamliters: 3makes P99 meaningless;ag_rs.yamlis named "AG+RS (256M)" but its all-gather is 4M.
Backward Compatibility
pyproject.toml:15:torch>=2.6, butWork.block_current_stream(used every_execute,core/utils/async_work.py:22) only exists from 2.9, andbackend.mem_allocator(register_buffers) from 2.7. On <2.9 every pattern raisesAttributeErrorand is skipped. Requiretorch>=2.9.pyproject.toml:12,14: exact pinsclick==8.3.0,pandas==2.3.3will force downgrades/upgrades in a TorchBench env; use lower bounds.
Thread Safety
core/orchestrator.py:81-86(stop_on_errordefaults false,config_parser.py:226): an exception on a subset of ranks (per-rank CUPTI capture failures at :376-383 / :198-215, OOM, the uneven-team case above) makes that rank skip to the next pattern and enter the world barrier/all_gather_objectwhile peers are still in axis collectives — collective mismatch → hang until the NCCL watchdog or corrupt results. Make per-rank checks reach a common status collective and raise on all ranks together; don't continue after a block has issued collectives.core/utils/distributed.py:15-21:SLURM_PROCID/SLURM_NTASKStake precedence overRANK/WORLD_SIZE, sosrun ... torchrungives every local worker the node index as rank → duplicate ranks/hang. PreferRANKwhen set.:147also selects the device byglobal_rank % device_count; useLOCAL_RANK/SLURM_LOCALID.blocks/torch/comm.py:95:batch_isend_irecvworks are discarded, soaxis.streamnever depends on the P2P kernels; the per-iterationwait_stream+ barrier fence (orchestrator.py:415-424) doesn't cover the send-recv ring and iterations drift across barriers. Wait the works insideuse_axis().core/axis.py:18-31: per-axisenvsare applied only aroundnew_group. MostNCCL_*knobs areNCCL_PARAMs cached process-wide on first read (during world init), so per-axis values are ignored, anduse_axisnever applies them despite theorchestrator.py:48docstring. Document/validate which vars can vary per axis or useProcessGroupNCCL.Options().configviapg_options.main.py:139-152,run.py:38,config_parser.py:14: every rank writessave_results_toanddump_output's second-resolutionmetrics-<ts>.jsonto the same paths;%Dis computed per rank at import with minute resolution. Write from rank 0 only and broadcast the timestamp.
API Design
main.py:95-101: the exportedrun_benchmarkcallssys.exit(1)on config errors and logs apython main.py run -cusage line; raise instead.main.py:145-147:to_json(path, index=False)with default orient writes the columns/index-keyed shape (different fromrun.py'sorient="records");to_excelneedsopenpyxl, which isn't a dependency, and fails after the benchmark completes.maestro/__init__.py:12-21: version parsed from../pyproject.tomlis0.0.0+unknownafter a wheel install (the Dockerfile path). Useimportlib.metadata.- Docs:
docs/doc.md/developer.mdreferencesrc/...paths,python main.py,uv sync/uv.lock,src/blocks/torch/__init__.py— none exist;main.pydocstrings uselist_blocks(click exposeslist-blocks); README title still "NCCL Multi Test";doc.md:194says preset name is a suffix (it's a prefix).
Recommendation
Request Changes
The reported metrics are currently wrong (warmup contamination, halved GEMMs, non-functional start gate, aliased buffers), the documented TorchBench invocation and non-editable install don't work, and rank-local errors can deadlock. Separately, the head commit ("Sync maestro docs/pyproject from nccl-multi-test") indicates the code is mirrored from an external upstream — the ownership/sync model should be settled before landing.
|
@d4l3k addressed. |
|
@d4l3k The CI fail look unrelated to maestro |
|
@d4l3k Can we merge? |
Maestro is a Pytorch-based benchmarking framework for overlapping communication/compute operations in distributed environments.
This presents a more accurate way to measure performance of AI patterns compared to the micro-benchmarking which measures performance of standalone operations.
It allows a user to define a workload pattern, then benchmark performance of every running element.
Example use-case of an overlapping benchmarking suite
As an example, one user would want to see how well AllGather and ReduceScatter overlap given a certain fabric and algorithms, maestro allow him to easily define it as a pattern running the two and measure the performance of each while they are overlapping, he would get such a result:
And therefore be able to see the impact of tuning each part of the system on the collectives, and optimize it.
Quick description of maestro capabilities
Maestro benchmark patterns, which are composed of blocks, i.e reusable unit of works that enqueue a GPU activity, and axes, i.e a collection of GPU teams, the user defines which blocks should run on which axis, maestro run the full pattern and report the performance of each block, separately.
An axis is representing the way of splitting the ranks in a classic parallelized AI workload, for example:

Who benefits from maestro
Use cases
Below is a few use cases that leveraged maestro for optimization: