Repository navigation
Bugfix: Preserve Configuration-Specific PyTorch Traces - #893
Shenghsun Cho (shcho) wants to merge 3 commits into
Conversation
Derive the benchmark configuration from the executor's benchmarks/<name>/rank<N> working directory and retain an independent configuration-qualified copy after writing the legacy trace. Preserve legacy filenames and last-write behavior, and retain legacy-only output outside the recognized executor layout. Validated on MI300X and GB200: successful configuration/rank captures remain distinct and legacy copies match the last corresponding capture. The unchanged diagnosis converter preserves old-baseline/new-target conversion results. Focused exporter checks, YAPF, and Flake8 pass.
|
Azure Pipelines: Successfully started running 1 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🟡 Changes recommended
Parallel local workers can overwrite shared profiler trace files, causing configuration-qualified traces to contain another worker’s events or incomplete JSON.
2 open issues
What changed in this PR
This PR extends PyTorch trace export to preserve configuration-specific captures while retaining legacy filenames and their existing overwrite behavior.
Changes:
- Detects the executor’s
benchmarks/<configuration>/rank<N>working-directory layout. - Copies exported traces to configuration-qualified filenames.
- Keeps legacy-only output outside the recognized layout.
| File | Description |
|---|---|
superbench/benchmarks/model_benchmarks/pytorch_base.py |
Detects configuration context and adds a separate trace copy after the legacy JSON write. |
Review effort: Balanced
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #893 +/- ##
==========================================
+ Coverage 86.31% 86.65% +0.33%
==========================================
Files 107 107
Lines 8256 8263 +7
==========================================
+ Hits 7126 7160 +34
+ Misses 1130 1103 -27
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Use the validated executor-directory rank for configuration-qualified filenames. Serialize each worker's own events directly to its qualified file so concurrent writes to the shared legacy file cannot corrupt the preserved trace. Keep legacy filenames and writes unchanged. Validation: six CUDA/HIP mocked exporter race and rank checks, five legacy compatibility checks against the pre-PR exporter, and the existing PyTorch base CPU unit test pass. One CUDA-only test was skipped. YAPF, Flake8, scoped mypy, and whitespace checks pass. No hardware captures were rerun for this revision.
Exercise the real profiler exporter with CPU-safe CUDA/HIP mocks. Cover multiple configurations and local/distributed ranks, independent legacy and qualified files, forced legacy overwrite/truncation, CPU event filtering, and legacy-only fallback. Validation: 15 tests passed and 1 CUDA-only test skipped in the existing PyTorch base test file. All seven new executable production lines are covered locally (100% patch coverage). YAPF, Flake8, scoped mypy, and whitespace checks pass. Production code is unchanged.
| json.dump(diag_agent_events, f, sort_keys=True) | ||
| if has_config_name: | ||
| config_trace_path = f'{dump_file_dir}/torch-profiler-sb-{self._name}__cfg={config_name}-{rank_dir[4:]}.json' | ||
| with open(config_trace_path, 'w') as trace_file: |
There was a problem hiding this comment.
[SHOULD-FIX] Encode and bound configuration-qualified filenames
Location: superbench/benchmarks/model_benchmarks/pytorch_base.py:659 -- PytorchBase._benchmark
Evidence:
config_trace_path = f'{dump_file_dir}/torch-profiler-sb-{self._name}__cfg={config_name}-{rank_dir[4:]}.json'
with open(config_trace_path, 'w') as trace_file:
json.dump(diag_agent_events, trace_file, sort_keys=True)Issue:
The raw configuration name is embedded directly in a new filename. Common annotated names such as model-benchmarks:resnet contain :, which Azure Files naming rules prohibit on SMB shares without character mapping. Separately, a valid 209-byte configuration name with pytorch-resnet50 produces a 256-byte filename, exceeding a common 255-byte component limit.
Impact:
The new open() fails after the benchmark and legacy export succeeded. Benchmark.run() records RUNTIME_EXCEPTION_ERROR and skips post-run model logging and postprocessing. The length failure was reproduced on the local filesystem; the prohibited-character failure was reproduced with a simulated filesystem restriction, not a live SMB mount.
Recommendation:
Encode configuration identifiers into collision-safe, portable filename components and bound the full filename by the destination filesystem limit, using a stable hash for oversized names. Add regressions for prohibited characters, multibyte names, and the exact filename-length boundary.
| with open(diag_agent_dump_file_path, 'w') as f: | ||
| json.dump(diag_agent_events, f, sort_keys=True) | ||
| if has_config_name: | ||
| config_trace_path = f'{dump_file_dir}/torch-profiler-sb-{self._name}__cfg={config_name}-{rank_dir[4:]}.json' |
There was a problem hiding this comment.
[SHOULD-FIX] Disambiguate qualified traces across nodes
Location: superbench/benchmarks/model_benchmarks/pytorch_base.py:658 -- PytorchBase._benchmark
Evidence:
config_trace_path = f'{dump_file_dir}/torch-profiler-sb-{self._name}__cfg={config_name}-{rank_dir[4:]}.json'
with open(config_trace_path, 'w') as trace_file:
json.dump(diag_agent_events, trace_file, sort_keys=True)Issue:
The executor directory rank is local to each node. In a multi-node distributed run, workers with RANK=0 and RANK=8 can both have LOCAL_RANK=0; if they use the same shared SB_TORCH_PROFILER_TRACE_DIR, their model, configuration, and rank_dir[4:] are identical.
Impact:
Both workers open the same qualified file, so one capture overwrites the other. A deterministic two-process reproduction also produced invalid JSON from overlapping qualified writes.
Recommendation:
Include a stable node identity or globally unique worker identity in the qualified output namespace while preserving the legacy filename. Add a shared-directory test with two nodes having the same local rank but different global ranks.
| benchmark_dir, rank_dir = os.path.split(os.getcwd()) | ||
| config_name = os.path.basename(benchmark_dir) | ||
| has_config_name = ( | ||
| os.path.basename(os.path.dirname(benchmark_dir)) == 'benchmarks' and rank_dir.startswith('rank') |
There was a problem hiding this comment.
[SHOULD-FIX] Preserve traces for slash-containing configuration annotations
Location: superbench/benchmarks/model_benchmarks/pytorch_base.py:634 -- PytorchBase._benchmark
Evidence:
benchmark_dir, rank_dir = os.path.split(os.getcwd())
config_name = os.path.basename(benchmark_dir)
has_config_name = (
os.path.basename(os.path.dirname(benchmark_dir)) == 'benchmarks' and rank_dir.startswith('rank')
and rank_dir[4:].isdigit()
)Issue:
The configuration schema and executor accept annotations such as model-benchmarks:experiment/fp16. The executor uses that key as a path, producing benchmarks/model-benchmarks:experiment/fp16/rank0; the new grandparent check then sees model-benchmarks:experiment rather than benchmarks and silently selects legacy-only output.
Impact:
Two such configurations of the same model and rank overwrite the shared legacy file and produce no configuration-qualified traces. This was reproduced through the pinned executor control flow and exporter.
Recommendation:
Pass the original configuration identity explicitly from the executor, and encode the complete identifier safely for filenames. Add a shared-directory test for two annotations containing /.
|
|
||
|
|
||
| @decorator.pytorch_test | ||
| @pytest.mark.parametrize('directory', ['other', 'benchmarks/config/worker0', 'benchmarks/config/rankx']) |
There was a problem hiding this comment.
[NON-BLOCKING] Test a valid rank outside the executor layout
Location: tests/benchmarks/model_benchmarks/test_pytorch_base.py:333 -- test_pytorch_profiler_legacy_fallback
Evidence:
@pytest.mark.parametrize('directory', ['other', 'benchmarks/config/worker0', 'benchmarks/config/rankx'])Issue:
All current fallback cases fail the rank-folder checks; none has a valid rank<N> folder under a parent other than benchmarks. Removing the benchmarks ancestor check still passes all existing fallback cases and all 14 committed profiler tests.
Impact:
A future regression can emit qualified traces outside the recognized executor layout without these tests detecting the behavior change.
Recommendation:
Add a fallback case such as other/config/rank0 and assert that it produces only the legacy trace.
| if has_config_name: | ||
| config_trace_path = f'{dump_file_dir}/torch-profiler-sb-{self._name}__cfg={config_name}-{rank_dir[4:]}.json' | ||
| with open(config_trace_path, 'w') as trace_file: | ||
| json.dump(diag_agent_events, trace_file, sort_keys=True) |
There was a problem hiding this comment.
[NON-BLOCKING] Avoid serializing the same event list twice
Location: superbench/benchmarks/model_benchmarks/pytorch_base.py:660 -- PytorchBase._benchmark
Evidence:
with open(diag_agent_dump_file_path, 'w') as f:
json.dump(diag_agent_events, f, sort_keys=True)
if has_config_name:
config_trace_path = f'{dump_file_dir}/torch-profiler-sb-{self._name}__cfg={config_name}-{rank_dir[4:]}.json'
with open(config_trace_path, 'w') as trace_file:
json.dump(diag_agent_events, trace_file, sort_keys=True)Issue:
When a qualified trace is enabled, the identical event list is JSON-encoded and its keys sorted twice. Writing two independent files is intentional, but the second encoding is avoidable.
Impact:
This increases post-benchmark CPU time. In a CPU-only synthetic probe with 50,000 events and output sent to /dev/null, two dumps took about 2.03 seconds versus 1.04 seconds for one streamed fan-out; these are not GPU or end-to-end timings.
Recommendation:
Encode once and stream the same encoded chunks to two independently opened outputs, or use a per-worker spool; do not copy from the shared legacy file.
Description
Preserve PyTorch traces for separate benchmark configurations without removing the legacy filenames used by existing consumers. Follow-up to #817.
Currently, configurations sharing an internal model name and local rank write the same file, so a later configuration replaces the earlier trace.
Major Revision
benchmarks/<configuration>/rank<N>before profiling.torch-profiler-sb-<model>__cfg=<configuration>-<rank>.json, including non-distributed local workers.Minor Revision
Validation
--cov=superbenchcover all 7 newly added executable production lines: 100% local patch coverage. Earlier isolated compatibility checks also confirmed legacy filename/JSON-byte equivalence against the pre-PR exporter.tests/benchmarks/model_benchmarks/test_pytorch_base.py: 15 passed, 1 CUDA-only test skipped, using Python 3.10 and PyTorch 2.4.1 with hardware-only tests disabled.--follow-imports=skip), and whitespace checks passed. The latest test-only commit leaves the production exporter unchanged.Earlier Hardware Validation
The MI300X and GB200 smoke/full-profile runs below used the earlier revision
c8030384, before the local-worker rank/direct-write fix. They were not rerun for the latest revision6ee090f9:Multiple configurations sharing the same model/rank remain distinct. All 96 qualified GB200 JSON files were checked for valid, finite timing data and positive GPU timings.
Using real historical LSTM traces and an unchanged downstream converter, old-baseline/new-target conversion produced byte-identical outputs to the legacy-only control on both platforms. New/new comparisons discovered the additional qualified pairs. The existing new-baseline/old-target missing-counterpart warnings remain unchanged.
Validation Limits
The full suites completed with some benchmark failures; this is not an all-benchmarks-pass claim. On MI300X, the 16 absent qualified files corresponded to failed captures, not lost successful exports. No LLM-generated report validation is claimed. Hardware testing covers the earlier revision only; this follow-up changes the exporter bytes and has local CPU/mocked validation, not a fresh hardware capture. The pre-existing shared legacy-file race in parallel local mode remains unchanged; this fix protects the new qualified outputs. The PR changes two existing files: the ten-line production patch and its automated tests. No new source/test files or coverage-threshold changes are introduced.