Skip to content

ci(test): OPS-9049 replace flake8 with ruff and upgrade the ruff hook to 0.17.3 - #17536

Merged
dmitry-tokarev-nv merged 6 commits into
mainfrom
dtokarev/ops-9049-remove-flake8-hook
Oct 29, 2026
Merged

dmitry-tokarev-nv merged 6 commits into
mainfrom
dtokarev/ops-9049-remove-flake8-hook

Conversation

@dmitry-tokarev-nv

@dmitry-tokarev-nv dmitry-tokarev-nv commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Linear: OPS-9049

Overview

The flake8 hook takes 17 to 27 s of each CI pre-commit run, but it enforces nothing. YAML splits its args list at every comma, so flake8 receives only --select=C. This PR removes the hook. Ruff is then the only Python linter, so this PR also moves the ruff hook from 0.5.2 to 0.17.3. It pins the old rule set, so nothing that passed before fails now, and it adds two rules that have no findings on main.

Key Changes

Core logic: none. The change is configuration only.

  • .pre-commit-config.yaml: the flake8 hook is gone. ruff-pre-commit moves from v0.5.2 to v0.17.3, and the hook id from ruff to ruff-check. In v0.17.3, ruff is a legacy alias.
  • pyproject.toml: [tool.ruff.lint] select = ["E4", "E7", "E9", "F", "W605", "PLW0602"]. The first four are the default rule set of ruff 0.5.2. W605 flags invalid escape sequences, and PLW0602 flags a global name that is never assigned. A TODO records the next step: enable flake8-bugbear (B) after its findings on main are fixed.
  • .ai/python-guidelines.md, a docstring in docs/fern/scripts/tests/test_gen_python_api.py, and a comment in lib/bindings/python/codegen/src/gen_python_prometheus_names.rs mentioned flake8. They now name ruff or drop the mention.
  • lib/bindings/kvbm/python/kvbm/__init__.py: the file-level # flake8: noqa becomes # ruff: noqa, which ruff treats the same way. The kvbm package is deprecated, so this PR does not clean it up.
  • lib/bindings/python/src/dynamo/llm/exceptions.py: the file-level directive is removed, because it suppressed nothing. Ruff now checks that file.
  • lib/bindings/python/src/dynamo/llm/__init__.py: the file-level directive is removed, and the 5 F401 findings it hid are fixed. Four are public re-exports (EngineType, make_engine, run_input, HttpError), now written as X as X like the other re-exports in the file. The fifth, import logging, was unused and is gone.

Validation

flake8 enforced nothing

The args line was [--max-line-length=88, --select=C,E,F,W,B,B950, --extend-ignore = E203,E501]. PyYAML, which pre-commit uses, parses it into --select=C, E, F, W, B, B950, --extend-ignore = E203, and E501. With only C selected and no --max-complexity, flake8 has nothing to report. The line has not changed since df90e29eba.

Check before this PR Result
pre-commit run flake8 on a planted unused import and a planted syntax error Passed
pre-commit run ruff on the same two files Failed: it reports the syntax error and fixes the unused import
flake8 7.3.0 with the parsed arguments on all 1,947 .py files of main a5c73615d0 exit 0, no output
flake8 with the intended arguments on the two planted files E999 and F401

Fixing the arguments instead is not a real option. The intended rules flag about 3,250 lines on main, and 2,758 of them are W503, which conflicts with black. In the rule families that ruff also covers (F, E4, E7, E9), they flag 149 more lines than ruff. All 149 follow conventions that ruff accepts on purpose. Of the 123 late imports, 110 follow pytest.importorskip and 13 follow other allowed setup code. The other 26 are 12 import X as X re-exports, 7 unused names that start with _, 6 duplicate imports, and 1 import inside a loop.

The ruff upgrade does not change enforcement

Ruff run on the 1,958 files the hook checks Result
0.5.2, the old hook 60 rules, exit 0, no findings
0.17.3 with the repository configuration before this PR 407 rules by default, 9,238 findings (2,991 UP045, 1,313 UP006, 699 I001, 569 BLE001, and more)
0.17.3 with the pinned select 59 rules, exit 0, no findings

The only rule that drops out is E999. Newer ruff removed it as a rule, because it always reports syntax errors. No nested pyproject.toml has a [tool.ruff] table, so the root pin applies to the whole repository.

The upgraded hook, run through pre-commit:

  • It passes on all files.
  • It fails a planted unused import (and fixes it) and a planted syntax error.
  • It passes a 116-character line, so it does not select E501.
  • It fails an import after code with E402.

These results also keep the two rewritten sentences true. pre-commit run --all-files passes locally with 40 hooks.

Two added rules and a TODO

These two rules close the gaps between the intended flake8 rules and ruff that cost nothing today:

Rule Covers Findings on main Planted bad case Planted good case
W605 pycodestyle W605, an invalid escape sequence such as "\d+" 0 fails, and the hook fixes it r"\d+" passes
PLW0602 pyflakes F824, which ruff does not have: a global name that the function never assigns 0 fails an assigned global passes

The TODO is for flake8-bugbear (B), which never ran here because the hook never installed the plugin. On main, ruff check --select B reports 254 findings, among them 23 B023 (a closure that captures a loop variable). Those need their own PR. Ruff has no B950, so the TODO names E501 (line too long) as the closest ruff rule.

The file-level noqa directives

Ruff skips a file that has either # flake8: noqa or # ruff: noqa. Without a directive, ruff reports 4 E402 in the kvbm __init__.py, 5 F401 in dynamo/llm/__init__.py, and nothing in exceptions.py.

  • kvbm __init__.py keeps a whole-file # ruff: noqa, because the package is deprecated.
  • exceptions.py loses its directive, and the ruff, black, and isort hooks pass on it.
  • dynamo/llm/__init__.py loses its directive, and ruff 0.17.3 reports no findings after the fixes. The four re-exports have users: for example, components/src/dynamo/mocker/main.py imports EngineType, make_engine, and run_input from dynamo.llm, and components/src/dynamo/common/utils/guided_json.py imports HttpError. Nothing in the repository uses dynamo.llm.logging. An AST comparison shows that the module binds the same 61 names as before, without logging.

A local import of dynamo.llm was not possible: the compiled dynamo._core on hand predates main's AisPerfConfig, and the original file fails the same way there. The CI test jobs build dynamo._core from this PR and import these names.

CI

This PR edits .pre-commit-config.yaml, so its own pre-commit job starts with an empty hook cache. Such cold runs took 152 to 189 s on 2026-10-28, which is close to the 3-minute step limit that #17532 raises. The cold job 123074777938 of the first commit passed with a 122 s step, and flake8 does not appear in its log. Part of that gap comes from a faster runner: black took 34 s there, against 47 to 55 s in the other runs. The cold job 123083739885 of the second commit passed with a 107 s step, and its ruff check hook ran ruff 0.17.3 and passed.

Issues and Ongoing Work

Pull Request Checklist
  • I used Dynamo's /pr-checklist skill.

🤖 Generated with Claude Code

The flake8 hook passed its arguments as a YAML flow list. YAML splits a
flow list at every comma, so flake8 received --select=C alone, with E,
F, W, B, B950, and E501 as loose arguments. With no --max-complexity, it
reported nothing. It passed a planted syntax error and a planted unused
import, and it took 17 to 27 s of each CI pre-commit run.

The ruff hook already reports both of those problems. Remove the flake8
hook, and update the guideline, test docstring, and generator comment
that mention it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Dmitry Tokarev <dtokarev@nvidia.com>
@dmitry-tokarev-nv
dmitry-tokarev-nv requested review from a team as code owners October 28, 2026 20:15
@dmitry-tokarev-nv
dmitry-tokarev-nv requested a review from a team as a code owner October 28, 2026 20:15
@github-actions github-actions Bot added ci Issues/PRs that reference CI build/test documentation Improvements or additions to documentation labels Oct 28, 2026
@coderabbitai

coderabbitai Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between 8a9517c and 8bcb684.

Files selected for processing: 4
  • .ai/python-guidelines.md
  • .pre-commit-config.yaml
  • docs/fern/scripts/tests/test_gen_python_api.py
  • lib/bindings/python/codegen/src/gen_python_prometheus_names.rs
Files with no reviewable changes: 1
  • .pre-commit-config.yaml

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.

Walkthrough

The pre-commit configuration no longer includes the Flake8 hook. Related documentation and comments now refer to Ruff or no longer list Flake8.

Changes

Python lint references

Layer / File(s) Summary
Remove Flake8 references
.pre-commit-config.yaml, .ai/python-guidelines.md, docs/fern/scripts/tests/test_gen_python_api.py, lib/bindings/python/codegen/src/gen_python_prometheus_names.rs
The pre-commit configuration removes the Flake8 hook. The guidelines and code comments remove or replace Flake8 references with Ruff.

Priority: Low

Estimated code review effort: 1 (Trivial) | ~5 minutes

Merge Risk: Minimal · up to 8bcb6

The PR removes the Flake8 hook and updates its documentation references without an identified merge-blocking issue.

Pre-merge checks: Passed 5
Check name Status Explanation
Docstring Coverage Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description check Passed The pull request description includes all required sections: Overview, Key Changes, Validation, Issues and Ongoing Work, issue closure, and the checklist. It provides detailed technical context and va…
Title check Passed The title names the two changes the PR makes: replacing flake8 with ruff in the pre-commit setup, and upgrading the ruff hook to 0.17.3. The changeset removes the flake8 pre-commit hook and updates re…
Full details: Docstring Coverage

Explanation

Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 2 files. (1 skipped: 1 unsupported.)

Full details: Description check

Explanation

The pull request description includes all required sections: Overview, Key Changes, Validation, Issues and Ongoing Work, issue closure, and the checklist. It provides detailed technical context and validation results. The checklist item remains unchecked, but this is non-critical for the description check.

Full details: Title check

Explanation

The title names the two changes the PR makes: replacing flake8 with ruff in the pre-commit setup, and upgrading the ruff hook to 0.17.3. The changeset removes the flake8 pre-commit hook and updates references to flake8 to name ruff or omit it, which matches the first part of the title. The changeset summaries do not mention the ruff version upgrade to 0.17.3, so that part of the title is not visible in the provided file summaries. The title's main point is the flake8-to-ruff replacement, so it is related to the main change.

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

The hook pinned ruff-pre-commit v0.5.2. Move it to v0.17.3, and to the
hook id ruff-check, because ruff is now the only Python linter. In
v0.17.3 the old id ruff is a legacy alias.

Under the repository configuration, ruff 0.17.3 enables 407 rules by
default, against 60 for 0.5.2. With them it reports 9,238 findings on
main. Pin select to E4, E7, E9, and F, the old default set, so that
enforcement does not change. The only rule that drops out is E999,
because newer ruff always reports syntax errors.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Dmitry Tokarev <dtokarev@nvidia.com>
@dmitry-tokarev-nv dmitry-tokarev-nv changed the title ci(test): OPS-9049 remove the flake8 pre-commit hook, which enforces nothing ci(test): OPS-9049 remove the flake8 hook and upgrade the ruff hook to 0.17.3 Oct 28, 2026
dmitry-tokarev-nv and others added 2 commits October 28, 2026 16:43
W605 flags invalid escape sequences. PLW0602 flags a global name that a
function never assigns, which is ruff's version of pyflakes F824. Ruff
has no F824. Neither rule has findings on main.

Add a TODO to enable flake8-bugbear (B) after its findings on main are
fixed. Ruff has no B950, and E501 is the closest ruff rule.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Dmitry Tokarev <dtokarev@nvidia.com>
Three files skip linting with "# flake8: noqa". Ruff honors that
directive too, but flake8 no longer runs, so the name points to a
removed tool. Rename the three directives to "# ruff: noqa", which ruff
treats the same way.

Without a directive, ruff reports 4 E402 in kvbm/__init__.py and 5 F401
in dynamo/llm/__init__.py, so two of the directives still do work.
exceptions.py has no findings either way.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Dmitry Tokarev <dtokarev@nvidia.com>
@dmitry-tokarev-nv
dmitry-tokarev-nv requested a review from a team as a code owner October 28, 2026 20:47
@pull-request-size pull-request-size Bot added size/M and removed size/S labels Oct 28, 2026
dmitry-tokarev-nv and others added 2 commits October 28, 2026 16:49
The file-level "# ruff: noqa" in exceptions.py suppressed nothing: ruff
reports no findings for the file without it. Remove it, so that ruff
checks the file like any other.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Dmitry Tokarev <dtokarev@nvidia.com>
Remove the file-level "# ruff: noqa" from dynamo/llm/__init__.py, and
fix the 5 F401 findings that it hid. Four of them are public re-exports
(EngineType, make_engine, run_input, HttpError) that other modules
import from dynamo.llm. Write them as "X as X", like the other
re-exports in the file. The fifth, "import logging", is unused, so
remove it. The module binds the same names as before, except logging.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Dmitry Tokarev <dtokarev@nvidia.com>
@devin-ai-integration

Copy link
Copy Markdown
Contributor

✅ Dynamo PR CI passed — run 40767121576 (attempt 1) on 9daa76e83b

Gate checks: ✅ backend-status-check · ✅ deploy-status-check · ✅ dynamo-status-check

Posted automatically by Devin for run 40767121576. Updated on every full-CI run of this PR.

@dmitry-tokarev-nv dmitry-tokarev-nv changed the title ci(test): OPS-9049 remove the flake8 hook and upgrade the ruff hook to 0.17.3 ci(test): OPS-9049 replace flake8 with ruff and upgrade the ruff hook to 0.17.3 Oct 29, 2026

@harryskim harryskim left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs changes LGTM.

@dmitry-tokarev-nv
dmitry-tokarev-nv enabled auto-merge (squash) October 29, 2026 18:16
@dmitry-tokarev-nv
dmitry-tokarev-nv merged commit eeda105 into main Oct 29, 2026
131 of 132 checks passed
@dmitry-tokarev-nv
dmitry-tokarev-nv deleted the dtokarev/ops-9049-remove-flake8-hook branch October 29, 2026 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci Issues/PRs that reference CI build/test documentation Improvements or additions to documentation size/M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants