diff --git a/README.md b/README.md index c8ad7b5..705bb71 100644 --- a/README.md +++ b/README.md @@ -49,7 +49,7 @@ composition.) | Build | `ptxas` | `ptxas-verbose` | kernel_codegen | `nvcc -Xptxas -v` (**no GPU needed**) | ✅ pure Python | | Build | `clang_time_trace` | `clang-time-trace`, `ftime-trace` | build_phase | `clang -ftime-trace` | ✅ pure Python | | Build | `cmake_profile` | `cmake-profile`, `cmake-trace` | build_phase | `cmake --profiling-format=google-trace` (≥3.18) | ✅ pure Python | -| Build | `ninja_log` | `ninja-log` | build_step | `.ninja_log` (byproduct of any `ninja` build) | ✅ pure Python | +| Build | `ninja_log` | `ninja-log`, `ninja_log` | build_step | `.ninja_log` (byproduct of any `ninja` build) | ✅ pure Python | | Build | `cargo_diag` | `cargo-diag`, `cargo-json` | build_diag | `cargo build --message-format=json` | ✅ pure Python | | Build | `criterion` | `criterion`, `criterion-json` | benchmark | `cargo bench` (`target/criterion` dir) | ✅ pure Python | | CI | `gha_log` | `gha-log`, `gh-run-log` | ci_step | saved `gh run view --log` | ✅ pure Python | diff --git a/src/perfdigest/adapters/chrome_trace/backend.py b/src/perfdigest/adapters/chrome_trace/backend.py index 3d1724f..4aa7e9a 100644 --- a/src/perfdigest/adapters/chrome_trace/backend.py +++ b/src/perfdigest/adapters/chrome_trace/backend.py @@ -17,7 +17,11 @@ _PLATFORMS = frozenset({"linux", "darwin", "win32"}) # torch captures everywhere CHROME_TRACE_USAGE = ( - "Chrome-trace digest (torch/Kineto, JAX, clang -ftime-trace...). Capture in " + "Chrome-trace digest (torch/Kineto, JAX, and other Chrome-trace emitters). " + "NOT for compiler/build traces that have their own backend: clang " + "-ftime-trace files belong to format clang-time-trace (its [total] tagging " + "prevents double-counting aggregate phases) and cmake profiling output to " + "format cmake-profile (B/E pair folding). Capture in " "code, exporting to a FILE (never print the profiler table):\n" " with torch.profiler.profile(activities=[ProfilerActivity.CPU, " "ProfilerActivity.CUDA]) as prof:\n" diff --git a/src/perfdigest/adapters/chrome_trace/trace_reader.py b/src/perfdigest/adapters/chrome_trace/trace_reader.py index 6cd37cb..3a6c678 100644 --- a/src/perfdigest/adapters/chrome_trace/trace_reader.py +++ b/src/perfdigest/adapters/chrome_trace/trace_reader.py @@ -1,15 +1,18 @@ """Read Chrome-trace JSON (torch/Kineto & friends) -> NormalizedUnit. The FRAMEWORK layer. ``torch.profiler`` (Kineto) exports the Chrome Trace -Format — and so do JAX, clang ``-ftime-trace``, Bazel and others — so this one -pure-Python reader digests them all. We bind to the exported ARTIFACT, never to -the framework's profiler API: the format has stayed stable across torch -releases while the internals churned (fragility stays on the other side of the -file boundary). +Format — and so do JAX, Bazel and others — so this one pure-Python reader +digests them all. We bind to the exported ARTIFACT, never to the framework's +profiler API: the format has stayed stable across torch releases while the +internals churned (fragility stays on the other side of the file boundary). +Compiler/build traces that share the format have DEDICATED backends built on +these same helpers — clang ``-ftime-trace`` -> ``clang_time_trace`` (adds +``[total]`` aggregate tagging), cmake profiling -> ``cmake_profile`` (adds +B/E pair folding) — route those formats there, not here. Two top-level shapes, both handled: - * dict with a ``traceEvents`` list (Kineto, ``-ftime-trace``) + * dict with a ``traceEvents`` list (Kineto and friends) * bare JSON array of events (legacy exporters) Only complete events (``"ph": "X"``) with a numeric ``dur`` are aggregated — @@ -92,6 +95,11 @@ def _fold_begin_end_pairs(events: list) -> list: dur = float(e["ts"]) - float(b["ts"]) except (KeyError, TypeError, ValueError): continue # unpaired timing info: skip, never fabricate + if dur < 0: + # E before its own B on one thread is emitter/clock + # corruption — a negative elapsed is physically impossible, + # so drop the pair like an unparseable one. + continue out = dict(b) out["ph"] = "X" out["dur"] = dur @@ -127,7 +135,7 @@ def _complete_events(report_path: str, *, fold_be_pairs: bool = False) -> list[d ) if fold_be_pairs: events = _fold_begin_end_pairs(events) - return [ + complete = [ e for e in events if isinstance(e, dict) @@ -137,6 +145,23 @@ def _complete_events(report_path: str, *, fold_be_pairs: bool = False) -> list[d and e.get("name") is not None and str(e["name"]).strip() != "" ] + if not complete and events: + # A valid trace with zero digestible events must not become a silent + # empty report ("the run did nothing" is a conclusion, not a default). + phases = {e.get("ph") for e in events if isinstance(e, dict)} + if not fold_be_pairs and ("B" in phases or "E" in phases): + raise ValueError( + f"{report_path} contains only begin/end (ph B/E) pair events and " + "no complete (ph 'X') events — this looks like a cmake " + "--profiling-format=google-trace capture; digest it with format " + "cmake-profile, which folds the pairs." + ) + raise ValueError( + f"{report_path} is a valid trace container but has no complete " + "(ph=='X') events with a numeric dur — nothing to digest. Check the " + "emitter and the format argument." + ) + return complete def _grouped( @@ -199,7 +224,13 @@ def _grouped( forced = tag_override(rec) if tag_override is not None else None collide = len(cats_per_name[rec["name"]]) > 1 and rec["cat"] if forced: - rec["unit_name"] = f"{rec['name']} [{forced}]" + # A forced tag must not defeat the uniqueness invariant: if the + # name ALSO collides across categories, keep the cat tag too. + rec["unit_name"] = ( + f"{rec['name']} [{forced}] [{rec['cat']}]" + if collide + else f"{rec['name']} [{forced}]" + ) elif collide or rec["cat"] in _BOOKKEEPING_CATS: rec["unit_name"] = f"{rec['name']} [{rec['cat']}]" else: diff --git a/src/perfdigest/adapters/criterion/criterion_reader.py b/src/perfdigest/adapters/criterion/criterion_reader.py index 8c71f1c..4fc623f 100644 --- a/src/perfdigest/adapters/criterion/criterion_reader.py +++ b/src/perfdigest/adapters/criterion/criterion_reader.py @@ -49,11 +49,11 @@ def _bench_dirs(root: Path) -> list[tuple[str, Path]]: found: list[tuple[str, Path]] = [] for est in root.glob("**/new/estimates.json"): rel_parts = est.parent.parent.relative_to(root).parts - # criterion's own HTML lives under report/ dirs (the root's and each - # benchmark's); no estimates.json is written there, but guard anyway so - # a bookkeeping dir can never masquerade as a benchmark. - if "report" in rel_parts: - continue + # criterion's own HTML bookkeeping lives under report/ dirs, but those + # never contain a new/estimates.json, so the glob alone excludes them. + # No name-based guard on top: a user CAN name a bench "report" (it + # collides with criterion's HTML dir — criterion's wart, not ours), and + # suppressing its genuinely-written estimates would fabricate absence. found.append(("/".join(rel_parts), est)) if not found: raise ValueError( diff --git a/src/perfdigest/adapters/gha_log/backend.py b/src/perfdigest/adapters/gha_log/backend.py index cfb4c4c..a581f97 100644 --- a/src/perfdigest/adapters/gha_log/backend.py +++ b/src/perfdigest/adapters/gha_log/backend.py @@ -11,6 +11,7 @@ from __future__ import annotations import subprocess +import time from perfdigest.adapters import registry from perfdigest.adapters.gha_log import gha_log_reader, mapping @@ -57,6 +58,49 @@ ) +# `gh auth status` is the fleet's only probe doing I/O beyond shutil.which — +# it can hit the network. capability_summary() probes every backend, so the +# auth result is memoized per process with a TTL: platform_capabilities stays +# fast/offline-safe, while a mid-session `gh auth login` is still picked up +# within a few minutes (pre-release review finding 5). +_AUTH_TTL_S = 300.0 +_auth_memo: tuple[float, CapabilityReport] | None = None + + +def _clear_auth_memo() -> None: + """Testing hook: forget the memoized auth probe (mirrors cache.clear()).""" + global _auth_memo + _auth_memo = None + + +def _auth_probe(exe: str) -> CapabilityReport: + global _auth_memo + now = time.monotonic() + if _auth_memo is not None and now - _auth_memo[0] < _AUTH_TTL_S: + return _auth_memo[1] + try: + result = subprocess.run( + [exe, "auth", "status"], capture_output=True, text=True, timeout=10 + ) + except Exception as exc: # noqa: BLE001 — surfaced as a diagnosable reason + report = CapabilityReport( + False, f"gh present but 'gh auth status' failed to run: {exc}", exe + ) + else: + if result.returncode != 0: + report = CapabilityReport( + False, + "gh present but not authenticated (gh auth status failed); run " + "`gh auth login` to enable capture. Digesting an already-saved " + ".gha.log never needs authentication.", + exe, + ) + else: + report = CapabilityReport(True, "gh present and authenticated", exe) + _auth_memo = (now, report) + return report + + def _probe() -> CapabilityReport: info = detect() exe = info.profilers_on_path.get("gha_log") @@ -65,23 +109,7 @@ def _probe() -> CapabilityReport: # `gh` present is necessary but not sufficient for CAPTURE: pulling a real # run's log also needs an authenticated session. Digesting an already-saved # log needs neither check — that split is called out in the usage prompt. - try: - result = subprocess.run( - [exe, "auth", "status"], capture_output=True, text=True, timeout=10 - ) - except Exception as exc: # noqa: BLE001 — surfaced as a diagnosable reason - return CapabilityReport( - False, f"gh present but 'gh auth status' failed to run: {exc}", exe - ) - if result.returncode != 0: - return CapabilityReport( - False, - "gh present but not authenticated (gh auth status failed); run " - "`gh auth login` to enable capture. Digesting an already-saved " - ".gha.log never needs authentication.", - exe, - ) - return CapabilityReport(True, "gh present and authenticated", exe) + return _auth_probe(exe) def _capture_command(target: str, info: PlatformInfo) -> str: diff --git a/src/perfdigest/adapters/gha_log/gha_log_reader.py b/src/perfdigest/adapters/gha_log/gha_log_reader.py index 8e4d8ec..988bdc0 100644 --- a/src/perfdigest/adapters/gha_log/gha_log_reader.py +++ b/src/perfdigest/adapters/gha_log/gha_log_reader.py @@ -198,6 +198,12 @@ def raw_metrics(report_path: str, kernel_index: int, name_filter: str) -> dict[s "error_lines": rec["error_lines"][:_MAX_RAW_ANNOTATION_LINES], "warning_lines": rec["warning_lines"][:_MAX_RAW_ANNOTATION_LINES], } + # Explicit truncation markers (only when truncation happened) — the caller + # must not have to infer it by comparing counts against list lengths. + if len(rec["error_lines"]) > _MAX_RAW_ANNOTATION_LINES: + out["error_lines_truncated_to"] = _MAX_RAW_ANNOTATION_LINES + if len(rec["warning_lines"]) > _MAX_RAW_ANNOTATION_LINES: + out["warning_lines_truncated_to"] = _MAX_RAW_ANNOTATION_LINES wanted = None if name_filter.lower() == "all" else name_filter.lower() if wanted is not None: out = {k: v for k, v in out.items() if wanted in k.lower()} diff --git a/src/perfdigest/adapters/git_numstat/numstat_reader.py b/src/perfdigest/adapters/git_numstat/numstat_reader.py index 3055be1..bebf498 100644 --- a/src/perfdigest/adapters/git_numstat/numstat_reader.py +++ b/src/perfdigest/adapters/git_numstat/numstat_reader.py @@ -34,6 +34,11 @@ * Non-ASCII paths arrive C-quoted by git (``"docs/\\303\\266l\\303\\247..."``, core.quotePath default) and are kept VERBATIM as printed — the name is honest to the artifact; we do not unquote. + * Known ambiguity, inherent to the unquoted artifact: a plain file whose + NAME literally contains ``" => "`` (``notes about a => b.txt``) is + indistinguishable from a whole-path rename in numstat output — git prints + both identically — and parses as a rename here. git's own limitation; + ``expand`` shows ``path_as_printed`` so the raw line is always recoverable. ``duration_us`` is ``None`` for every unit: a change digest has no time dimension at all, so ``summarize_report`` falls back to file order and ranking diff --git a/src/perfdigest/report_store/cache.py b/src/perfdigest/report_store/cache.py index 6f9f149..a9490d3 100644 --- a/src/perfdigest/report_store/cache.py +++ b/src/perfdigest/report_store/cache.py @@ -48,7 +48,12 @@ def cached_units( # re-run and would serve stale units. Caching must stay behaviorally # invisible, so directory refs are parsed fresh every time (the trees # are small JSON files; the IO saving is not worth the staleness risk). - return loader(path) + # Same mutation-proofing as the cached path — the module invariant + # ("no caller can mutate shared state") holds for every return. + return [ + replace(u, metrics=MappingProxyType(dict(u.metrics))) + for u in loader(path) + ] key = (backend_name, abspath, st.st_mtime_ns, st.st_size) with _LOCK: diff --git a/src/perfdigest/server/prompts.py b/src/perfdigest/server/prompts.py index 583627b..ae6f5c7 100644 --- a/src/perfdigest/server/prompts.py +++ b/src/perfdigest/server/prompts.py @@ -12,10 +12,13 @@ from perfdigest.server.app import mcp _HEADER = """\ -You have perfdigest: token-efficient access to performance-profiler reports across -backends (NVIDIA, AMD HIP, CPU perf, Apple Metal, ptxas codegen, torch/Chrome -traces). It is a translator, not a judge — it returns clean numbers; deciding -"memory-bound?"/"occupancy-limited?" is YOUR job. +You have perfdigest: token-efficient access to the development loop's report +artifacts across four feedback channels — performance profilers (NVIDIA, AMD +HIP, CPU perf, Apple Metal, torch/Chrome traces), build tools (ptxas codegen, +clang -ftime-trace, cmake configure profiles, ninja logs, cargo diagnostics, +criterion benchmarks), CI run logs (GitHub Actions), and repo change state +(git numstat). It is a translator, not a judge — it returns clean numbers; +deciding "memory-bound?"/"build-bottlenecked?"/"release-ready?" is YOUR job. perfdigest has TWO operations — keep them separate: diff --git a/tests/test_criterion.py b/tests/test_criterion.py index 2fb4708..58fb8a0 100644 --- a/tests/test_criterion.py +++ b/tests/test_criterion.py @@ -113,18 +113,21 @@ def test_dir_without_estimates_raises_loudly_never_silent_empty(tmp_path): assert "no */new/estimates.json" in str(exc.value) -def test_criterion_report_bookkeeping_dirs_are_skipped(tmp_path, fixtures_dir): - # criterion writes its own HTML under report/ dirs. Real trees put no - # estimates.json there, but the guard must hold even if one appeared - # (synthetic tree: a real estimates.json copied under a report/ path). +def test_bench_named_report_is_digested_never_suppressed(tmp_path, fixtures_dir): + # criterion's own HTML bookkeeping lives under report/ dirs but NEVER + # contains new/estimates.json, so the glob alone excludes it. A user CAN + # name a bench "report" (criterion's known dir-collision wart) and its + # genuinely-written estimates must appear — suppressing them by path name + # would fabricate absence (pre-release review finding 2; synthetic tree + # built from a real estimates.json). root = tmp_path / "criterion" real = fixtures_dir / "criterion_sample" / "criterion" / "fib_plain" / "new" / "estimates.json" - for rel in ("mybench/new", "report/mybench/new"): + for rel in ("mybench/new", "report/new"): d = root / rel d.mkdir(parents=True) shutil.copy(real, d / "estimates.json") units = tools.list_kernels(str(root), FMT) - assert [u["name"] for u in units] == ["mybench"] # report/ never a benchmark + assert [u["name"] for u in units] == ["mybench", "report"] def test_rerun_overwrite_is_seen_immediately(tmp_path, fixtures_dir): diff --git a/tests/test_gha_log.py b/tests/test_gha_log.py index c739caa..a50bc6a 100644 --- a/tests/test_gha_log.py +++ b/tests/test_gha_log.py @@ -296,6 +296,7 @@ class _Unauthed: returncode = 1 monkeypatch.setattr(gha_backend.subprocess, "run", lambda *a, **k: _Unauthed()) + gha_backend._clear_auth_memo() # TTL memo (finding 5) must not leak stages report = gha_backend._probe() assert report.available is False assert "auth" in report.reason.lower() @@ -305,6 +306,7 @@ class _Authed: returncode = 0 monkeypatch.setattr(gha_backend.subprocess, "run", lambda *a, **k: _Authed()) + gha_backend._clear_auth_memo() # TTL memo (finding 5) must not leak stages report = gha_backend._probe() assert report.available is True assert report.tool == "/usr/bin/gh"