fix(profiling): stop losing TraceLens reports to two avoidable outcomes - #173
Open
Cemberk wants to merge 2 commits into
Open
fix(profiling): stop losing TraceLens reports to two avoidable outcomes#173Cemberk wants to merge 2 commits into
Cemberk wants to merge 2 commits into
Conversation
Both come from running the tools against real traces on an AMD GPU node. - `--short_kernel_study` makes TraceLens die while writing the workbook on some real traces: one of its short-kernel sheets has MultiIndex columns, and TraceLens writes every sheet with `index=False`, which pandas refuses. Every CSV is written before that, so the analysis itself was fine and only the .xlsx was lost, but the non-zero exit turned the trace into a FAILURE. madengine no longer requests the flag; it stays available through the analyzer's trailing extra args. Reported upstream as AMD-AGI/TraceLens#938. - A trace that holds no GPU activity is now SKIPPED rather than FAILURE, with a reason. dynolog configures every process that registered with it, so a torchrun job hands us the launcher's trace alongside the ranks': it only supervises children and runs no kernels. Every distributed run therefore ended with a failure row sitting next to its real report. The docs gain the two things only the GPU runs could show: an iteration-based request that lands before the workload's first optimizer step captures nothing, and an N-rank torchrun job produces N+1 traces. Co-authored-by: Cursor <cursoragent@cursor.com>
TraceLens' rocprof report writes either the CSVs or the workbook, never both, and madengine asks for the CSVs. That also makes the flag we had to drop for the PyTorch report harmless here, which is worth saying next to the arguments so nobody drops it twice. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Pull request overview
Improves the TraceLens profiling pipeline so madengine doesn’t treat “no GPU activity” traces (e.g., torchrun launcher traces captured by dynolog) as failures, and avoids losing the PyTorch workbook due to TraceLens’ --short_kernel_study Excel-writing limitation.
Changes:
- Treat TraceLens’ “No GPU events found in the trace” as a SKIPPED outcome (no output path, exit remains 0 unless real failures occurred).
- Stop requesting
--short_kernel_studyfor the PyTorch TraceLens report by default (still possible via extra args). - Extend the dummy TraceLens fixture + add unit/e2e coverage for launcher/no-GPU-event traces; update profiling docs accordingly.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_tracelens_analyze.py | Adds unit coverage for skipping no-GPU-activity traces and for omitting --short_kernel_study by default. |
| tests/fixtures/dummy_tracelens/TraceLens/Reporting/_dummy.py | Updates dummy TraceLens contract and adds GPU-activity validation for PyTorch traces. |
| tests/e2e/test_tracelens_dummy_pipeline.py | Adds an end-to-end scenario with a launcher trace that has no GPU events and must be skipped. |
| src/madengine/scripts/common/tools/tracelens_analyze.py | Implements SKIPPED handling for “no GPU events” and removes --short_kernel_study from PyTorch args. |
| docs/profiling.md | Documents dynolog/torchrun empty traces and rocprof CSV-only behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+358
to
+364
| # --short_kernel_study is deliberately not requested. On some real traces its | ||
| # sheets have MultiIndex columns, and TraceLens writes every sheet with | ||
| # `index=False`, which pandas refuses: "Writing to Excel with MultiIndex | ||
| # columns and no index ('index'=False) is not yet implemented". That loses the | ||
| # whole workbook after the CSVs are already written. Pass it back through the | ||
| # analyzer's trailing extra args if you want those sheets. | ||
| # https://github.com/AMD-AGI/TraceLens/issues/938 |
Comment on lines
+123
to
+128
| try: | ||
| with opener(path, "rt", encoding="utf-8") as handle: | ||
| payload = json.load(handle) | ||
| except (OSError, ValueError): | ||
| # Unreadable input is what the other checks are for. | ||
| return None |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #172. Two things were still costing us TraceLens reports on real MI300X traces, both found by running the pipelines on hardware rather than by reading the code.
What was going wrong
A torchrun run always reported a failure it should not have. dynolog configures every process that registered with it, and for a torchrun job that includes the launcher, which supervises its children and runs no kernels. TraceLens is asked to analyze its trace, correctly says
No GPU events found in the trace, and madengine turned that into a FAILURE row sitting next to the real report, plus a non-zero exit. Now that case is reported as SKIPPED with the reason spelled out, and no output path, since nothing was written.--short_kernel_studycould throw away the whole PyTorch workbook. On some real traces the short-kernel sheets come out with MultiIndex columns, and TraceLens writes every sheet withindex=False, which pandas refuses:Every CSV is written first, so the analysis is complete and only the workbook is lost. It is data-dependent: the same flags succeed on a duration-based capture of the same workload and fail on an iteration-based one. We no longer request the flag for the PyTorch report; it stays reachable through the analyzer's trailing extra args. Reported upstream as TraceLens#938.
What hardware showed
On MI300X (
banff-cyxtera-s83-5), one torchrun job now produces exactly the intended mix, and the run exits 0:libkineto_trace_896.json, the rank that ran kernelslibkineto_trace_728.json, the torchrun launcherThe rocprofv3 pipeline still succeeds with all six CSV sheets, including the two short-kernel ones.
One thing I got wrong, and what it taught us
I first extended the flag removal to the rocprof report on the theory that it shares the same Excel writer. It does share the code, but it writes either the CSVs or the workbook:
Since madengine asks for the CSVs, that writer is unreachable and the flag was harmless there; removing it only deleted
short_kernels_summary.csvandshort_kernel_histogram.csv. That change is not in this PR. The same either/or also means rocprof runs never produce an.xlsxeven though we pass--output_xlsx_path, which is now documented and noted next to the arguments so nobody drops the flag twice.Tests
Both behaviors are covered without a GPU, through the dummy TraceLens fixture from #172, which now validates GPU events the way the real tool does:
test_a_trace_without_gpu_activity_is_skipped_not_failed— unittest_a_launcher_trace_is_skipped_rather_than_failed— e2e, one worker trace and one launcher trace through the whole pipelinetest_no_report_requests_the_short_kernel_studyand a companion asserting it still works when asked for explicitlyTest plan