Skip to content

Simplify ruff configuration #41259

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
vincentmacri wants to merge 18 commits into
base: develop
Choose a base branch
from
+262 −129
Open

Simplify ruff configuration #41259

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
a99f822
Cleanup ruff configuration
vincentmacri Dec 3, 2025
feea4ab
Fixes
vincentmacri Dec 3, 2025
8025e2e
Tweak workflow
vincentmacri Dec 3, 2025
2f00d98
Skip preview check if no preview failed
vincentmacri Dec 3, 2025
e44886c
Cleanup
vincentmacri Dec 3, 2025
5e4de08
Add more rules to CI ignore
vincentmacri Dec 3, 2025
572ebf9
Fix PLR1711
vincentmacri Dec 3, 2025
0024305
Fix ruff PLR1733
vincentmacri Dec 4, 2025
2202a87
Ruff tweaks
vincentmacri Dec 4, 2025
92cf8f1
Fix a few more preview rules
vincentmacri Dec 4, 2025
a8e04e8
Print ruff version
vincentmacri Dec 4, 2025
9b2924c
List all rules manually
vincentmacri Dec 4, 2025
2519d31
Remove debugging rule
vincentmacri Dec 4, 2025
3b467a7
TOML formatting
vincentmacri Dec 4, 2025
a58fcf9
Space
vincentmacri Dec 4, 2025
9858891
Remove ruff from tox, update developer tools docs
vincentmacri Dec 6, 2025
e1f8a9b
Formatting
vincentmacri Dec 8, 2025
fe61df1
Formatting
vincentmacri Dec 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 5 additions & 7 deletions .github/sync_labels.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ def is_pull_request(self):

def reset_view(self):
r"""
Reset cache of ``gh view`` results.
Reset cache of ``gh view`` results.
"""
self._labels = None
self._author = None
Expand All @@ -187,7 +187,7 @@ def rest_api(self, path_args, method=None, query=''):
"""
meth = '-X GET'
if method:
meth='-X %s' % method
meth = '-X %s' % method
cmd = 'gh api %s -H \"Accept: application/vnd.github+json\" %s %s' % (meth, path_args, query)
debug('Execute command: %s' % cmd)
if method:
Expand Down Expand Up @@ -248,6 +248,7 @@ def bot_login(self):
outtxt = str(capt.stdout)
debug('auth status err: %s' % errtxt)
debug('auth status out: %s' % outtxt)

def read_login(txt, position_mark):
for t in txt:
for p in position_mark:
Expand Down Expand Up @@ -413,7 +414,7 @@ def get_commits(self):
date_commits = list(self._commits)
if Status.positive_review.value not in self.get_labels():
for com in self._commits:
message = com['messageHeadline']
message = com['messageHeadline']
if message.startswith('Merge') and 'develop' in message:
debug('Ignore merge commit %s for commit_date' % com['oid'])
date_commits.remove(com)
Expand Down Expand Up @@ -796,7 +797,6 @@ def reject_label_addition(self, item):
if item is Status.positive_review:
self.add_warning('Label *%s* cannot be added by the author of the PR.' % item.value)
self.remove_label(item.value)
return

def warning_about_label_addition(self, item):
r"""
Expand All @@ -808,7 +808,6 @@ def warning_about_label_addition(self, item):
self.add_warning('Label *%s* may be incorrect, since there are unresolved reviews.' % item.value)
else:
self.add_warning('Label *%s* does not match the state of GitHub\'s review system.' % item.value)
return

def hint_about_label_removal(self, item):
r"""
Expand All @@ -819,7 +818,6 @@ def hint_about_label_removal(self, item):
else:
sel_list = 'priority'
self.add_hint('You don\'t need to remove %s labels any more. You\'d better just add the label which replaces it.' % sel_list)
return

# -------------------------------------------------------------------------
# methods to act on events
Expand Down Expand Up @@ -942,7 +940,7 @@ def on_review_comment(self):
info('Simulate label addition of %s for %s' % (label, self._issue))
self.select_label(status)
self.run(Action.labeled, label=status.value)

def remove_all_labels_of_sel_list(self, sel_list):
r"""
Remove all labels of given selection list.
Expand Down
12 changes: 6 additions & 6 deletions .github/workflows/lint.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,18 +31,18 @@ jobs:
id: deps
run: pip install uv

