fix(profiling): make TraceLens and dynolog survive real GPU traces - #172
Open
Cemberk wants to merge 1 commit into
Open
fix(profiling): make TraceLens and dynolog survive real GPU traces#172Cemberk wants to merge 1 commit into
Cemberk wants to merge 1 commit into
Conversation
Everything here came out of running the tools on an AMD GPU node against real rocprofv3 and dynolog captures. None of it is reachable without hardware, which is why the no-GPU test suite went green over broken code. - rocprofv3 copies HIP API `const char *` arguments into its JSON verbatim, so an argument that does not point at a string leaves raw bytes behind. TraceLens loads traces with orjson, which rejects the whole document, so twelve stray bytes cost the entire report on a 295 MB trace. The analyzer now detects undecodable bytes incrementally and analyzes a sanitized copy, leaving the original trace untouched. - `dyno gputrace` has no `--fail-on-no-process` flag in the pinned v0.5.0, and exits 0 whether or not it matched a process. Every attempt therefore looked like "no PyTorch process registered yet" and the trigger retried a request dyno had refused, twelve times. It now reads `processesMatched` out of the response, and gives up immediately when dyno rejected the request rather than when the workload is merely slow to register. `dynolog_stop.sh` reports that outcome instead of blaming the workload. - The multi-rank collective report globbed the whole run directory and assumed every PyTorch trace carried its rank in its filename. Traces captured on demand are named after the process id, so TraceLens matched three files and then rejected all of them. The report is now built only from rank-labelled traces, from a glob scoped to their own directory, and is skipped with an explanation when no trace carries a rank. - `_build_command` fell back to searching PATH for TraceLens console scripts, which defeats the isolation `--python` exists to provide: TraceLens pins protobuf and xprof, and is installed in a venv of its own. - `pip install -e .` failed outright on this branch: the `tracelens` extra installs from git, and hatchling rejects direct references unless they are allowed explicitly. Co-authored-by: Cursor <cursoragent@cursor.com>
Cemberk
requested review from
Rohan138,
coketaste,
gargrahul and
leconcio
as code owners
August 13, 2026 01:23
There was a problem hiding this comment.
Pull request overview
This PR hardens the profiling toolchain (TraceLens analyzer + dynolog integration) to behave correctly on real AMD GPU nodes, addressing invalid UTF-8 rocprofv3 JSON traces, dynolog request/response handling, and multi-rank collective report trace selection. It also updates packaging metadata so the tracelens extra can be installed from git under hatchling.
Changes:
- Add incremental UTF-8 validation and “sanitized copy” analysis for rocprofv3 / PyTorch JSON traces, and keep
--pythonisolation by avoiding PATH fallback. - Fix dynolog trigger semantics by interpreting
processesMatchedfrom output (exit code is not meaningful) and failing fast on rejected requests; improve stop-script diagnostics. - Restrict multi-rank collective report inputs to rank-labelled traces and add regression tests; enable hatch direct references for
pip install -e ..
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_tracelens_analyze.py | Updates collective-args expectations and adds coverage for skipping collective reports when ranks are not identifiable. |
| tests/fixtures/dummy_tracelens/TraceLens/Reporting/_dummy.py | Makes the dummy TraceLens fail on invalid UTF-8 JSON to emulate orjson/TraceLens behavior. |
| tests/e2e/test_tracelens_dummy_pipeline.py | Adds e2e regression tests for analyzing traces containing undecodable bytes via sanitized copies. |
| tests/e2e/test_dynolog_dummy_pipeline.py | Expands the dyno stub contract and adds tests for fail-fast behavior on rejected requests and accurate reporting. |
| src/madengine/scripts/common/tools/tracelens_analyze.py | Implements incremental UTF-8 detection + sanitization, improves collective report trace selection/globbing, and enforces --python environment isolation. |
| src/madengine/scripts/common/tools/dynolog_trigger.sh | Switches from non-existent flag semantics to parsing processesMatched, and fails fast when dyno rejects the request. |
| src/madengine/scripts/common/post_scripts/dynolog_stop.sh | Improves post-run messaging to distinguish “no process matched” vs “request rejected”. |
| pyproject.toml | Enables hatchling allow-direct-references to support git-based dependencies in extras. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+310
to
+313
| if workspace[0] is None: | ||
| workspace[0] = tempfile.mkdtemp(prefix="madengine-tracelens-") | ||
| destination = os.path.join(workspace[0], os.path.basename(trace)) | ||
| print( |
Comment on lines
+556
to
+564
| elif mode in ("auto", "collective") and len(pytorch_traces) > 1: | ||
| unrankable.append( | ||
| ( | ||
| f"{len(pytorch_traces)} PyTorch traces", | ||
| "the collective report needs the rank in each trace's filename, " | ||
| "and none of these carry one. Traces captured on demand through " | ||
| "dynolog are named after the process id.", | ||
| ) | ||
| ) |
5 tasks
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.
Summary
#170 proved the TraceLens and dynolog tools work without a GPU. Running them on
an actual AMD GPU node against real rocprofv3 and dynolog captures proved they
did not work with one. This is the fallout.
const char *arguments into its JSON verbatim, so an argument that does notpoint at a string leaves raw bytes behind. TraceLens loads traces with orjson,
which rejects the whole document, so twelve stray bytes cost the entire report
on a 295 MB trace. The analyzer now detects undecodable bytes incrementally
(no 295 MB read into memory) and analyzes a sanitized copy, leaving the
original trace untouched.
dyno gputracehas no--fail-on-no-processin the pinned v0.5.0, and exits 0whether or not it matched a process. Every attempt therefore looked like "no
PyTorch process registered yet", and the trigger retried a request dyno had
refused, twelve times, before giving up with a misleading diagnosis. It now
reads
processesMatchedout of the response, and stops immediately when dynorejected the request rather than when the workload is merely slow to register.
globbed the whole run directory and assumed every PyTorch trace carried its
rank in its filename. Traces captured on demand are named after the process
id, so TraceLens matched three files and rejected all of them. The report is
now built only from rank-labelled traces, from a glob scoped to their own
directory, and is skipped with an explanation when no trace carries a rank.
--pythonwas not actually isolating anything._build_commandfell backto searching PATH for TraceLens console scripts, which defeats the isolation
that flag exists to provide: TraceLens pins protobuf and xprof and lives in a
venv of its own.
pip install -e .failed outright on this branch. Thetracelensextrainstalls from git, and hatchling rejects direct references unless they are
allowed explicitly. Nobody could install the branch to try any of the above.
Test plan
Every item above has a regression test that fails without its fix and needs no
GPU, so CI covers them. On the GPU node (
banff-cyxtera-s83-5):tests/e2e/test_tracelens_dummy_pipeline.py+test_dynolog_dummy_pipeline.py— 35 passedtests/unit/test_tracelens_analyze.py+test_tracelens_report.py+tests/integration/test_tracelens_tools_config.py— 66 passedtests/e2e/test_tracelens_workflows.py(GPU-gated) — 9 passed, 2 skippedrocprofv3_lightweight+tracelensrun end to end: the analyzerreported the trace as not valid UTF-8, sanitized it, and produced a
SUCCESSrow from the realTraceLens_generate_perf_report_rocproftorch_profiler_dynologcapture, then TraceLens on the resultingKineto trace
tracelens or dynolog or profilsurface: 127 passed. The threetest_profiling_workflows.pyfailures (rpd,gpu_info_power,gpu_info_vram) are pre-existing and unrelated — they fail identically atec4de0b, the commit before any TraceLens work.rpddies inside thecontainer on a missing
librocm_sysdeps_sqlite3.so, and thegpu_infotools fail in
gpu_info_pre.sh.Note
Stacked on #170, so it targets
cem/tracelens-dummy-tests.