Repository navigation
Conversation
Every pull request ran the full pre-commit suite twice: on pre-commit.ci, configured by the ci: block in .pre-commit-config.yaml, and in the Format job of the Run pre-commit workflow. The two disagree, because they resolve the hooks differently. The flake8 hook declared flake8-bugbear and flake8-print with no versions, and each service caches its own hook environment, built at a different time. When flake8-bugbear 26.10.27 added B045, the Actions job installed it on its next cache miss and started failing unrelated pull requests, while pre-commit.ci kept passing from an older cached environment. Keep pre-commit.ci as the formatting gate and reduce the workflow to the one hook pre-commit.ci does not run: check-manifest, which is manual-stage. Pin the two flake8 plugins so local runs, pre-commit.ci and the workflow resolve to the same tools. Pinning flake8-bugbear at 26.10.27 turns on B045, which flags five nested loops that reuse an enclosing loop's control variable. Rename the inner variables. The enumerate index in model_to_dot was only ever read by the inner loop that shadowed it, so drop it. No behaviour changes. Also drop the python-version matrix from the workflow, which has had no effect since fastmachinelearning#741 removed the setup-python step it fed.
The flake8 hook duplicated ruff's E, W and F checks; what it added was flake8-bugbear. Select B in ruff, plus PLE0100, PLE0101, PLW0129 and RUF024 for the bugbear checks ruff has no B code for, and drop the flake8 hook together with its pinned plugins. Ignore B028 and B905, which flake8-bugbear did not report on this tree and which need a decision at each call site, and chain the four re-raised exceptions B904 reports. Point ruff's include at test/, where the tests are, instead of the nonexistent tests/.
There was a problem hiding this comment.
🟡 Changes recommended
Ruff’s selection omits the previously enabled T203 check for pprint debug output, so T203 must be added to preserve lint coverage.
1 open issue
What changed in this PR
This PR removes duplicate pre-commit execution from CI, keeps formatting checks in pre-commit.ci, and moves supported flake8-bugbear coverage into Ruff.
Changes:
- Narrows the GitHub Actions workflow to the manual-only
check-manifesthook and updates contributor guidance. - Removes flake8, expands Ruff’s rules, and corrects test-directory discovery.
- Renames shadowed loop variables and adds explicit exception chaining.
Tests and hooks were not run during this review.
| File | Description |
|---|---|
pyproject.toml |
Adds Ruff rules and corrects the test discovery path. |
hls4ml/writer/vivado_writer.py |
Renames a shadowed inner-loop variable. |
hls4ml/writer/libero_writer.py |
Renames a shadowed inner-loop variable. |
hls4ml/writer/coyote_writer.py |
Renames a shadowed inner-loop variable. |
hls4ml/writer/catapult_writer.py |
Renames a shadowed inner-loop variable. |
hls4ml/utils/plot.py |
Removes an unused index and clarifies input iteration. |
hls4ml/optimization/dsp_aware_pruning/knapsack.py |
Chains optional-dependency import errors explicitly. |
hls4ml/optimization/dsp_aware_pruning/keras/reduction.py |
Chains the Keras Surgeon import error explicitly. |
hls4ml/backends/altera/altera_backend.py |
Chains the missing-tool error explicitly. |
.pre-commit-config.yaml |
Removes the flake8 hook and its plugins. |
.github/workflows/check-manifest.yml |
Runs only manifest validation and removes the unused matrix. |
.agents/contributing-changes.md |
Documents Ruff and the split CI responsibilities. |
Review effort: Balanced
Added T203 to lint selection for improved code checks. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
JanFSchulte
left a comment
There was a problem hiding this comment.
A few comments inline. Two more that don't attach to lines in the diff:
- Leftover
# noqa: E741comments. With flake8 gone they do nothing, since ruff already ignores E741 globally throughlint.ignore. There are about 20 of them, indistributed_arithmetic.py,keras/hgq_proxy_model.py,keras_v3/hgq2/{_base,softmax,unary_lut}.py,keras_v3/pquant/_base.py,pytorch/pquant.pyandtest/pytest/test_softmax.py. They could be removed here, or left until someone enablesRUF100. - PR template. "I added tests under test/pytest covering this change" is ticked, but the Tests section says no test is added. Since this is an agent-assisted PR, please also confirm that the author ticked the attestation boxes themselves.
| lint.select = [ "E", "F", "F401", "I", "W" ] | ||
| lint.ignore = [ "E741" ] | ||
| # PLE0100, PLE0101, PLW0129 and RUF024 stand in for flake8-bugbear B037, B044 and B046, which ruff has no B code for | ||
| lint.select = [ "B", "E", "F", "F401", "I", "PLE0100", "PLE0101", "PLW0129", "RUF024", "T203", "W" ] |
There was a problem hiding this comment.
The description says nothing checks for the B045 loop-variable shadowing after this change. As far as I know, ruff's PLW2901 (redefined-loop-name) flags an inner for target that overwrites an outer loop variable, which is the bug class the writer renames in this PR fix. Could it be selected? It will also flag reassignments such as line = line.replace(...) in coyote_writer.py (around line 226), so those would need renaming too.
There was a problem hiding this comment.
Selected in 5e77481. On main's versions of the files, PLW2901 reports all five nested loops this PR
renames, so it covers what B045 caught.
It also reports 93 reassignments of a loop variable inside its own loop. 66 of them reassign line in the
writers' template loops, as in line = line.replace(...). All 93 are renamed:
- Usually the loop target gets a new name (
template_line,weight,item, ...) and the body keeps its
name, so each loop changes in one or two lines. - Where the reassignment was conditional, the result gets its own name, with the loop value as the default.
To check that nothing moved, I generated output before and after the renames and diffed it. That covered:
write()for Vivado, Vitis, Quartus, Altera, Catapult, Coyote, VivadoAccelerator, VitisUnified (io_stream),
XLS and Symbolic (Vivado HLS, Vitis HLS, libs path present and missing)- a Vitis multigraph and two PyTorch RNN models
- serialization
- the PyTorch and Keras weight and activation profiling
- the einsum and Vivado report helpers
The outputs are identical. The only differences are files that also differ between two runs of the unchanged
tree: pickled tensors in hls4ml_config.yml, the Catapult tarball's timestamp, and the flow order in the
serialized graph state.
Coverage shows every renamed statement ran except two:
- the
getitem-inside-a-tuple branch of the PyTorch RNN input parsing - the Libero build-script loop. Its template,
templates/libero/build_lib.sh, doesn't exist onmain, so
that write fails before and after this change.
| - id: pyupgrade | ||
| args: ["--py310-plus"] | ||
|
|
||
| - repo: https://github.com/pycqa/flake8 |
There was a problem hiding this comment.
Removing flake8 also removes pycodestyle E1xx/E2xx/E3xx and W391. In ruff those rules are preview-only, and E/W in lint.select enable only the stable ones. ruff-format fixes most of these, but not inside # fmt: off blocks or in comment spacing. The check that today's tree passes shows nothing breaks now, not that future code would still be caught. Is losing these acceptable, or should preview = true (or an explicit preview selection) be considered?
There was a problem hiding this comment.
They're back. lint.preview = true and lint.explicit-preview-rules = true are now set, and
lint.extend-select lists the 38 preview pycodestyle codes flake8 ran. flake8 skipped E203, E226, E241 and
E242 through its default and --extend-ignore lists, so those stay off. On the tree they report nothing.
Some details:
- Prefixes still select stable rules only. With
explicit-preview-rules, selections likeEandB
don't pick up preview rules. - Preview changes stable rules too.
lint.previewalso turns on preview behaviour for the stable rules.
That adds no findings today, but it can change with a ruff release. If it does, it will show up on
pre-commit.ci's quarterly autoupdate PR, not on unrelated PRs. - Rule codes in output. Preview makes ruff print rule names instead of codes.
output-prefer-rule-codes = trueundoes that.# noqa: <code>works either way. - Still not checked: E12x. pycodestyle's E12x continuation-line checks have no ruff implementation, stable
or preview. ruff-format covers them everywhere except inside# fmt: off.
| `.pre-commit-config.yaml` runs ruff (with `--fix`) and ruff-format, pyupgrade targeting Python 3.10, | ||
| pyproject-fmt, the standard whitespace and YAML hooks, and `p-clang-format` for C and C++ — so edited | ||
| `nnet_utils` headers are reformatted too. Ruff's rules live in `[tool.ruff]` in `pyproject.toml`: a | ||
| 125-character line limit plus the pycodestyle, pyflakes, isort and bugbear checks. |
There was a problem hiding this comment.
This overstates the coverage. Only the stable pycodestyle subset runs (see the comment on .pre-commit-config.yaml), and bugbear B028/B905 are ignored, with B045 having no ruff equivalent. Maybe something like "pyflakes, isort, the stable pycodestyle rules and most bugbear checks".
There was a problem hiding this comment.
Reworded to "pyflakes, isort, and most pycodestyle and bugbear checks". I also added a pointer to PLW2901,
since it now rejects the line = line.replace(...) idiom.
|
|
||
| The first run of a hook installs its own environment and needs network access. Commit the reformatting the | ||
| hooks apply; pull requests that fail these checks are not reviewed. | ||
| The first run of a hook installs its own environment and needs network access. Hook revisions are pinned, so |
There was a problem hiding this comment.
"Hook revisions are pinned, so a local run resolves to the same tools CI uses" is stronger than what pinning rev guarantees. Unpinned additional_dependencies and transitive dependencies resolve when the environment is built, and check-manifest builds the sdist under build isolation with whatever setuptools/setuptools_scm is current. I'd soften this or drop the sentence.
There was a problem hiding this comment.
Dropped. The paragraph is back to what main says.
Select PLW2901 and rename the 93 loop variables it reports, enable the preview-only pycodestyle checks flake8 ran, remove the E741 noqa comments only flake8 needed, and correct the contributing doc's description of ruff.
I did tick them myself, but erroneously ticked this one :) Also, I unchecked the AI credit tickbox because Copilot signed its autofix :| |
PLW2901, selected on this branch, flags n_out being reassigned when the call gives no output_size. The parser came in from main with fastmachinelearning#1597.

Description
Every pull request ran the full pre-commit suite twice, and the two runs can disagree:
Format (3.8)/ "Run pre-commit".github/workflows/pre-commit.ymlpre-commit run --hook-stage manual --all-files(18 hooks)pre-commit.ci - prci:block in.pre-commit-config.yaml--all-files(17 hooks)They diverged for two independent reasons.
The hook sets differ by design.
check-manifestis markedstages: [manual], so only the workflow ranit. pre-commit.ci runs default-stage hooks only, so it never did.
The flake8 plugins were unpinned.
additional_dependencies: [flake8-bugbear, flake8-print]carried noversions, and each service caches its own hook environment, built at a different time. flake8-bugbear
26.10.27 added
B045; the Actions job installed it on its next cache miss and began failing pull requestson pre-existing code, while pre-commit.ci kept passing from an environment cached before that release. #1537
is an example:
Format (3.8)failed with fiveB045hits in files the PR never touched, andpre-commit.ci - prpassed on the same commit.This PR keeps pre-commit.ci as the formatting gate, reduces the workflow to the single hook pre-commit.ci
cannot run, and removes the flake8 hook whose unpinned plugins caused the drift:
.github/workflows/pre-commit.ymlbecomes.github/workflows/check-manifest.ymland runs--hook-stage manual --all-files check-manifest. Itspython-version: [3.8]matrix is dropped — it hashad no effect since Fix precommits #741 removed the
setup-pythonstep it fed, so the check name advertised a Pythonversion the job never used.
The flake8 hook is removed, along with both plugins. Its checks move into ruff, as described below:
E,WandF, partly only in previewT203Pinning the two plugins would also have stopped the drift, but pre-commit.ci's
autoupdatebumpsrev:only, so the pins would have had to be moved by hand from then on.
Five nested loops reuse an enclosing loop's control variable; this is what
B045reported on Retire Quartus backend #1537. Theinner variables are renamed (
line→func_linein four writers,i→input_nameinmodel_to_dot).The
enumerateindex inmodel_to_dotwas only ever read by the inner loop that shadowed it, so it isdropped. Ruff has no
B045, butPLW2901, selected below, reports all five..agents/contributing-changes.mdis updated to describe the new split and no longer mentions flake8.Moving flake8's checks into ruff
I measured this rather than reading it off the rule tables. The tools were ruff 0.16.10 and flake8 7.4.1
with flake8-bugbear 26.10.27 and flake8-print 5.0.0, run over every tracked
.pyfile.configured on
main. No finding flake8 currently makes is lost.Badded reports 97 findings:B905×64,B028×29 andB904×4. The flake8 hook reportsnone of them, for two reasons:
B904andB905are among bugbear's opinionatedB9checks, which bugbear leaves off unless they areselected explicitly.
B028is on by default in bugbear, but bugbear only matcheswarnings.warn(...). Every hit here is abare
warn(...)imported withfrom warnings import warn, which ruff matches and bugbear does not.Bugbear, check by check. bugbear 26.10.27 has 47 checks enabled by default, not counting
B9. 35 ofthem have a stable ruff
Bcode. For each of the other 12, I wrote a snippet, confirmed that the flake8 hookflags it, and ran ruff on it:
B001except:E722, already selectedB041F601, already selectedB037returnoryieldin__init__PLE0101/PLE0100, added hereB045PLW2901, added hereB046dict.fromkeysRUF024, added hereB044asserton an always-true valuePLW0129(added) covers strings and f-strings,F631covers tuples; generators, lambdas, lists and dicts are not checkedB036except BaseExceptionwithout re-raisingB040add_note()that is never raisedB042__init__not passing all args tosuper()B043delattrwith a constant nameB047exceptclause unreachable after a broader oneB049@abstractmethodnot the innermost decoratorPLE0100,PLE0101,PLW0129andRUF024report nothing on the tree.PLW2901is broader thanB045. It also flags a loop variable reassigned inside its own loop, and itreported 93 of those. 66 of them reassign
linein the writers' template loops, as inline = line.replace(...). They are renamed (see thechanges below).
BLE001comes close toB036, but it also flagsexcept Exception(33 hits), so it is not added.pycodestyle. ruff has several of pycodestyle's checks only in preview, so
EandWalone do notselect them:
E11x)E2xx)E3xx)E502andW391They are restored with
lint.preview = true,lint.explicit-preview-rules = true, and the 38 codes flake8ran listed in
lint.extend-select. flake8's default and--extend-ignorelists leftE203,E226,E241and
E242off, so those stay off. The 38 report nothing on the tree.Side effects of turning on preview:
explicit-preview-ruleskeeps prefixes likeEandBfrom selecting other preview rules.lint.previewstill turns on preview behaviour for the stable rules. That adds no findings today. If aruff release changes it, the change surfaces on pre-commit.ci's quarterly autoupdate PR.
output-prefer-rule-codes = truereverts that.pycodestyle's
E12xcontinuation-line checks have no ruff implementation, stable or preview, and are nolonger checked. ruff-format enforces the layout they checked everywhere except inside
# fmt: off.flake8-print. The hook ignored only
T201, so flake8-print'sT202,T203andT204were on.T203(
pprint) is now selected in ruff. Ruff has no equivalent forT202(printreferenced without beingcalled) or
T204(a function or argument namedpprint), and neither occurs in the tree.The changes:
pyproject.toml,lint.select: adds the following:BPLW2901T203pyproject.toml, preview pycodestyle checks:lint.extend-select,lint.preview,lint.explicit-preview-rulesandoutput-prefer-rule-codes, as described above.pyproject.toml,lint.ignore: addsB028andB905, with a comment. Neither is a mechanical fix.B028, the rightstackleveldepends on how far eachwarn()sits from user code.B905,strict=Falseon all 64zip()calls would only be noise, whilestrict=Truechangesbehaviour wherever the lengths can differ.
Enabling either is a separate decision.
E741: the hook's per-file
E741ignore forbit_exact.pyneeds no carrying over, because the globallint.ignore = ["E741"]already covers it.# noqa: E741comments flake8 needed elsewhere are removed.utils/fixed_point_utils.py, lacked the colon, so both tools read them as blanketnoqa.B904fixes: the fourraisestatements insideexceptblocks now chain withfrom e:ahlslookup inAlteraBackend.buildkerassurgeonimport inreduce_modelortoolsimports inknapsack.pyThe original exception still appears in the traceback, which now says "direct cause" where it said
"during handling".
PLW2901renames: the 93 reassigned loop variables in 21 files are renamed.template_line,weight,item, ...) and the bodykeeps its name, so each loop changes in one or two lines.
default.
Ruff's file scope:
includelistedtests/**/*.py, but the tests live intest/; it now saystest/**/*.py. Under pre-commit this made no difference. Pre-commit passes file names, and ruff appliesonly
excludeto names it is given, so the 109 test files were already linted. A plainruff check .,however, skipped them.
Two scope differences from flake8 remain, both deliberate:
hls4ml/templates/**, so the threevivado_accelerator/*/python_drivers/axi_stream_driver.pyfiles that flake8 linted are no longer linted. They are clean under the new rules. Widening the scope
would also reformat all three, because the same
excludeapplies to ruff-format.docs/conf.py, which the flake8 hook excluded. It is clean.Type of change
io_type, Strategy, or config attributeAffected areas
Backends: local variable renames only, in the writers for every backend. Altera also gets exception
chaining in
AlteraBackend.build.SymbolicExpression, XLS (writers)
hls4ml.model): renames inLayer.__init__,Layer._validate_attributes,bit_exact.get_input_layers, profiling and serializationFrontends:
parse_pytorch_modelComponents:
hls4ml/templates/Configurations affected and exercised
No configuration axis is involved. The Python changes rename local variables and chain exceptions where a
missing tool or optional dependency is reported. Each row below is a project written with
write()on thecommit before the renames and again after them, then diffed.
io_typewrite()output identical before and afterwrite()output identical before and afterwrite()output andbit_exact.get_input_layersresults identicalwrite()output identical before and afterwrite()output identical before and afterwrite()output identical before and afterwrite()output identical before and afterwrite()fails onmaintoo, becausetemplates/libero/build_lib.shdoes not existNew configuration axis introduced by this PR (if any): none
Impact on generated HLS
Numerical behaviour: bit-exact vs
main. The generated projects in the table above are identical beforeand after the renames.
Tests
No test is added. The change is CI and lint configuration, plus edits that no test can distinguish from
main:__cause__set on exceptions raised when an optional dependency or tool is missing# noqacommentsTest configuration: pre-commit 4.6.0 on macOS 15 (Darwin 25.6.0); ruff 0.16.10, and flake8 7.4.1 under
Python 3.12 for the comparison above. The before/after check ran under Python 3.12.13 in a scratch
environment:
torch 2.15.0
keras 3.15.1 with the torch backend
quantizers 1.2.2. The current 2.0.0 removes
get_fixed_quantizer_np, soimport hls4mlfails with it, onmaintoo. bump quantizers to 2 #1604 adapts hls4ml to it.pre-commit run --all-files: every default-stage hook passes.pre-commit run --hook-stage manual --all-files:check-manifestcould not be checked locally. It failsfor reasons unrelated to this change:
.bak~file sits underhls4ml/templates/p-clang-formatrunssed -i.bakwithout removing the backups, so any all-files run leaves.bakfiles under
hls4ml/templates/thatcheck-manifestthen trips onThat last cause is not addressed here.
test/pytest/test_agent_docs.py: passes (38 passed, 1 skipped).Before/after comparison of the renames. I ran a script on the commit before the renames and again on
the change. It writes the projects in the table above and also exercises:
WeightsTorch,weights_kerasandactivations_kerasparse_einsumon invalid strings_make_vivado_tablescompare_dictsfromsynthesis_helpers.pyAll recorded results are identical. Of the 1816 files written, the only ones that differ are files that
also differ between two runs of the unchanged commit:
hls4ml_config.ymlgraph_state.jsonCoverage shows every renamed statement ran, with these exceptions:
getitem-inside-a-tuple branch inparse_pytorch_modeltest_graph.pyandtest/board/vitis_unified_hw_test.py, which need Keras with TensorFlow and a boardThe rest of the pytest suite was not run locally. The GitLab pipeline on this PR is the first run of it.
AI assistance disclosure
Tool(s) and model(s): Claude Code (Claude Opus 5 and Claude Opus 5.5). The
T203commit (e9fabcb) wasapplied from a Copilot review suggestion.
Where it was used: the whole change. That covers:
.pre-commit-config.yamlandpyproject.tomleditsB904fixes and thenoqaremovalsIf anything other than None is ticked, confirm all of the following:
it. I have read and understood every line and can explain and defend it
in review.
no invented APIs, config keys, pragmas or citations.
actually performed, not from agent output.
my knowledge it does not reproduce third-party code that would conflict
with it.
Checklist
Required:
pre-commiton the files I edited.test/pytestcovering this change (a bug fix shouldadd a test that fails on
mainand passes here).commented-out blocks or unrelated changes.
If applicable:
docs/updated.Release note