8000 MNT Update black and ruff config to match min Python version by DimitriPapadopoulos · Pull Request #31016 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

MNT Update black and ruff config to match min Python version #31016

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

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

MNT Update black and ruff config to match min Python version #31016

wants to merge 4 commits into from

Conversation

DimitriPapadopoulos
Copy link
Contributor
@DimitriPapadopoulos DimitriPapadopoulos commented Mar 18, 2025 •
edited
Loading

Reference Issues/PRs

Follow-up of #30895.

What does this implement/fix? Explain your changes.

Align black and ruff target Python versions with the currently supported Python versions.

Any other comments?

These will disappear anyway with #30976 and #31015, but I thought a quick fix would be useful in the meantime.

@github-actions GitHub Actions
Copy link
github-actions bot commented Mar 18, 2025 •
edited
Loading

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: b8cf03d. Link to the linter CI: here

@DimitriPapadopoulos
Copy link
Contributor Author

Not sure about the mypy error, itertools.pairwise has been available since Python 3.10:

error: Module "itertools" has no attribute "pairwise"  [attr-defined]

@DimitriPapadopoulos
Copy link
Contributor Author

From the CircleCI log :
https://app.circleci.com/pipelines/github/scikit-learn/scikit-learn/65895/workflows/58bbcb7d-92fa-4811-a04d-39f4940185d0/jobs/300091/parallel-runs/0/steps/0-102

# Include pytest compatibility with mypy
pip install pytest $(get_dep ruff min) $(get_dep mypy min) $(get_dep black min) cython-lint
[...]
Collecting mypy==1.9
[...]

I suspect MyPy 1.9 is outdated.

@DimitriPapadopoulos
Copy link
Contributor Author
DimitriPapadopoulos commented Mar 18, 2025 •
edited
Loading

I get it. Again from the CircleCI log:
https://app.circleci.com/pipelines/github/scikit-learn/scikit-learn/65895/workflows/58bbcb7d-92fa-4811-a04d-39f4940185d0/jobs/300091/parallel-runs/0/steps/0-0

CircleCI uses Python 3.9:

Starting container cimg/python:3.9.18
Warning: No authentication provided, using CircleCI credentials for pulls from Docker Hub.
  image cache not found on this host, downloading docker.io/cimg/python:3.9.18

From the mypy documentation:

python_version

Specifies the Python version used to parse and check the target program. The string should be in the format MAJOR.MINOR – for example 2.7. The default is the version of the Python interpreter used to run mypy.

This option may only be set in the global section ([mypy]).

So the current mypy target remains 3.9. Bumping CircleCI to 3.10 looks like the proper fix.

@DimitriPapadopoulos DimitriPapadopoulos changed the title MNT Update black and ruff to match min Python version MNT Update black and ruff config to match min Python version Mar 18, 2025
@DimitriPapadopoulos
MNT Update black and ruff to match min Python version
3b6959a
@DimitriPapadopoulos
Apply ruff rule RUF007
0158baf 
RUF007 Prefer `itertools.pairwise()` over `zip()` when iterating over successive pairs

Bumping the Python target version from 3.8 to 3.10 triggered this rule.
@DimitriPapadopoulos
MNT Bump Python in CircleCI to match min version
3b221a3
This was referenced Mar 19, 2025
Merged
Merged
lesteve
lesteve approved these changes Mar 20, 2025
View reviewed changes
Copy link
Member
@lesteve lesteve left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

sklearn/decomposition/tests/test_incremental_pca.py Outdated
@@ -228,7 +229,7 @@ def test_incremental_pca_batch_signs():
ipca = IncrementalPCA(n_components=None, batch_size=batch_size).fit(X)
all_components.append(ipca.components_)

for i, j in zip(all_components[:-1], all_components[1:]):
for i, j in itertools.pairwise(all_components):
Copy link
Member

Choose a reason for hiding this comment

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

Note for other reviewers: this is an automated replacement, ruff is doing this when using target-version "py310"

pyproject.toml
@@ -106,7 +106,7 @@ requires = [

[tool.black]
line-length = 88
target-version = ['py310', 'py311']
target-version = ['py310', 'py311', 'py312']
Copy link
Member

Choose a reason for hiding this comment

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

Not sure it matters that much in practice, but we may as well add py313 right?

Suggested change
target-version = ['py310', 'py311', 'py312']
target-version = ['py310', 'py311', 'py312', 'py313']

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, py313 is not supported — at least not by the version of black used here.

Note that #31015, if accepted, will replace black with ruff format and remove this line altogether.

@lesteve
Copy link
Member
lesteve commented Mar 24, 2025

Is this PR still needed? It seems like in #31014 similar changes were done.

@DimitriPapadopoulos
Copy link
Contributor Author

I thought this one might be accepted more rapidly, while other PRs would require longer discussions. That's not the case, closing this PR.

@lesteve
Copy link
Member
lesteve commented Mar 24, 2025

OK thanks for your answer!

If you could try to avoid opening multiple PRs that overlap (even only a little bit) that would be more than welcome 🙏! Basically this would avoid having different reviewers duplicate efforts.

@DimitriPapadopoulos DimitriPapadopoulos deleted the black branch March 24, 2025 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lesteve lesteve lesteve approved these changes

Assignees
No one assigned
Labels
No Changelog Needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@DimitriPapadopoulos @lesteve
0