- name: Code style check with ruff-minimal
if: (success() || failure()) && steps.deps.outcome == 'success'
- name: Code style check with ruff
if: ${{ !cancelled() && steps.deps.outcome == 'success' }}
run: |
uv run --frozen --only-group lint -- ruff check --output-format github --ignore E402,E721,E731,E741,E742,E743,F401,F402,F403,F405,F821,F841,I001,PLC0206,PLC0208,PLC1802,PLC2401,PLC3002,PLC0415,PLE0302,PLR0124,PLR0402,PLR0911,PLR0912,PLR0913,PLR0915,PLR1704,PLR1711,PLR1714,PLR1716,PLR1733,PLR1736,PLR2004,PLR5501,PLW0120,PLW0211,PLW0602,PLW0603,PLW0642,PLW1508,PLW1510,PLW1641,PLW2901,PLW3301 src/
uv run --frozen --only-group lint -- ruff check --output-format github --preview --select E111,E115,E21,E221,E222,E225,E227,E228,E25,E271,E272,E275,E302,E303,E305,E306,E401,E502,E701,E702,E703,E71,W291,W293,W391,W605,RUF013,RUF036,TC,UP004,UP006,UP008,UP010,UP015,UP022,UP034,UP035 src/sage/
uv run --frozen --only-group lint -- ruff check --output-format github --config .github/workflows/ruff.toml --no-preview
uv run --frozen --only-group lint -- ruff check --output-format github --config .github/workflows/ruff.toml --preview

- name: Code style check with relint
if: (success() || failure()) && steps.deps.outcome == 'success'
if: ${{ !cancelled() && steps.deps.outcome == 'success' }}
run: uv run --frozen --only-group lint -- relint -c src/.relint.yml -- src/sage/

- name: Validate docstring markup as RST
if: (success() || failure()) && steps.deps.outcome == 'success'
if: ${{ !cancelled() && steps.deps.outcome == 'success' }}
run: uv run --frozen --only-group lint -- flake8 --select=RST src/sage/ --config src/tox.ini

- name: TOML format check with taplo
Expand Down
59 changes: 59 additions & 0 deletions .github/workflows/ruff.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
# Configuration for GitHub Action running ruff
extend = "../../pyproject.toml" # Start with the project configuration

[lint]
# Remove rules that we do not yet follow across the whole codebase.
# Roughly sorted by how easy these should be to fix.
extend-ignore = [
"E742", # 2 failures
"PLW3301", # 3 failures, should be easy to autofix
"PLW1510", # 18 failures, should be easy to autofix
"PLC3002", # 3 failures
"PLE0302", # 3 failures
"E721", # 5 failures
"PLR0124", # 7 failures
"PLW1508", # 8 failures
"PLR0402", # 90 failures, should be easy to autofix
"PLR5501", # 375 failures, should be easy to autofix
"PLC0206", # 39
"PLW0602", # 43 failures
"PLR1704", # 56 failures
"PLW0642", # 61 failures
"F841", # 61 failures
"PLW0603", # 94 failures
"F403", # 142 failures
"F405", # 163 failures
"E731", # 370 failures
"PLW0211", # 495 failures
"PLW2901", # 474 failures
"F821", # 2271 failures
"E231", # 49042 failures, could be autofixed but would change too many files for one PR
"E226", # 29323 failures, could be autofixed but would change too many files for one PR
"F401", # 583 failures
"I001", # 2888 failures

# The remaining failing rules were in ruff preview mode only when added to this file.
"PLW3201",
"PLR6301",
"E241",
"E203",
"E265",
"E201",
"E261",
"PLR6104",
"E202",
"PLR6201",
"E266",
"PLR1702",
"E262",
"PLW1514",
"PLC1901",
"PLC2801",
"E116",
"E114",
"PLC2701",
"E251",
"PLW0108",
"E302",
"PLE4703",
]
5 changes: 3 additions & 2 deletions conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
import inspect
import sys
import warnings
from typing import Any, Iterable, Optional, TYPE_CHECKING
from typing import Any, Optional, TYPE_CHECKING

