Skip to content

Modernize linting stack: migrate to Ruff and streamline pre-commit hooks#614

Open
Shivampal157 wants to merge 4 commits intomllam:mainfrom
Shivampal157:chore/ruff-migration-601
Open

Modernize linting stack: migrate to Ruff and streamline pre-commit hooks#614
Shivampal157 wants to merge 4 commits intomllam:mainfrom
Shivampal157:chore/ruff-migration-601

Conversation

@Shivampal157
Copy link
Copy Markdown

Describe your changes

Switched the lint/format setup to Ruff and cleaned up pre-commit accordingly.

I removed the separate black, isort, and flake8 hooks and replaced them with Ruff (ruff-check + ruff-format). I also added a few small pre-commit checks we discussed in #601:

  • pyproject-fmt
  • python-check-blanket-noqa
  • no-commit-to-branch (main)

I kept the stricter rule families configured but with targeted ignores where the repo is not ready yet, so we can tighten them in follow-up PRs without blocking contributors now.

I then ran pre-commit on all files and fixed what was needed so the hook suite is green.

Dependencies required for this change:

  • none at runtime
  • pre-commit now uses Ruff/pyproject-fmt/pygrep-hooks instead of black/isort/flake8 hooks

Issue Link

Part of #601

Type of change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 💥 Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • 📖 Documentation (Addition or improvements to documentation)

Checklist before requesting a review

  • My branch is up-to-date with the target branch - if not update your fork with the changes from the target branch (use pull with --rebase option if possible).
  • I have performed a self-review of my code
  • For any new/modified functions/classes I have added docstrings that clearly describe its purpose, expected inputs and returned values
  • I have placed in-line comments to clarify the intent of any hard-to-understand passages of my code
  • I have updated the README to cover introduced code changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have given the PR a name that clearly describes the change, written in imperative form (context).
  • I have requested a reviewer and an assignee (assignee is responsible for merging). This applies only if you have write access to the repo, otherwise feel free to tag a maintainer to add a reviewer and assignee.

Checklist for reviewers

Each PR comes with its own improvements and flaws. The reviewer should check the following:

  • the code is readable
  • the code is well tested
  • the code is documented (including return types and parameters)
  • the code is easy to maintain

Author checklist after completed review

  • I have added a line to the CHANGELOG describing this change, in a section
    reflecting type of change (add section where missing):
    • added: when you have added new functionality
    • changed: when default behaviour of the code has been changed
    • fixes: when your contribution fixes a bug
    • maintenance: when your contribution is relates to repo maintenance, e.g. CI/CD or documentation

Checklist for assignee

  • PR is up to date with the base branch
  • the tests pass
  • (if the PR is not just maintenance/bugfix) the PR is assigned to the next milestone. If it is not, propose it for a future milestone.
  • author has added an entry to the changelog (and designated the change as added, changed, fixed or maintenance)
  • Once the PR is ready to be merged, squash commits and merge the PR.

@sadamov sadamov self-requested a review April 20, 2026 18:11
@sadamov sadamov self-assigned this Apr 20, 2026
@sadamov sadamov added the cicd label Apr 20, 2026
@sadamov sadamov added this to the v0.8.0 (proposed) milestone Apr 20, 2026
@Shivampal157 Shivampal157 force-pushed the chore/ruff-migration-601 branch from 096e65b to d62a12f Compare April 20, 2026 19:02
@Shivampal157
Copy link
Copy Markdown
Author

@sadamov ,thanks for assigning this.
I’ve kept this PR to the first part of #601 only:

moved black/isort/flake8 to Ruff
updated pre-commit hooks
added the small safety hooks from the issue

I also cleaned the branch so only these toolchain/config changes are in this PR.
If this looks good, I’ll do the rule-tightening part in follow-up PRs.

Copy link
Copy Markdown
Collaborator

@sadamov sadamov left a comment

Choose a reason for hiding this comment

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

Thanks! Overall direction is good. Please run the re-commits and pytest locally they are currently broken.

Old tool Replaced by Notes
black ruff-format line-length 80 preserved
isort ruff I rules import sorting only; heading comments not enforced (see below)
flake8 E/F/W ruff E/F/W same rules; E203 and F401/__init__.py ignores preserved
flake8 W503 not needed ruff formatter enforces one style; rule not implemented
flake8 I002 not needed flake8-isort-specific; no equivalent in ruff

New rule families added that were not in the old setup: G (logging format), PT
(pytest style), PTH (pathlib), S (bandit), TCH (type-checking imports), UP
(pyupgrade). Most are gated behind per-rule ignores for incremental tightening.


TB Fixed

1. Dangling # Pylint config comment

pyproject-fmt moved the pylint sections up but left the preceding comment orphaned at
the very bottom of the file after [tool.codespell]. Delete

2. pylint ignore pointed at non-existent file

-  "create_mesh.py", # Disable linting for now, as major rework is planned/expected
+  "create_graph.py", # Disable linting for now, as major rework is planned/expected

create_mesh.py does not exist; Bug predates this PR

3. isort known-first-party made explicit

Added [tool.ruff.lint.isort] with known-first-party = ["neural_lam", "tests"].
Ruff auto-detects these when run from the project root, but being explicit avoids
surprises when ruff is invoked in other contexts (CI steps, editors, /tmp test files).

The old isort config also enforced import section heading comments (# Standard library, # Third-party, etc.) via import_heading_*. Ruff supports the equivalent
(import-heading.*) but enabling it today would fail on a stray blank line within the
third-party block in several test files. Fix and activate the rule please.

4. Python 3.13 classifier needs CI coverage

pyproject-fmt auto-injected "Programming Language :: Python :: 3.13". No CI matrix
currently tests against 3.13. Either add a 3.13 job to the workflow or remove the
classifier.

4. CHANGELOG entry missing

Needs a maintenance entry before merge.

5. Branch not up to date with main

Should be rebased before merge.


@GiGiKoneti
Copy link
Copy Markdown
Contributor

GiGiKoneti commented Apr 22, 2026

Hey @Shivampal157, thanks for picking this up! I was actually gonna grab this issue but my thoughts are the same which u did.

  1. Pylint & Mesh fixes: Just delete that orphaned pylint comment at the very bottom of pyproject.toml, and change "create_mesh.py" to "create_graph.py" in the ignore list.

  2. Fixing the imports: Add these to your [tool.ruff.lint.isort] block in pyproject.toml:

   import-heading-stdlib = "Standard library"
   import-heading-thirdparty = "Third-party"
   import-heading-firstparty = "First-party"

Then just run ruff check --select I --fix in ur terminal. Ruff should magically remove those stray blank lines in the tests and fix all the headers for you!

  1. Python 3.13: The easiest path is to just delete the Programming Language :: Python :: 3.13 classifier from pyproject.toml so you dont have to wire up a new CI job.

  2. Wrap up: Add a quick note in CHANGELOG.md under maintenance, run git pull --rebase upstream main, and you should be good to go!

Lmk if the Ruff auto-fix gives you any trouble!

cc : @sadamov

Copy link
Copy Markdown
Contributor

@kshirajahere kshirajahere left a comment

Choose a reason for hiding this comment

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

@sadamov covered most of the things about the broken pre-commits ;). Leaving one comment about a missing dependency

Comment thread pyproject.toml
dynamic = [ "version" ]
# PEP 621 project metadata
# See https://www.python.org/dev/peps/pep-0621/
dependencies = [
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.

pillow disappeared from project.dependencies, but neural_lam/custom_loggers.py and neural_lam/models/ar_model.py still import PIL.Image. This turns a tooling-only PR into a packaging regression for fresh install

Replace separate black/isort/flake8 hooks with Ruff, add pyproject formatting plus explicit-noqa and branch-protection hooks, and keep phased rule ignores so we can tighten enforcement incrementally.

Made-with: Cursor
Restore the missing pillow runtime dependency, align pylint ignore and Ruff isort settings with reviewer guidance, and apply the resulting lint-driven cleanup so the updated pre-commit stack passes consistently.

Made-with: Cursor
@Shivampal157 Shivampal157 force-pushed the chore/ruff-migration-601 branch from c95705f to f2df692 Compare April 22, 2026 17:31
@Shivampal157
Copy link
Copy Markdown
Author

@sadamov @kshirajahere addressed the requested updates and pushed the latest changes: switched pylint ignore to create_graph.py, added Ruff isort config (known-first-party, import-heading), replaced blanket # noqa with explicit suppressions, restored pillow dependency, added the changelog entry, rebased on latest main, resolved the tests/test_plotting.py conflict, and re-ran pre-commit --all-files (passing). please let me know if you’d like any further changes.

@sadamov sadamov self-requested a review April 23, 2026 03:41
Upgrade pyproject-fmt to a current release and apply its normalized pyproject formatting so the 3.14 pre-commit job no longer crashes and runs consistently across the matrix.

Made-with: Cursor
Copy link
Copy Markdown
Collaborator

@sadamov sadamov left a comment

Choose a reason for hiding this comment

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

Thanks @Shivampal157. One item still open from the previous review, see inline.

Comment thread pyproject.toml Outdated
"UP045", # Optional/Union modernization deferred to follow-up cleanup
]
lint.per-file-ignores."__init__.py" = [ "F401" ]
lint.isort.import-heading = { first-party = "First-party", standard-library = "Standard library", third-party = "Third-party" }
Copy link
Copy Markdown
Collaborator

@sadamov sadamov Apr 24, 2026

Choose a reason for hiding this comment

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

The old isort had import_heading_localfolder = "Local", which enforces the # Local marker before relative imports in ~18 modules. The new config covers first-party/standard-library/third-party but not local-folder, so # Local headings go unenforced.

Suggested change
lint.isort.import-heading = { first-party = "First-party", standard-library = "Standard library", third-party = "Third-party" }
lint.isort.import-heading = { first-party = "First-party", local-folder = "Local", standard-library = "Standard library", third-party = "Third-party" }

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@sadamov thanks for pointing that out - I’ve now added local-folder = "Local" to the Ruff isort import-heading config in pyproject.toml and pushed the update

@sadamov sadamov added the ready Review complete - proposed for milestone label Apr 24, 2026
@sadamov sadamov self-requested a review April 24, 2026 10:54
Copy link
Copy Markdown
Collaborator

@sadamov sadamov left a comment

Choose a reason for hiding this comment

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

please fix the final suggestions above then we wait for next dev-meeting

Include the Local section heading in Ruff isort import-heading config so existing relative-import heading conventions remain enforced.

Made-with: Cursor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cicd ready Review complete - proposed for milestone

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants