Fix #67 lint/type/test-dep debt on main#70
Open
nicholaslawrence-hub wants to merge 2 commits into
Open
Conversation
#67 (gitm analyze) landed with three pre-existing issues that break a clean CI signal for any branch that merges main: * ruff: 8 unused imports (F401) across the new importers + one already in optimizer/metrics.py, plus an unsorted import block (I001) in importers/_common.py. `ruff check .` is a required CI job and was red. * pytest collection: tests/test_importers_hypothesis.py imports `hypothesis`, which was in no dependency group, so `pip install -e ".[dev,bench]"` + pytest could not even collect the suite. Added hypothesis>=6.0 to the dev extra. * mypy: the three HFT/OpenFold/Edge early-return blocks in scheduler/loop.py each bound a local `applicator` (Any | None) before the real `applicator: Applicator`, tripping [no-redef] and cascading into two [arg-type] errors. Renamed the block-locals to `runner_applicator`; loop.py is mypy-clean again. Behavior-preserving: only unused imports and a local variable name change. Full suite shows no new failures vs origin/main. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
🤖 Claude Code ReviewReview💡 SuggestionsImport cleanup (
The three blocks are now identical in structure; consider extracting a helper: # Suggested refactor to eliminate the triple repetition:
_INTERVENTION_MAP = {
frozenset(_HFT_INTERVENTION_WORKLOADS): _hft_intervention_result,
frozenset(_OPENFOLD_INTERVENTION_WORKLOADS): _openfold_intervention_result,
frozenset(_EDGE_INTERVENTION_WORKLOADS): _edge_intervention_result,
}
for workload_set, handler in _INTERVENTION_MAP.items():
if workload in workload_set:
runner_applicator = getattr(runner, "applicator", None)
if runner_applicator is not None:
return handler(
run_dir=run_dir, run_id=run_id, workload=workload,
trace=trace, qual=qual, applicator=runner_applicator,
started_ns=started_ns, trace_path=trace_path,
)
break # workload matched but no applicator; fall through once
🐛 BugsNothing definitive from the diff alone, but the mass-removal of imports without a corresponding test run shown is the highest-risk area. Ensure the CI suite exercises all affected importers end-to-end, not just unit tests of individual functions, since dead-import errors in Python only surface at import time. |
Code Review by GeminiThe changes in this pull request effectively address the linting, type-checking, and test dependency issues as described in the commit message. No new bugs or issues are introduced by these changes, and the proposed solutions are correct. Here's a breakdown of the changes:
|
The pinned CI ruff (0.12.11) enforces UP038, which newer local ruff (0.15+) dropped — so these three isinstance(x, (A, B)) calls in #67 code were invisible locally but red on CI's lint job. Rewrite them as isinstance(x, A | B) (PEP 604 union; runtime-supported on the 3.10-3.12 CI matrix). test_importers_hypothesis passes unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
🤖 Claude Code ReviewReview🐛 Bugs
if isinstance(val, list | tuple) and len(val) >= 1:
x = as_int(val[0], 1) if len(val) > 0 else 1 # dead branchThe outer Unused imports silently removed ( 💡 Suggestions
Import sort change ( |
Code Review by GeminiThe changes in this pull request effectively address the linting, typing, and test dependency issues as described in the commit message. The fixes are clean and align with modern Python practices. Here are the specific improvements and fixes observed:
No issues or bugs were found in the provided changes. --- a/gitm/importers/_common.py
+++ b/gitm/importers/_common.py
@@ -1,20 +1,20 @@
"""Shared helpers for profiler importers."""
from __future__ import annotations
import os
+import sys
import tempfile
import warnings
from collections.abc import Iterable
from dataclasses import dataclass, field
-import sys
from pathlib import Path
from typing import Any
from gitm.tracer.schema import KernelEvent, MemcpyEvent, SyncEvent, Trace, TraceEvent
# Above this event count, skip O(n) exact-row dedupe (dominant cost on 5M-event
# imports; identical-row rate on real profiler dumps is near zero).
_DEDUPE_MAX_EVENTS = 100_000
Comment: Correctly reordered imports to fix --- a/gitm/importers/analyze.py
+++ b/gitm/importers/analyze.py
@@ -11,21 +11,21 @@ from typing import Any
from gitm.importers._common import ImportError, ImportStats, atomic_write_text
from gitm.importers.detect import DetectedFormat, detect_format
from gitm.importers.node_rollup import NodeRollup, build_node_rollup
from gitm.optimizer.headroom import HeadroomReport, build_headroom, render_headroom_md
from gitm.optimizer.metrics import HardwarePeak, compute_metrics
from gitm.optimizer.preconditions import GateContext
from gitm.optimizer.qualification import QualificationResult, qualify
from gitm.planner.context import peak_for_sku
from gitm.tracer.capture import write_trace_jsonl
-from gitm.tracer.schema import KernelEvent, Trace
+from gitm.tracer.schema import Trace
# Default catalogue peak when SKU is unknown (A100 PCIe figures; called out in caveats).
_DEFAULT_PEAK = HardwarePeak(name="A100", peak_flops=312e12, peak_bw_bytes_s=1555e9)
@dataclass
class DeviceAnalysis:
"""One device's metrics + headroom within a multi-device workload file."""
device_id: intComment: Correctly removed unused import --- a/gitm/importers/node_rollup.py
+++ b/gitm/importers/node_rollup.py
@@ -4,21 +4,21 @@ Computes device skew, collective communication share, and exposed (non-overlappe
communication time. Pure timestamp math — no invented counters.
"""
from __future__ import annotations
import re
from dataclasses import dataclass, field
from typing import Any
from gitm.optimizer.metrics import _merge_intervals
-from gitm.tracer.schema import KernelEvent, Trace
+from gitm.tracer.schema import Trace
# Collective kernel name patterns (case-insensitive). One module-level table;
# each entry is a substring match against the demangled/exported kernel name.
# Comment: covers NCCL device kernels and common collective op labels in chrome
# traces. Unknown collective libraries (e.g. proprietary) will not match —
# zero hits on multi-device is reported as inconclusive, not zero communication.
_COMM_PATTERNS: tuple[tuple[str, str], ...] = (
("nccl", "NCCL library prefix (any nccl* kernel)"),
("ncclkernel", "NCCL device kernel entry points"),
("allreduce", "AllReduce collective"),Comment: Correctly removed unused import --- a/gitm/importers/nsys.py
+++ b/gitm/importers/nsys.py
@@ -10,25 +10,22 @@ import sqlite3
import subprocess
import tempfile
from collections.abc import Iterator
from pathlib import Path
from typing import Any
from gitm.importers._common import (
ImportError,
ImportStats,
as_int,
- device_count_from_events,
file_mtime_ns,
- filter_device,
finish_trace,
- per_device_kernel_counts,
)
from gitm.tracer.schema import KernelEvent, MemcpyEvent, SyncEvent, Trace, TraceEvent
# ---------------------------------------------------------------------------
# CUPTI enum maps — integers differ across CUPTI versions; one table per kind.
# Values annotated with the CUPTI header name from cupti_activity.h.
# ---------------------------------------------------------------------------
# CUpti_ActivityMemoryKind
_MEMORY_KIND: dict[int, str] = {Comment: Correctly removed unused imports --- a/gitm/importers/torch_trace.py
+++ b/gitm/importers/torch_trace.py
@@ -17,21 +17,21 @@ from typing import Any
from gitm.importers._common import (
ImportError,
ImportStats,
as_int,
device_count_from_events,
file_mtime_ns,
filter_device,
finish_trace,
per_device_kernel_counts,
)
-from gitm.tracer.schema import KernelEvent, MemcpyEvent, Trace, TraceEvent
+from gitm.tracer.schema import MemcpyEvent, Trace, TraceEvent
# Max decompressed size for .json.gz (customer dumps can be multi-GB).
DEFAULT_MAX_DECOMPRESSED_BYTES = 20 * 1024 ** 3 # 20 GiB
# Complete events only.
_PHASE_COMPLETE = "X"
# Kernel categories across torch versions.
_KERNEL_CATS = frozenset({"kernel", "Kernel", "gpu_op", "gpu_memcpy", "Memcpy"})
_MEMCPY_CATS = frozenset({"gpu_memcpy", "Memcpy", "memcpy", "gpu_memset"}) # memset skipped later
@@ -84,21 +84,21 @@ def _arg_get(args: dict[str, Any] | None, keys: tuple[str, ...]) -> Any:
lower = {str(k).lower(): v for k, v in args.items()}
for k in keys:
if k.lower() in lower:
return lower[k.lower()]
return None
def _as_dims(val: Any) -> tuple[int, int, int]:
if val is None:
return (1, 1, 1)
- if isinstance(val, (list, tuple)) and len(val) >= 1:
+ if isinstance(val, list | tuple) and len(val) >= 1:
x = as_int(val[0], 1) if len(val) > 0 else 1
y = as_int(val[1], 1) if len(val) > 1 else 1
z = as_int(val[2], 1) if len(val) > 2 else 1
return (max(x, 1), max(y, 1), max(z, 1))
if isinstance(val, dict):
return (
max(as_int(val.get("x", val.get("X", 1)), 1), 1),
max(as_int(val.get("y", val.get("Y", 1)), 1), 1),
max(as_int(val.get("z", val.get("Z", 1)), 1), 1),
)Comment:
--- a/gitm/optimizer/metrics.py
+++ b/gitm/optimizer/metrics.py
@@ -19,21 +19,21 @@ MBU (Memory Bandwidth Utilization) — achieved bytes/s ÷ peak bandwidth,
HFU vs MFU is deliberately explicit: HFU counts every FLOP the silicon issued,
MFU only the FLOPs that advanced the model. Reporting both makes "the GPU is
busy" (high HFU) vs "the GPU is doing useful work" (high MFU) separable.
"""
from __future__ import annotations
from collections.abc import Callable, Iterable
from dataclasses import dataclass
-from gitm.tracer.schema import KernelEvent, MemcpyEvent, SyncEvent, Trace
+from gitm.tracer.schema import KernelEvent, Trace
FlopsModel = Callable[[KernelEvent], float]
@dataclass(frozen=True)
class HardwarePeak:
"""Vendor peak rates for the SKU under test (dense, not sparse)."""
name: str
peak_flops: float # FLOP/sComment: Correctly removed unused imports --- a/gitm/scheduler/loop.py
+++ b/gitm/scheduler/loop.py
@@ -332,65 +332,65 @@ def run_loop(cfg: LoopConfig) -> dict[str, Any]:
},
indent=2,
)
)
# HFT carries a real, output-verified intervention on its runner. Apply+prove
# it through the rollback gate — the A/B runs on the active backend, so the
# delta is measured even on a box without CUPTI. (Runs before the empty-trace
# guard for that reason; attribution below is included only if kernels exist.)
if workload in _HFT_INTERVENTION_WORKLOADS:
- applicator = getattr(runner, "applicator", None)
- if applicator is not None:
+ runner_applicator = getattr(runner, "applicator", None)
+ if runner_applicator is not None:
return _hft_intervention_result(
run_dir=run_dir,
run_id=run_id,
workload=workload,
trace=trace,
qual=qual,
- applicator=applicator,
+ applicator=runner_applicator,
started_ns=started_ns,
trace_path=trace_path,
)
# OpenFold/AF2 carries the bf16 intervention on its runner. Same pattern as
# HFT: apply+prove through the rollback gate (measure() runs the fp32-vs-bf16
# A/B, gated on plDDT-equivalence). Before the empty-trace guard so the A/B
# still runs on a box without CUPTI; attribution is included if kernels exist.
if workload in _OPENFOLD_INTERVENTION_WORKLOADS:
- applicator = getattr(runner, "applicator", None)
- if applicator is not None:
+ runner_applicator = getattr(runner, "applicator", None)
+ if runner_applicator is not None:
return _openfold_intervention_result(
run_dir=run_dir,
run_id=run_id,
workload=workload,
trace=trace,
qual=qual,
- applicator=applicator,
+ applicator=runner_applicator,
started_ns=started_ns,
trace_path=trace_path,
)
# Edge (kitti/nuscenes) carries the fp16 intervention on its runner. Same
# pattern as HFT/AF2: apply+prove through the rollback gate (measure() runs
# the fp32-vs-fp16 A/B, gated on detection-equivalence). Before the empty-
# trace guard so the A/B still runs on a box without CUPTI.
if workload in _EDGE_INTERVENTION_WORKLOADS:
- applicator = getattr(runner, "applicator", None)
- if applicator is not None:
+ runner_applicator = getattr(runner, "applicator", None)
+ if runner_applicator is not None:
return _edge_intervention_result(
run_dir=run_dir,
run_id=run_id,
workload=workload,
trace=trace,
qual=qual,
- applicator=applicator,
+ applicator=runner_applicator,
started_ns=started_ns,
trace_path=trace_path,
)
# Guard: if the tracer captured nothing (no GPU/shim, or the workload never
# ran), do NOT proceed to attribution + emit claims — that fabricates a
# result from an empty trace. Report no-data honestly instead.
if trace.vendor == "none" or not trace.kernels():
diagnostic = runner_error or qual.diagnostic or (
"Tracer captured no GPU kernels. Either no GPU/CUPTI shim is present, "Comment: Correctly renamed the local --- a/pyproject.toml
+++ b/pyproject.toml
@@ -48,20 +48,21 @@ gpu = [
"pynvml>=11.5",
"pyarrow>=15",
"pandas>=2.0",
]
prometheus = ["prometheus-client>=0.20"]
otlp = ["opentelemetry-sdk>=1.25", "opentelemetry-exporter-otlp>=1.25"]
s3 = ["boto3>=1.34"]
dev = [
"pytest>=8.0",
"pytest-cov>=4.1",
+ "hypothesis>=6.0", # property-based fuzz tests for the importers (tests/test_importers_hypothesis.py)
"ruff>=0.4",
"mypy>=1.10",
"build>=1.0", # `python -m build` for the wheel-build verification check
"pre-commit>=3.5", # local lint/test gate; see .pre-commit-config.yaml
]
[project.scripts]
gitm = "gitm.cli:main"
gitm-run-workload = "gitm.runtime_driver:main"
Comment: Correctly added --- a/tests/test_importers_hypothesis.py
+++ b/tests/test_importers_hypothesis.py
@@ -34,21 +34,21 @@ def test_prop_us_to_ns_monotonic(ts: float, dur: float):
{
"ph": "X",
"cat": "Kernel",
"name": "k",
"ts": ts,
"dur": dur,
"args": {"device": 0, "stream": 0},
}
)
# Default import path uses LightKernel for scale; --strict uses KernelEvent.
- assert isinstance(ev, (KernelEvent, LightKernel))
+ assert isinstance(ev, KernelEvent | LightKernel)
assert ev.start_ns == int(ts * 1000.0)
assert ev.end_ns == int((ts + dur) * 1000.0)
assert ev.end_ns >= ev.start_ns or dur == 0.0
# ── null / missing args fall back without crashing ───────────────────────────
@given(
cat=st.sampled_from(["Kernel", "kernel", "gpu_op", "Memcpy", "gpu_memcpy"]),
@@ -67,21 +67,21 @@ def test_prop_missing_args_never_crash(cat, name, has_grid, has_stream, has_devi
if has_grid:
args["grid"] = [2, 1, 1]
args["block"] = [32, 1, 1]
# memset names must skip for Memcpy-ish cats
assume("memset" not in name.lower())
ev = event_from_chrome(
{"ph": "X", "cat": cat, "name": name, "ts": 1.0, "dur": 2.0, "args": args}
)
# May be None for weird name/cat combos; must not raise.
if ev is not None:
- assert isinstance(ev, (KernelEvent, MemcpyEvent, LightKernel, LightMemcpy))
+ assert isinstance(ev, KernelEvent | MemcpyEvent | LightKernel | LightMemcpy)
assert getattr(ev, "kind", None) in ("kernel", "memcpy")
assert ev.end_ns >= ev.start_ns
# Light events are not pydantic; only validate the full Trace path
# when the object supports model_dump (strict/full KernelEvent path).
if hasattr(ev, "model_dump"):
Trace.model_validate(
{
"workload_id": "w",
"fingerprint": "f",
"run_id": "r",Comment: Updated |
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
1 participant
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.
Why
#67 (
gitm analyze/ customer profiler intake) landed onmainwith three pre-existing issues that break a clean CI signal for any branch that merges main. This PR is scoped strictly to fixing that debt — behavior-preserving, no feature work.What
ruff check .CI job): 8 unused imports (F401) across the newgitm/importers/*modules andgitm/optimizer/metrics.py, plus an unsorted import block (I001) inimporters/_common.py.pytestjob):tests/test_importers_hypothesis.pyimportshypothesis, which was in no dependency group — sopip install -e ".[dev,bench]"+pytestcould not collect the suite at all. Addedhypothesis>=6.0to thedevextra.scheduler/loop.pyeach bound a localapplicator(Any | None) before the realapplicator: Applicator, tripping[no-redef]and cascading into two[arg-type]errors. Renamed the block-locals torunner_applicator;loop.pyis mypy-clean again.Verification
ruff check .— passes.mypy gitm/scheduler/loop.py— the three targeted errors are gone.pytestsuite: no new failures vs pristineorigin/main(the handful of local failures are Windows/optional-dep env issues, identical before and after this change), and thehypothesisimporter tests now collect.🤖 Generated with Claude Code