test(profiling): verify the TraceLens and dynolog tools without a GPU - #170
Open
Cemberk wants to merge 1 commit into
Open
test(profiling): verify the TraceLens and dynolog tools without a GPU#170Cemberk wants to merge 1 commit into
Cemberk wants to merge 1 commit into
Conversation
Neither tool could be exercised in CI: TraceLens pins protobuf and xprof and reports only on recorded GPU traces, and dynolog ships as a GitHub release asset and attaches to a running PyTorch workload. Stand-ins for both now let the real scripts run against fabricated trace artifacts, covering the half of the integration madengine owns -- which report generator each trace kind is routed to, the flags it receives, the retry loop that waits for the workload to register, and the guarantee that a failed analysis never fails a model run. Two defects the new tests found: The analyzer's module fallback ran main() and discarded its return value, so whenever TraceLens' console scripts were absent every failed report was recorded as a success. It now exits with that value, the way pip's console scripts do. Stopping dynolog burned its full 20s grace period and then claimed the daemon "did not stop gracefully". Both processes are reparented to PID 1 when the pre-script exits, and a container's PID 1 is the model command rather than an init that reaps children, so a terminated process lingers as a zombie that kill -0 still reports as alive. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Pull request overview
Adds CPU/CI-friendly end-to-end coverage for the TraceLens and dynolog profiling integrations by substituting dummy tool implementations, and fixes two real defects uncovered by those tests (TraceLens module fallback exit code propagation; dynolog stop behavior around zombies).
Changes:
- Add a dummy
TraceLensPython package fixture (with all expected report entry-point modules) plus a shared implementation that validates madengine’s CLI contract and writes placeholder report outputs. - Add POSIX-only e2e tests that run the real madengine analyzer/scripts against fabricated trace artifacts and dummy dynolog binaries (no GPU/Docker/network required).
- Fix
tracelens_analyze.pyto propagate exit codes when falling back to module invocation, and updatedynolog_stop.shto avoid waiting out the full grace period on zombie processes.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/fixtures/dummy_tracelens/TraceLens/Reporting/generate_perf_report_rocprof.py | Dummy entry-point module for rocprof JSON report generation. |
| tests/fixtures/dummy_tracelens/TraceLens/Reporting/generate_perf_report_pytorch.py | Dummy entry-point module for PyTorch trace report generation. |
| tests/fixtures/dummy_tracelens/TraceLens/Reporting/generate_perf_report_pftrace_memory_copy.py | Dummy entry-point module for pftrace memory copy report generation. |
| tests/fixtures/dummy_tracelens/TraceLens/Reporting/generate_perf_report_pftrace_hip_api.py | Dummy entry-point module for pftrace HIP API report generation. |
| tests/fixtures/dummy_tracelens/TraceLens/Reporting/generate_perf_report_pftrace_hip_activity.py | Dummy entry-point module for pftrace HIP activity report generation. |
| tests/fixtures/dummy_tracelens/TraceLens/Reporting/generate_multi_rank_collective_report_pytorch.py | Dummy entry-point module for multi-rank collective report generation. |
| tests/fixtures/dummy_tracelens/TraceLens/Reporting/compare_perf_reports_pytorch.py | Dummy entry-point module for report comparison workflow. |
| tests/fixtures/dummy_tracelens/TraceLens/Reporting/_dummy.py | Shared dummy TraceLens implementation: strict flag contract + report artifact writing + invocation logging. |
| tests/fixtures/dummy_tracelens/TraceLens/Reporting/init.py | Package init for dummy TraceLens reporting modules. |
| tests/fixtures/dummy_tracelens/TraceLens/init.py | Dummy TraceLens package root used via PYTHONPATH in tests. |
| tests/e2e/test_tracelens_dummy_pipeline.py | End-to-end tests covering analyzer routing, output layout, summary recording, collective report behavior, env-var config, and non-fatal failure handling. |
| tests/e2e/test_dynolog_dummy_pipeline.py | End-to-end tests covering dynolog daemon lifecycle, trigger retry behavior, request flags, and post-run diagnostics using dummy dynolog/dyno. |
| src/madengine/scripts/common/tools/tracelens_analyze.py | Fix module-fallback invocation to exit with main()’s return value (don’t swallow failures). |
| src/madengine/scripts/common/post_scripts/dynolog_stop.sh | Improve stop logic to treat zombies as stopped by checking /proc/<pid>/stat state. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+118
to
+126
| def cleanup(self) -> None: | ||
| """Kill anything left running and clear the handoff files.""" | ||
| for pid_file in (TRIGGER_PID_FILE, DYNOLOG_PID_FILE): | ||
| if pid_file.is_file(): | ||
| with contextlib.suppress(ValueError, OSError): | ||
| os.kill(int(pid_file.read_text().strip()), signal.SIGKILL) | ||
| for path in HANDOFF_FILES: | ||
| with contextlib.suppress(OSError): | ||
| path.unlink() |
6 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
Stacked on #167 and targets its branch, so merging this folds the commit into that PR.
Neither tool added in #167 could be exercised in CI: TraceLens pins
protobufandxprofand reports only on recorded GPU traces, and dynolog ships as a GitHub release asset and attaches to a running PyTorch workload. This adds stand-ins for both, so the real scripts run against fabricated trace artifacts with no GPU, no Docker, no network, and no TraceLens install.tests/fixtures/dummy_tracelens/— a stand-inTraceLenspackage placed onPYTHONPATH. Its seven report entry points declare exactly the flags madengine passes, fail loudly if one is renamed or dropped, write the report files they promise, and log every invocation.tests/e2e/test_tracelens_dummy_pipeline.py(20 tests) — drives the real analyzer, the real in-container post-script, and realmadengine report tracelens: per-kind routing, report layout, the summary CSV, the multi-rank collective report,.dbskip guidance, env-var config, and non-fatality.tests/e2e/test_dynolog_dummy_pipeline.py(11 tests) — dummydyno/dynologbinaries plus the real start/trigger/stop scripts: daemon lifecycle, the retry loop that waits for the workload to register, the request flags, and the give-up diagnostics.Two defects the new tests found, both fixed here:
main()and discarded its return value, so whenever TraceLens' console scripts were absent every failed report was recorded as SUCCESS. It now exits with that value, the way pip's console scripts do.kill -0still reports as alive. Real runs save 20s each.Substituting TraceLens is not only convenience: the real one reports only on kernels it can link back to the runtime calls that launched them, so no fabricated trace produces a report however well shaped. Real analysis stays covered by the GPU-gated tests in
test_tracelens_workflows.py.Test plan
pytest tests/e2e/test_tracelens_dummy_pipeline.py tests/e2e/test_dynolog_dummy_pipeline.py— 31 passed (Linux, Python 3.12)black,isort, andflake8clean at the repo-pinned versionstests/e2e/test_tracelens_workflows.pyThe new tests are POSIX-only (the in-container half is shell) and skip on Windows.