Skip to content

Replace the flake8 hook with ruff's bugbear rules - #1600

Closed
jmduarte wants to merge 2 commits into
fastmachinelearning:mainfrom
jmduarte:remove-flake8-hook
Closed

jmduarte wants to merge 2 commits into
fastmachinelearning:mainfrom
jmduarte:remove-flake8-hook

Conversation

@jmduarte

Copy link
Copy Markdown
Member

Description

Stacked on #1599. This branch contains #1599's commit; the change proposed here is the last commit,
a75ae54. Rebase onto main once #1599 merges. Opened as a draft until then.

This removes the flake8 hook from .pre-commit-config.yaml. The hook ran pycodestyle and pyflakes, which ruff
already runs as E, W and F, plus two plugins. flake8-print checks only T201, and that code was in
--extend-ignore. That left flake8-bugbear as the hook's only contribution, and since #1599 it has cost two
pins that must be moved by hand, because pre-commit.ci's autoupdate bumps rev: but never
additional_dependencies. This PR moves the bugbear coverage into ruff and drops the hook, its environment and
both pins.

What was compared

I measured this rather than reading it off the rule tables. The tools were the pinned versions, 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 .py file.

  • The flake8 hook as configured reports nothing on the tree, and neither does ruff as configured on
    main. No finding flake8 currently makes is lost.
  • Ruff with B added reports 97 findings: B905 ×64, B028 ×29 and B904 ×4. The flake8 hook reports
    none of them, for two reasons:
    • B904 and B905 are among bugbear's opinionated B9 checks, which bugbear leaves off unless they are
      selected explicitly.
    • B028 is on by default in bugbear, but bugbear only matches warnings.warn(...). Every hit here is a
      bare warn(...) imported with from warnings import warn, which ruff matches and bugbear does not.

Coverage rule by rule: bugbear 26.10.27 has 47 checks enabled by default, not counting B9. 35 of them have a
stable ruff B code. For each of the other 12, I wrote a snippet, confirmed that the flake8 hook flags it,
and ran ruff on it:

bugbear check after this PR
B001 bare except: caught by E722, already selected
B041 repeated key-value pair in a dict literal caught by F601, already selected
B037 return or yield in __init__ caught by PLE0101 / PLE0100, added here
B046 mutable value passed to dict.fromkeys caught by RUF024, added here
B044 assert on an always-true value partly: PLW0129 (added) covers strings and f-strings, F631 covers tuples; generators, lambdas, lists and dicts are not checked
B036 except BaseException without re-raising not checked
B045 inner loop reuses an enclosing loop's variable not checked
B040 exception with add_note() that is never raised not checked
B042 exception __init__ not passing all args to super() not checked
B043 delattr with a constant name not checked (ruff has it only in preview)
B047 except clause unreachable after a broader one not checked
B049 @abstractmethod not the innermost decorator not checked

The four codes added in the third column report nothing on the tree. Ruff does have broader rules close to
B036 and B045, but neither is a like-for-like replacement, so neither is added:

  • BLE001 reports 33 hits here, because it also flags except Exception.
  • PLW2901 reports 93 hits, because it also flags reassigning a loop variable inside its own body.

B045 is the one with a recent track record: #1599 fixed five instances of it. After this PR, nothing checks
for it.

Changes

  • pyproject.toml, lint.select: adds B and the four stand-in codes above.

  • pyproject.toml, lint.ignore: adds B028 and B905, with a comment. Neither is a mechanical fix.

    • For B028, the right stacklevel depends on how far each warn() sits from user code.
    • For B905, strict=False on all 64 zip() calls would only be noise, while strict=True changes
      behaviour wherever the lengths can differ.

    Enabling either is a separate decision.

  • E741 ignore: the hook's per-file E741 ignore for bit_exact.py needs no carrying over, because the
    global lint.ignore = ["E741"] already covers it.

  • B904 fixes: the four raise statements inside except blocks now chain with from e:

    • the ahls lookup in AlteraBackend.build
    • the kerassurgeon import in reduce_model
    • two ortools imports in knapsack.py

    The original exception still appears in the traceback, which now says "direct cause" where it said
    "during handling".

  • Ruff's file scope: include listed tests/**/*.py, but the tests live in test/; it now says
    test/**/*.py. Under pre-commit this made no difference. Pre-commit passes file names, and ruff applies
    only exclude to names it is given, so the 109 test files were already linted. A plain ruff check .,
    however, skipped them.

  • Two scope differences from flake8 remain, both deliberate:

    • Ruff skips hls4ml/templates/**, so the three vivado_accelerator/*/python_drivers/axi_stream_driver.py
      files 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 exclude applies to ruff-format.
    • Ruff lints docs/conf.py, which the flake8 hook excluded. It is clean.
  • .agents/contributing-changes.md: no longer describes a flake8 hook or pinned plugins, and the cleanup
    sweep no longer cites flake8-print.

Type of change

  • Bug fix (non-breaking)
  • New feature (non-breaking)
  • Breaking change (existing configs, APIs or generated code behave differently)
  • New frontend / backend
  • New layer / operator support
  • New configuration option — a new io_type, Strategy, or config attribute
  • Research paper implementation
  • Documentation
  • Build, CI or tooling
  • Refactor / cleanup (no functional change)

Affected areas

Backends: Altera (exception chaining in AlteraBackend.build only)

  • Altera
  • Backend-independent (core IR, optimizer, hls4ml.model)

Frontends:

  • Not related to a frontend

Components:

  • IR (layers + model graph) / optimizer passes
  • C++ templates / HLS sources under hls4ml/templates/
  • Profiling, reporting or the CLI
  • Packaging, build or CI

Configurations affected and exercised

No configuration axis is involved.

Backend io_type Strategy Verified
n/a n/a n/a pre-commit (default stage)

New configuration axis introduced by this PR (if any): none

Impact on generated HLS

  • This PR does not change the generated HLS for existing models.
  • This PR changes the generated HLS. Numbers below.

Numerical behaviour: bit-exact vs main. No code on the conversion path is touched.

Tests

No test is added. Most of the change is lint configuration. The B904 edits only set __cause__ on
exceptions that are raised when an optional dependency or tool is missing.

Test configuration: macOS (Darwin 25.6.0), pre-commit 4.6.0, ruff 0.16.10, and flake8 7.4.1 under Python
3.12 for the comparison.

  • pre-commit run --all-files: every default-stage hook passes.
  • pre-commit run --hook-stage manual --all-files: check-manifest fails locally for environment
    reasons
    . Four submodules are uninitialised (hls4ml/contrib/Coyote, hls4ml/templates/catapult/ac_*),
    and an untracked hls4ml/templates/vitis/nnet_utils/nnet_conv1d_latency.h.bak~ predates this change.
    This PR adds, moves and renames no files.
  • Separately: p-clang-format runs sed -i.bak and does not remove the backups. Any all-files run therefore
    leaves .bak files under hls4ml/templates/, and check-manifest then fails on them locally. That is not
    addressed here.
  • test/pytest/test_agent_docs.py: passes (38 passed, 1 skipped).
  • The rest of the pytest suite was not run locally.

AI assistance disclosure

  • None — no AI tool was used.
  • Assisted — completion, refactoring, docstrings, tests; design and code are mine.
  • Substantial — significant AI-generated portions, reviewed and edited by me.
  • Agentic — produced largely end-to-end by an AI agent from my prompts.

Tool(s) and model(s): Claude Code (Claude Opus 5.5)

Where it was used: the whole change. That covers the flake8 and ruff comparison and the per-rule probes, the
pyproject.toml and .pre-commit-config.yaml edits, the four B904 fixes, the agent-doc update and this
description.

If anything other than None is ticked, confirm all of the following:

  • I am the author of this contribution and take full responsibility for
    it.
    I have read and understood every line and can explain and defend it
    in review.
  • I verified the generated code against real hls4ml and HLS semantics —
    no invented APIs, config keys, pragmas or citations.
  • All numbers, logs and test results quoted in this PR come from runs I
    actually performed, not from agent output.
  • I have the right to submit this work under the project's licence, and to
    my knowledge it does not reproduce third-party code that would conflict
    with it.
  • No AI tool is credited as an author in any commit in this branch.

Checklist

Required:

  • I have read the contributing guidelines.
  • I installed and ran pre-commit on the files I edited.
  • I added tests under test/pytest covering this change (a bug fix should
    add a test that fails on main and passes here).
  • I self-reviewed the full diff and it contains no leftover debug code,
    commented-out blocks or unrelated changes.
  • The AI assistance disclosure above is complete and accurate.

If applicable:

  • Documentation under docs/ updated.
  • No new build or synthesis warnings.
  • Public API / config schema changes are documented.

Release note

none

🤖 Generated with Claude Code

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/.
@jmduarte

Copy link
Copy Markdown
Member Author

Folded into #1599: its branch now carries this commit (a75ae54), and its description covers the flake8 → ruff comparison.

@jmduarte jmduarte closed this Oct 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant