fix test#72
Conversation
🤖 Claude Code ReviewReview🐛 BugsRemoved imports may break runtime behaviour if the symbols are still used The diff removes several names from
Without seeing the full file bodies it is impossible to confirm these are safe, and the diff gives no evidence the usages were removed. Verify with 💡 Suggestions
|
Code Review by GeminiThe changes primarily involve removing unused imports across several files. This is a good practice for code hygiene and reducing unnecessary dependencies. No functional bugs or issues are introduced by these 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
Improvement: --- 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: intImprovement: --- 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"),Improvement: --- 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] = {Improvement: --- 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 laterImprovement: --- 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/sImprovement: |
🤖 Claude Code ReviewCode Review🐛 Bugs
# torch_trace.py line ~94
if isinstance(val, (list | tuple)) and len(val) >= 1:
# tests/test_importers_hypothesis.py
assert isinstance(ev, (KernelEvent | LightKernel))
assert isinstance(ev, (KernelEvent | MemcpyEvent | LightKernel | LightMemcpy))
This is a straight regression — the original code was correct, the new code is broken on any Python < 3.10. Fix by reverting to the tuple-of-types form: # Correct
isinstance(val, (list, tuple))
isinstance(ev, (KernelEvent, LightKernel))If Python 3.10+ is a hard requirement, that constraint should be explicit in 💡 SuggestionsRemoved imports from
Same concern — confirm
If |
Code Review by GeminiHere's a review of the provided code changes: Potential Issues1. Python Version Compatibility for
2. Verification of Removed Imports
|
🤖 Claude Code ReviewCode Review🐛 Bugs
# torch_trace.py line ~94
if isinstance(val, (list | tuple)) and len(val) >= 1:
# tests/test_importers_hypothesis.py
assert isinstance(ev, (KernelEvent | LightKernel))
assert isinstance(ev, (KernelEvent | MemcpyEvent | LightKernel | LightMemcpy))
Correct forms: # Python 3.10+ only:
isinstance(val, list | tuple)
# Compatible with 3.8+:
isinstance(val, (list, tuple))The test file also has 💡 SuggestionsRemoved imports from
Both modules dropped
if isinstance(val, (list | tuple)) and len(val) >= 1: # outer guard
x = as_int(val[0], 1) if len(val) > 0 else 1 # redundant re-checkThe inner |
Code Review by Gemini## Code Review
### gitm/importers/_common.py
**Suggestion:**
The reordering of `import sys` is a minor stylistic change and doesn't introduce any functional issues. It's generally good practice to group standard library imports together.
### gitm/importers/analyze.py
### gitm/importers/node_rollup.py
### gitm/importers/nsys.py
### gitm/importers/torch_trace.py
### gitm/optimizer/metrics.py
**Potential Issue:**
Several imports (`KernelEvent`, `MemcpyEvent`, `SyncEvent`, `device_count_from_events`, `filter_device`, `per_device_kernel_counts`) have been removed from various files. While this is often a cleanup of unused imports, please ensure that these types/functions are indeed no longer used within the respective files. If they are still referenced, this change will lead to runtime errors.
### gitm/importers/torch_trace.py
**Bug:**
The change in `isinstance` syntax from `(list, tuple)` to `(list | tuple)` is only valid in Python 3.10 and later. According to `pyproject.toml`, the project supports Python `>=3.9`. In Python 3.9, `type | type` is not supported for `isinstance` and will raise a `TypeError`.
**Proposed change:**
```diff
--- a/gitm/importers/torch_trace.py
+++ b/gitm/importers/torch_trace.py
@@ -84,7 +84,7 @@ def _arg_get(args: dict[str, Any] | None, keys: tuple[str, ...]) -> Any:
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 1pyproject.tomlSuggestion: tests/test_importers_hypothesis.pyBug: Proposed change: --- a/tests/test_importers_hypothesis.py
+++ b/tests/test_importers_hypothesis.py
@@ -34,7 +34,7 @@ def test_prop_us_to_ns_monotonic(ts: float, dur: float):
}
)
# 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
@@ -67,7 +67,7 @@ def test_prop_missing_args_never_crash(cat, name, has_grid, has_stream, has_devi
# 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 |
1 participant
No description provided.