MNT Use consistent ruff version in pre-commit and linting #32849
Conversation
| - repo: https://github.com/astral-sh/ruff-pre-commit | ||
| # WARNING if you update ruff version here, remember to update | ||
| # sklearn/_min_dependencies.py and in doc .rst files | ||
| rev: v0.12.2 |
There was a problem hiding this comment.
We have tests comparing _min_dependencies with readme/pyproject.toml, I wonder if we should also include one to check the pre-commit-config for matching versions?
There was a problem hiding this comment.
good point, my personal opinion is that I am fine with the low-tech approach with a comment. I am not super convinced it's worth adding a test, because this affects only contributors not users. And even if the versions did not match it did not cause us any harm, basically we barely noticed..
Having said this if you think this is a fun thing to try and end up doing a PR and it's not too many lines of code to check the .pre-commit-config.yaml with our min-dependencies, I will happily review it 😉.
| TODO Add |PythonMinVersion| to min_dependency_substitutions.rst one day. | ||
| Probably would need to change a bit sklearn/_min_dependencies.py since Python is not really a package ... | ||
| .. |PythonMinVersion| replace:: 3.11 | ||
|
|
There was a problem hiding this comment.
Small nit: To make the updating easier, we could add the ruff version here as a variable as well.
In general, I wonder, since we will also have upper bounds in the next release, do we need to specify more versions than only for ruff in the build environments?
There was a problem hiding this comment.
True good point, let's do that!
There was a problem hiding this comment.
Actually looks like you can not use substitutions inside a .. block (or any inline markup). I tried and could not make it work.
It seems like a limitation of docutils and sphinx is based on docutils. A similar thing is sphinx-doc/sphinx#2793. There is a work-around with parse-literal or source-read-event in sphinx-doc/sphinx#4054 (comment) and below.
Maybe there is a sphinx way to do what we want, not sure but I am not going to spend more time on this 😅.
AnneBeyer
left a comment
There was a problem hiding this comment.
No worries @lesteve, it makes sense to update everything in one go and I didn't know which version to choose (I'd be happy to learn what the heuristic is here, though). I just have two comments on possible future improvements, otherwise LGTM!
|
So I pushed a commit to make ruff happy but the way ruff wants to group the imports doesn't make any sense in this particular case. For some reason, a My wild-guess is that it is a bug of commit 0eaa20fa885c5c9207e6e100f0848eb1d070918c (HEAD -> use-consistent-ruff-version, origin/use-consistent-ruff-version)
Author: Loic Esteve <loic.esteve@ymail.com>
Date: Sat Dec 6 13:09:11 2025 +0100
Make ruff happy even if the grouping does not make any sense
diff --git a/benchmarks/bench_tsne_mnist.py b/benchmarks/bench_tsne_mnist.py
index 8649c7a46b6..4eba9482843 100644
--- a/benchmarks/bench_tsne_mnist.py
+++ b/benchmarks/bench_tsne_mnist.py
@@ -15,6 +15,7 @@ from time import time
import numpy as np
from joblib import Memory
+from sklearn.utils._openmp_helpers import _openmp_effective_n_threads
from sklearn.datasets import fetch_openml
from sklearn.decomposition import PCA
@@ -22,7 +23,6 @@ from sklearn.manifold import TSNE
from sklearn.neighbors import NearestNeighbors
from sklearn.utils import check_array
from sklearn.utils import shuffle as _shuffle
-from sklearn.utils._openmp_helpers import _openmp_effective_n_threads
LOG_DIR = "mnist_tsne_output"
if not os.path.exists(LOG_DIR): |
|
One thing I noticed actually in the development setup doc, PR would be more than welcome @AnneBeyer, is that for conda you need to use |
Actually I had no idea, but it looks like conda can deal with double equal sign oh well, let's forget about this one then 😅 apparenty |
Noticed thanks to @MarcoGorelli #32813 (review)
Sorry @AnneBeyer it would have been something you could have done indeed, but I only noticed your comment once I already did the work and was about to open the PR 😅. Feel free to review my PR, in case you can spot something I missed.