Skip to content

fix(profiling): make TraceLens and dynolog survive real GPU traces - #172

Merged
coketaste merged 6 commits into
developfrom
cem/tracelens-gpu-fixes
Oct 2, 2026
Merged

coketaste merged 6 commits into
developfrom
cem/tracelens-gpu-fixes

Conversation

@Cemberk

@Cemberk Cemberk commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

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.

  • rocprofv3 JSON is not valid UTF-8. 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
    (no 295 MB read into memory) and analyzes a sanitized copy, leaving the
    original trace untouched.
  • The dynolog trigger was arguing with a flag that does not exist.
    dyno gputrace has no --fail-on-no-process 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, before giving up with a misleading diagnosis. It now
    reads processesMatched out of the response, and stops immediately when dyno
    rejected the request rather than when the workload is merely slow to register.
  • The multi-rank collective report matched traces it could not use. It
    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.
  • --python was not actually isolating anything. _build_command fell back
    to 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. The tracelens extra
    installs 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 passed
  • tests/unit/test_tracelens_analyze.py + test_tracelens_report.py + tests/integration/test_tracelens_tools_config.py — 66 passed
  • tests/e2e/test_tracelens_workflows.py (GPU-gated) — 9 passed, 2 skipped
  • Real rocprofv3_lightweight + tracelens run end to end: the analyzer
    reported the trace as not valid UTF-8, sanitized it, and produced a
    SUCCESS row from the real TraceLens_generate_perf_report_rocprof
  • Real torch_profiler_dynolog capture, then TraceLens on the resulting
    Kineto trace
  • The whole tracelens or dynolog or profil surface: 127 passed. The three
    test_profiling_workflows.py failures (rpd, gpu_info_power,
    gpu_info_vram) are pre-existing and unrelated — they fail identically at
    ec4de0b, the commit before any TraceLens work. rpd dies inside the
    container on a missing librocm_sysdeps_sqlite3.so, and the gpu_info
    tools fail in gpu_info_pre.sh.

Note

Stacked on #170, so it targets cem/tracelens-dummy-tests.

Copilot AI lite review requested due to automatic review settings August 13, 2026 01:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 --python isolation by avoiding PATH fallback.
  • Fix dynolog trigger semantics by interpreting processesMatched from 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 thread src/madengine/scripts/common/tools/tracelens_analyze.py Outdated
Comment thread src/madengine/scripts/common/tools/tracelens_analyze.py Outdated

@coketaste coketaste left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found a real bug while reviewing this PR (3rd in the TraceLens stack, #167 -> #170 -> #172 -> #173). See inline comment -- recommend fixing before merge since it can silently corrupt a rank's report in exactly the multi-rank scenario this PR otherwise handles carefully.

Comment thread src/madengine/scripts/common/tools/tracelens_analyze.py Outdated
Cemberk and others added 2 commits October 1, 2026 18:10
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>
…h other

Copies were keyed only by basename in a shared scratch dir, so two invalid-UTF-8
traces with the same name from different ranks silently clobbered one another.
Give each trace its own hash-named subdirectory, keeping the basename intact.

Co-Authored-By: Claude <noreply@anthropic.com>
@coketaste
coketaste force-pushed the cem/tracelens-dummy-tests branch from cc18e64 to e226bdb Compare October 1, 2026 21:10
Copilot AI lite review requested due to automatic review settings October 1, 2026 21:10
@coketaste
coketaste force-pushed the cem/tracelens-gpu-fixes branch from db8bade to 44dec7b Compare October 1, 2026 21:10

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Three moderate issues remain in collective TraceLens input handling and diagnostics.

Review effort: Lite
Findings: 2 Medium severity · 1 Low severity

Open (3)
Resolved since last review (1)

Comment thread src/madengine/scripts/common/tools/tracelens_analyze.py Outdated
Comment thread src/madengine/scripts/common/tools/tracelens_analyze.py Outdated
Copilot AI lite review requested due to automatic review settings October 2, 2026 14:57

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Collective analysis still has unresolved filtering, sanitization, and diagnostic issues.

Review effort: Lite
Findings: 2 Medium severity · 1 Low severity

Open (3)

coketaste and others added 2 commits October 2, 2026 15:28
… traces

The collective glob was derived from the common ancestor of the ranked traces,
which in a multi-node run is the whole job, so it swept up rocprof results and
other JSON. It also read the original traces, so an undecodable one still failed
the collective report after its per-rank report had been sanitized. Stage the
readable per-rank traces into one directory and point the glob at it.

Also report how many traces carry a rank when the collective report is skipped,
instead of claiming none do.

Co-Authored-By: Claude <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings October 2, 2026 15:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Defer sanitization of unranked collective traces to avoid unnecessary I/O and temporary disk usage.

Review effort: Lite
Findings: None

Resolved since last review (3)

@coketaste
coketaste changed the base branch from cem/tracelens-dummy-tests to develop October 2, 2026 16:37
@coketaste
coketaste merged commit 6ae6c1a into develop Oct 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants