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>
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.
| if workspace[0] is None: | ||
| workspace[0] = tempfile.mkdtemp(prefix="madengine-tracelens-") | ||
| destination = os.path.join(workspace[0], os.path.basename(trace)) | ||
| print( |
| 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.", | ||
| ) | ||
| ) |
|
|
||
| if workspace[0] is None: | ||
| workspace[0] = tempfile.mkdtemp(prefix="madengine-tracelens-") | ||
| destination = os.path.join(workspace[0], os.path.basename(trace)) |
There was a problem hiding this comment.
destination = os.path.join(workspace[0], os.path.basename(trace)) writes into a single shared scratch dir (workspace[0] is created once, per the docstring above) keyed only by basename. If two traces needing sanitization share a basename but live in different subdirectories -- plausible for multi-rank collective traces, which this PR elsewhere goes out of its way to handle correctly via _collective_args -- the second sanitized copy silently overwrites the first, and one rank's report gets analyzed from the wrong (or already-consumed) trace data with no error surfaced.
Suggest deriving destination from the trace's relative path (or a hash of the full path) instead of bare basename, e.g. os.path.join(workspace[0], hashlib.sha1(trace.encode()).hexdigest() + os.path.splitext(trace)[1]), and adding a regression test with two same-basename traces in different subdirectories both needing sanitization.
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.