Repository navigation
chore: enforce lint and formatting with pre-commit - #18
Conversation
The repository declares these checks in the Makefile -- flake8 with --max-line-length=120 --ignore=E501,W503, isort and black -- but nothing enforces them: flake8 is not installed anywhere, `python3 -m isort` and `python3 -m black` resolve to modules that are not present, and the CI job only runs three test scripts. Pin the same tools in pre-commit so every hook runs in its own environment and `pre-commit run --all-files` is all a contributor needs. The flake8 limits are the Makefile's own (E501, W503), extended rather than replaced so the stock defaults keep working, plus F403/F405, because `from pymtml import *` is how the examples and tests are meant to use this package. The vendored mtml_2.2.0.h keeps its own text.
Black reformats 12 files and isort reorders imports in 3 of them; the whitespace and end-of-file hooks clean up a few blanks and the missing final newline in test_pynvml.py. All of it is mechanical; the vendored mtml_2.2.0.h is excluded so the C header keeps mirroring its source. Doing this in one commit now, while no pull request is open, keeps the reformat out of everyone's way: from here the hooks only touch files a change actually modifies.
Formatting could not fix these, so they are code changes: * `except:` with `pass` in the shutdown paths of test_pymtml.py and test_sglang_compat.py would also swallow KeyboardInterrupt; narrow it to `except Exception`. * `import torch_musa` is a side-effect import that registers the MUSA backend for torch; mark it `# noqa: F401` instead of letting the gate call it unused. * `x == None` -> `x is None` in four loader checks, and drop the unused `as ose` binding next to them. * 14 f-strings without placeholders lost their `f` prefix; two comment artifacts (`##`) were tidied. `pre-commit run --all-files` is green after this commit.
The `format` and `lint` targets called isort, black and flake8 through `$(PYTHON) -m ...`, which only works when those modules are installed for the same interpreter as the repository's python. They are not, so both targets died with ModuleNotFoundError. Delegate to the pinned hooks instead: `make format` runs everything and lets pre-commit fix what it can; `make lint` runs only the read-only checks (flake8, codespell, check-ast).
There was a problem hiding this comment.
Merge verdict: no blocking findings — ready to merge.
Verified at 6f8b66a: every reformat hunk is AST-identical to base.
No blocking findings remain, so a human can merge. This account authored #18 and cannot approve it, which is why this is a comment; this round ends auto-review for 6f8b66a, and a later push starts a fresh cycle at round 1.
Findings: 0 blocking · 6 nit
| nit | location | problem | fix |
|---|---|---|---|
| nit | .github/workflows/main.yml:27 |
no CI step runs the gate | add a pre-commit run --all-files step |
| nit | .pre-commit-config.yaml:27 |
isort has no profile: black (79 vs 88) |
add args: ["--profile=black"] |
| nit | Makefile:28 |
lint skips black/isort |
run both in lint |
| nit | pymtml.py:952 |
one message split across two literals | use a single f-string |
What changed · what I could not verify
Diff map. New .pre-commit-config.yaml (11 hooks); Makefile format/lint rewired to pre-commit; 15 Python files reformatted plus four real code fixes; two docs get a final newline.
Verified — the reformat is semantics-preserving. I parsed base b594a7db and head 6f8b66ab for all 15 modified Python files and compared normalized ASTs. examples/02,03,05,07,08,09,10,11,12,13 and test_pynvml.py are byte-identical as ASTs (including the triple-quoted .format() block in 13_comprehensive_report.py). The only deltas are the intended ones: contextlib import move, 4× == None → is None, the unused as ose binding, from sys import merge, and 2× bare except → except Exception.
Verified — flake8 classes. AST scan of all 23 head Python files: no F541 (I initially flagged examples/04/06, then found my own bug — {x:.2f} format specs are JoinedStr nodes; the real scan is clean), no E711/E712/E722, no unused imports beyond the # noqa-annotated torch_musa, no unused locals beyond global-declared names (pyflakes exempts those).
Verified — the other hooks. Across all 32 tracked text files: no trailing whitespace, no missing/extra EOF newline, no conflict or debug-statement patterns. The ====... lines in MANUAL.md are 60-char sample-output separators, not conflict markers.
Verified — pins and codespell. All five revs exist as tags (v6.0.0, 7.0.0, 26.1.0, 7.1.1, v2.4.1). v2.4.1 still maps renderd->rendered (dictionary.txt:49585), but -L renderD is honored: v2.4.1 added ignore_words_cased, a case-sensitive match for mixed-case entries. My local codespell 2.2.6 lacks that and flagged it — a version artifact, not a defect here.
Could not verify. pre-commit run --all-files, black, isort and flake8 were never executed: the tool modules are absent in this sandbox and installs are out of scope, so the PR's "all 11 hooks pass" is unconfirmed by static evidence only. codespell 2.4.1 carries a larger dictionary than the 2.2.6 I ran.
CI. gh pr checks reports two Unit-test-job entries, both pending, zero failing or cancelled; the merge is UNSTABLE on those.
Also noted, not in the table. Makefile:11 — PACKAGE lost its last use when the recipes moved to --all-files; delete it. Makefile:6-8 — the comment says no global install is needed, but pre-commit itself must be on PATH, so a fresh clone gets exit 127 with no hint; guard it or name the prerequisite.
auto-review round 1/3 · verdict: clean
The repository declares its lint and format rules in the Makefile -- flake8 with
--max-line-length=120 --ignore=E501,W503, isort and black -- but nothing enforces them, and the targets themselves do not run:python3 -m isortandpython3 -m blackresolve to modules that are not installed, and flake8 is missing entirely, somake lint/make formatdie withModuleNotFoundError.Pin the same tools in pre-commit, so each one runs in its own managed environment:
What the first run found, and what this PR fixes
F541f-string without placeholdersE711== NoneE722bareexcept(would swallowKeyboardInterrupt)F401unused side-effect import (torch_musa)F841unusedas osebindingThe reformat is one commit on its own so it is easy to skip in review; it is done now, while no pull request is open, and from here the hooks only touch the files a change actually modifies.
Configuration notes
mtml_2.2.0.his a vendored copy of the C header: excluded from the whitespace and spelling hooks so it keeps mirroring its source.E501,W503) through--extend-ignore, so the stock defaults keep working, and additionally allowsE203(what black produces),E266(the##banner comments this tree uses) andF403/F405(from pymtml import *is how the examples and tests are meant to use the package).Verification
pre-commit run --all-files-- all 11 hooks passmake lint,make format-- green, no changes left behindtest_pymtml.py,test_pynvml.py,test_sglang_compat.py-- each exits 0pytest test_library_loading.py test_abi_layout.py-- 13 passedNot in this PR
make test(pytest test_*.py) reports "15 passed, 8 errors":test_pynvml.pyandtest_sglang_compat.pyare unittest-style scripts that pytest also tries to collect. Worth a separate fix (atestpathsentry or a__test__ = Falsemarker); left out here to keep this one reviewable.