import pytest
from _pytest.doctest import (
Expand All @@ -32,6 +32,7 @@
from sage.doctest.parsing import SageDocTestParser, SageOutputChecker

if TYPE_CHECKING:
from collections.abc import Iterable
from pathlib import Path


Expand Down Expand Up @@ -270,7 +271,7 @@ def pytest_collect_file(

def pytest_ignore_collect(
collection_path: Path, path: str, config: pytest.Config
) -> None | bool:
) -> bool | None:
"""
This hook is called when collecting test files, and can be used to
prevent considering this path for collection by returning ``True``.
Expand Down
2 changes: 1 addition & 1 deletion pkgs/sagemath-objects/setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,6 @@
setup(
cmdclass = cmdclass,
packages = python_packages,
py_modules = python_modules,
py_modules = python_modules,
ext_modules = cython_modules,
)
157 changes: 150 additions & 7 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,24 +235,54 @@ test = ["coverage", "pytest", "pytest-xdist"]
# Python 3.12 is the minimum supported version
target-version = "py312"

# This directory is full of old autogenerated files.
# Linting it would just get in the way of adding stricter linter rules
# to our actual code
exclude = ["src/doc/"]
exclude = [
# This directory is full of old autogenerated files.
# Linting it would just get in the way of adding stricter linter rules
# to our actual code.
"src/doc/",
# This directory handles the build process,
# and some parts allow Python versions as old as 2.6, so we don't both linting.
"build/",
]

[tool.ruff.lint]

# Rules that we deliberately do not attempt to follow, either because we
# disagree with them or because following them is not currently practical.
ignore = [
"E501", # Line too long - hard to avoid in doctests, and better handled by black.

# Variable/function names I, O, l - common in math, and not an issue for most monospace fonts
"E741",
"E743",

"PLC2401", # Non-ASCII variables - we deliberately use Greek letters (albeit sparingly)

# The following rules are good in spirit but require some common sense, better enforced through code review.
"PLR09", # Rules about code complexity - better enforced through code review process.
"PLR1714", # Repeated equality comparison - too picky when only doing a couple comparisons.
"PLR2004", # Comparison to magic values - better enforced through code review process.
"PLW1641", # __eq__ without __hash__ - implementations of equivalence classes may have a well-defined notion of equality without a well-defined canonical representation that can be used to compute a hash

# Imports at top-level of file - we can't follow this because we intentionally defer imports for various reasons.
# maybe once we can use PEP 810 this can be fixed.
"PLC0415",
Copy link
Contributor

@tobiasdiez tobiasdiez Dec 6, 2025

Choose a reason for hiding this comment

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

I think there are actually quite a few imports in methods that could be moved to the top level without a problem. You want method-level imports only in one instance, namely to break circular imports, right? In that case it would be very helpful if the important is annotated to ease migrating to the new lazy imports (which doesn't help with circular imports) and so it's good if they have a comment like
# noqa: PLC0415 - breaks circular import x > y > z
Then during the migration to the built-in lazy imports, you know you don't need to touch those imports.

If it's just a heavy import that is rarely used in the module, it's okay to convert it to a lazy_import (and then later to the built-in lazy import).

Copy link
Member Author

@vincentmacri vincentmacri Dec 6, 2025

Choose a reason for hiding this comment

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

In that case it would be very helpful if the important is annotated to ease migrating to the new lazy imports (which doesn't help with circular imports)

Lazy imports can help with circular imports depending on the situation. Sometimes a lazy import can fix a circular import, sometimes it can't.

As for what to do until we can use built-in lazy imports, there are so many linter failures related to imports that I think trying to address is futile unless we can reliably do it automatically, I don't think we can until built-in lazy imports are available.

If it's just a heavy import that is rarely used in the module, it's okay to convert it to a lazy_import (and then later to the built-in lazy import).

Yeah but I don't think it's worth the trouble of fixing thousands of lines for this, just to do it again once built-in lazy imports are available.

Copy link
Member Author

@vincentmacri vincentmacri Dec 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

For context: PLC0415 currently has 10606 failures and has no autofix option available.

Copy link
Contributor

@tobiasdiez tobiasdiez Dec 8, 2025

Choose a reason for hiding this comment

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

A couple of weeks ago, I looked into how the migration to lazy import will most likely look like for us (and asked some clarifying questions to the PEP authors). Python will provide some way to call the underlying logic of the lazy import mechanism (similar to the dynamic import method). We can use this in our lazy_import method using something like

def lazy_import(module):
   if python >= 3.15: return python_builtin_lazy_import(module)
   else: our custom implementation

Then, once we have 3.15 as the minimal Python version (which will be in 4 to 5 years), we can replace lazy_import(...) by lazy import ... globally.
So it's a pretty long time frame until we can really profit from the new lazy imports syntax.

As for the local imports, the overall migration to lazy imports would look like:

  • Local import > global lazy_import
  • lazy_import > lazy import

Local imports that cannot be migrated to lazy imports, would need to be annotated in either case.

As for the reason why I like to keep PCL0415 activated is that IDEs highlight the import as a linter warning. So if you touch the surrounding code, you get a natural push to move the local import to a global (lazy) import. Then you usually run the doctests anyway, and will see if the import can be global or it really needs to be local (to break a circular import). I like to keep that nudge, as we would like to remove all possible local imports in the long term anyway.


# Module level import not at top of file.
# ruff is not aware of our lazy_imports, we may be able to remove this ignore after adopting PEP 810
"E402",
Copy link
Contributor

@tobiasdiez tobiasdiez Dec 6, 2025

Choose a reason for hiding this comment

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

I don't quite understand the lazy_import argument. Can you not just have all normal imports at the top, then the lazy imports afterwards?

(We might want to defer this rule until there is a decision on how lazy import should be grouped/sorted - if they get a separate group anyway, that would be a good reason to already follow E402; if they are not grouped separately, then it would only introduce additional noise to first follow E402).

Copy link
Member Author

@vincentmacri vincentmacri Dec 6, 2025

Choose a reason for hiding this comment

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

I don't quite understand the lazy_import argument. Can you not just have all normal imports at the top, then the lazy imports afterwards?

Sure, but I don't think it's worth the amount of effort to do that reformatting and then do it again if we decide to replace our custom lazy import system with the PEP 810 one.

]

select = [
"E", # pycodestyle errors - https://docs.astral.sh/ruff/rules/#error-e
"F", # pyflakes - https://docs.astral.sh/ruff/rules/#pyflakes-f
# A starting point for our ruff rules, we may want to ignore some of these later on.
"I", # isort - https://docs.astral.sh/ruff/rules/#isort-i
"PL", # pylint - https://docs.astral.sh/ruff/rules/#pylint-pl

# ruff-specific rules -- https://docs.astral.sh/ruff/rules/#ruff-specific-rules-ruf
"RUF013",
"RUF036",

"TC", # flake8-type-checking - https://docs.astral.sh/ruff/rules/#flake8-type-checking-tc

# pyupgrade -- https://docs.astral.sh/ruff/rules/#pyupgrade-up
"UP004",
"UP006",
Expand All @@ -262,6 +292,119 @@ select = [
"UP022",
"UP034",
"UP035",

"W", # pycodestyle warnings - https://docs.astral.sh/ruff/rules/#pycodestyle-e-w

# pycodestyle errors - https://docs.astral.sh/ruff/rules/#error-e
Copy link
Contributor

@tobiasdiez tobiasdiez Dec 6, 2025

Choose a reason for hiding this comment

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

Note that we use a fixed version of ruff in CI, so I would say the danger of random failures due to rules in preview is relatively low.
(But it's fine with me as well to list the rules explicitly as you do here)

Copy link
Member Author

@vincentmacri vincentmacri Dec 6, 2025

Choose a reason for hiding this comment

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

I have a different version ruff installed in my system than we fix in the CI, and found things passing locally then failing on CI because new rules were available on CI. I thought this might be annoying for developers to deal with, so this minimizes that risk. But if you think we should enable more rules then I'm okay with that if the CI version is fixed, but wouldn't that make it more work to do PRs that update our dependencies?

Copy link
Contributor

@tobiasdiez tobiasdiez Dec 8, 2025

Choose a reason for hiding this comment

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

Good point, I forgot about the possible version mismatch when running it locally. Then listing the rules like you did is indeed a good idea!

# Ideally we would enable (almost) all of the E rules, but too
# many of these rules are in preview to trust that something won't change
# in ruff itself and cause us headaches.
"E101",
"E401",
"E701",
"E702",
"E703",
"E711",
"E712",
"E713",
"E714",
"E721",
"E722",
"E731",
"E742",
"E902",

# pylint - https://docs.astral.sh/ruff/rules/#pylint-pl
# Like with the E rules, this isn't stable enough yet to enable all of PL

# pylint convention - https://docs.astral.sh/ruff/rules/#convention-plc
"PLC0105",
"PLC0131",
"PLC0132",
"PLC0205",
"PLC0206",
"PLC0208",
"PLC0414",
"PLC1802",
"PLC2403",
"PLC3002",

# pylint errors - https://docs.astral.sh/ruff/rules/#error-ple
"PLE0100",
"PLE0101",
"PLE0115",
"PLE0116",
"PLE0117",
"PLE0118",
"PLE0237",
"PLE0241",
"PLE0302",
"PLE0303",
"PLE0305",
"PLE0307",
"PLE0308",
"PLE0309",
"PLE0604",
"PLE0605",
"PLE0643",
"PLE0704",
"PLE1132",
"PLE1142",
"PLE1205",
"PLE1206",
"PLE1300",
"PLE1307",
"PLE1310",
"PLE1507",
"PLE1519",
"PLE1520",
"PLE1700",
"PLE2502",
"PLE2510",
"PLE2512",
"PLE2513",
"PLE2514",
"PLE2515",

# pylint refactor - https://docs.astral.sh/ruff/rules/#refactor-plr
"PLR0124",
"PLR0133",
"PLR0206",
"PLR0402",
"PLR1704",
"PLR1711",
"PLR1716",
"PLR1722",
"PLR1730",
"PLR1733",
"PLR1736",
"PLR2044",
"PLR5501",

# pyling warning - https://docs.astral.sh/ruff/rules/#warning-plw
"PLW0120",
"PLW0127",
"PLW0128",
"PLW0129",
"PLW0131",
"PLW0133",
"PLW0177",
"PLW0211",
"PLW0245",
"PLW0406",
"PLW0602",
"PLW0603",
"PLW0604",
"PLW0642",
"PLW0711",
"PLW1501",
"PLW1507",
"PLW1508",
"PLW1509",
"PLW1510",
"PLW2101",
"PLW2901",
"PLW3301",
]

[tool.ruff.lint.per-file-ignores]
Expand Down
Loading
Loading