Talip7/scikit-learn-issues-multilabel huggingface dataset:
Reuse the `pre-commit` linter config in the CI runners

At the moment, our CI runs linting jobs using a custom script `build_tools/lintin.sh` with manually pip installed dependencies in the Circle CI and Azure Pipelines CI configuration files.

Those linter tool versions are not always consistent with what we configure in `.pre-commit-config.yaml` and that can cause problems such as the following: https://github.com/scikit-learn/scikit-learn/pull/32499#issuecomment-3402244557.

I think we should refactor our CI config to reuse the `pre-commit` command, for instance by running:

```
pre-commit run --all-files
```

Instead of our custom `build_tools/linting.sh` script. However, this might not be that easy:

- there are probably linting failures to fix or config to update to ignore specific files or folders;
- there might be CI integration to update, in particular the `.github/workflows/bot-lint-comment.yml` and the `./build_tools/get_comment.py` script that generate a helpful user facing comments will need to be reworked and adapted to work with the output of the `pre-commit` command.

COMMENT:
> In practice, this has been pretty much the response on PRs. We just tell people to do pre-commit.

Indeed I misremembered and thought the bot linting comment did NOT mention pre-commit only the tool version so that people can use the correct version. Oh well, so I guess the bot linting comment has been nudging contributors towards `pre-commit` for some time [^1].

[^1]: probably one of many Adrin's cunning masterplans 😜

> pre-commit run --all-files was passing, not sure why it's failing now.

One of this caveats you hope this seemingly simple tools never bump into, except they somehow always do (with enough time). My best guess is that we updated the version of all the hooks but did not run the pre-commit on all files so you only then update files when you touch them through pre-commit git hooks?

Labels: Build / CI, help wanted
Reuse the `pre-commit` linter config in the CI runners

At the moment, our CI runs linting jobs using a custom script `build_tools/lintin.sh` with manually pip installed dependencies in the Circle CI and Azure Pipelines CI configuration files.

Those linter tool versions are not always consistent with what we configure in `.pre-commit-config.yaml` and that can cause problems such as the following: https://github.com/scikit-learn/scikit-learn/pull/32499#issuecomment-3402244557.

I think we should refactor our CI config to reuse the `pre-commit` command, for instance by running:

```
pre-commit run --all-files
```

Instead of our custom `build_tools/linting.sh` script. However, this might not be that easy:

- there are probably linting failures to fix or config to update to ignore specific files or folders;
- there might be CI integration to update, in particular the `.github/workflows/bot-lint-comment.yml` and the `./build_tools/get_comment.py` script that generate a helpful user facing comments will need to be reworked and adapted to work with the output of the `pre-commit` command.

COMMENT:
> You can easily do `pre-commit run --all-files` or if you want to run a single hook, you can do `pre-commit run <hook-id>`. I don't think it's too hard to remember those 😜 but I personally just have the hooks installed and never have to run either `ruff` or `pre-commit` since it goes out of the way and simply works.

I know I can do it, but it isn't "easily done". I use it rarely when I want to format some stuff without committing it, so I don't remember how to do it, which means I need to find the docs first, look it up, etc. For example, what is the hook ID of the ruff step? How do I only "ruff" this one file here? - I am sure it is possible, maybe Cursor even knows, but it does add some friction. Especially because the pre-commit docs are, unsurprisingly, targeted at "apply all the checks to all the stuff being committed".

Labels: Build / CI, help wanted
Reuse the `pre-commit` linter config in the CI runners

At the moment, our CI runs linting jobs using a custom script `build_tools/lintin.sh` with manually pip installed dependencies in the Circle CI and Azure Pipelines CI configuration files.

Those linter tool versions are not always consistent with what we configure in `.pre-commit-config.yaml` and that can cause problems such as the following: https://github.com/scikit-learn/scikit-learn/pull/32499#issuecomment-3402244557.

I think we should refactor our CI config to reuse the `pre-commit` command, for instance by running:

```
pre-commit run --all-files
```

Instead of our custom `build_tools/linting.sh` script. However, this might not be that easy:

- there are probably linting failures to fix or config to update to ignore specific files or folders;
- there might be CI integration to update, in particular the `.github/workflows/bot-lint-comment.yml` and the `./build_tools/get_comment.py` script that generate a helpful user facing comments will need to be reworked and adapted to work with the output of the `pre-commit` command.

COMMENT:
> I wouldn't mind us using https://github.com/pre-commit/action instead of writing our own, but my main concern would be if the bot can extract the right information as it is doing now.

I would use the pre-commit-ci in instead: https://pre-commit.ci/

Usually finishes before any github CI even starts.

By default it can autofix PRs by pushing commits with fixable things on it, but this can be turned off in the pre-commit config and activated via the github UI by labelling the issue with the proper label: if you don't remember the pre-commit command, just add the label to the PR and pre-commit CI will format things for you

Labels: Build / CI, help wanted
Reuse the `pre-commit` linter config in the CI runners

At the moment, our CI runs linting jobs using a custom script `build_tools/lintin.sh` with manually pip installed dependencies in the Circle CI and Azure Pipelines CI configuration files.

Those linter tool versions are not always consistent with what we configure in `.pre-commit-config.yaml` and that can cause problems such as the following: https://github.com/scikit-learn/scikit-learn/pull/32499#issuecomment-3402244557.

I think we should refactor our CI config to reuse the `pre-commit` command, for instance by running:

```
pre-commit run --all-files
```

Instead of our custom `build_tools/linting.sh` script. However, this might not be that easy:

- there are probably linting failures to fix or config to update to ignore specific files or folders;
- there might be CI integration to update, in particular the `.github/workflows/bot-lint-comment.yml` and the `./build_tools/get_comment.py` script that generate a helpful user facing comments will need to be reworked and adapted to work with the output of the `pre-commit` command.

COMMENT:
@rgommers had thought's about security risks with pre-commits. Ralf, do you want to express them here, so that I don't distort what you said?

Labels: Build / CI, help wanted
Reuse the `pre-commit` linter config in the CI runners

At the moment, our CI runs linting jobs using a custom script `build_tools/lintin.sh` with manually pip installed dependencies in the Circle CI and Azure Pipelines CI configuration files.

Those linter tool versions are not always consistent with what we configure in `.pre-commit-config.yaml` and that can cause problems such as the following: https://github.com/scikit-learn/scikit-learn/pull/32499#issuecomment-3402244557.

I think we should refactor our CI config to reuse the `pre-commit` command, for instance by running:

```
pre-commit run --all-files
```

Instead of our custom `build_tools/linting.sh` script. However, this might not be that easy:

- there are probably linting failures to fix or config to update to ignore specific files or folders;
- there might be CI integration to update, in particular the `.github/workflows/bot-lint-comment.yml` and the `./build_tools/get_comment.py` script that generate a helpful user facing comments will need to be reworked and adapted to work with the output of the `pre-commit` command.

COMMENT:
Sure. I consider pre-commit as used on the average OSS Python project quite risky security-wise, because:

1. It installs/updates packages automatically in a very low-visibility way without much control
    -  Contrast that to, e.g., `conda` which shows you a list of packages and then allows you to say Yes; `pixi`/`uv` as the currently best tools for conda-forge and PyPI respectively are also much better.
2. Usually the type of things included in pre-commit, like sorting import order or single vs double quotes, are pointing at random projects from a single person that I don't know. Each such project needs inspecting (for me) and is a potential security risk. The risk/reward tradeoffs are generally very poor.
3. There tend to be frequent updates of the packages pulled in by pre-commit with some bot (not for scikit-learn though it looks like), magnifying the risk.

For SciPy we ended up adding a pre-commit hook but making it fully optional - that was a hard requirement I and a number of others had - so it's opt-in only. Linting is done with `spin lint` by default, and the pre-commit hook just calls the same script as `spin lint` does. There's a discussion starting around https://github.com/scipy/scipy/issues/15489#issuecomment-1281062874 that may be relevant.

To each their own, so I don't mind it's in the SciPy repo or that other projects use it, but I personally don't want to touch it. If a project uses it in their CI, when I contribute I just let it fail in CI and iterate until green.

Labels: Build / CI, help wanted
Reuse the `pre-commit` linter config in the CI runners

At the moment, our CI runs linting jobs using a custom script `build_tools/lintin.sh` with manually pip installed dependencies in the Circle CI and Azure Pipelines CI configuration files.

Those linter tool versions are not always consistent with what we configure in `.pre-commit-config.yaml` and that can cause problems such as the following: https://github.com/scikit-learn/scikit-learn/pull/32499#issuecomment-3402244557.

I think we should refactor our CI config to reuse the `pre-commit` command, for instance by running:

```
pre-commit run --all-files
```

Instead of our custom `build_tools/linting.sh` script. However, this might not be that easy:

- there are probably linting failures to fix or config to update to ignore specific files or folders;
- there might be CI integration to update, in particular the `.github/workflows/bot-lint-comment.yml` and the `./build_tools/get_comment.py` script that generate a helpful user facing comments will need to be reworked and adapted to work with the output of the `pre-commit` command.

COMMENT:
> To each their own, so I don't mind it's in the SciPy repo or that other projects use it, but I personally don't want to touch it. If a project uses it in their CI, when I contribute I just let it fail in CI and iterate until green.

so in that sense pre-commit CI would help you so that you would not have to run it locally but you could ask it to run the autofixing for you without having you to run it locally

Labels: Build / CI, help wanted
Reuse the `pre-commit` linter config in the CI runners

At the moment, our CI runs linting jobs using a custom script `build_tools/lintin.sh` with manually pip installed dependencies in the Circle CI and Azure Pipelines CI configuration files.

Those linter tool versions are not always consistent with what we configure in `.pre-commit-config.yaml` and that can cause problems such as the following: https://github.com/scikit-learn/scikit-learn/pull/32499#issuecomment-3402244557.

I think we should refactor our CI config to reuse the `pre-commit` command, for instance by running:

```
pre-commit run --all-files
```

Instead of our custom `build_tools/linting.sh` script. However, this might not be that easy:

- there are probably linting failures to fix or config to update to ignore specific files or folders;
- there might be CI integration to update, in particular the `.github/workflows/bot-lint-comment.yml` and the `./build_tools/get_comment.py` script that generate a helpful user facing comments will need to be reworked and adapted to work with the output of the `pre-commit` command.

COMMENT:
I'm quite against using pre-commit CI. The main reason being that it is quite complicated to recover from having new commits in the remote branch and in your local branch. For example when pre-commit CI pushes a commit without the PR author noticing, and the PR author adding more changes locally. I don't enjoy resolving that state when I have to do it myself and I really don't want to have to explain via text how to do it to someone who is new to this.

In addition using pre-commit's CI ties us to yet another infrastructure provider. I've not invested the time to understand how sustainable the business model of pre-commit CI is, but I suspect like all services it will eventually start charging (which is a fine strategy) or stop existing (also understandable). The more different infrastructures you rely on, the lower the combined availability (multiplying numbers smaller than one always makes an even smaller number).

I had not considered the security aspects of the lack of transparency with pre-commit, I mostly meet it as something that makes it more tricky to replicate it to be able to run "just the formatting on just this particular file"

Labels: Build / CI, help wanted
Reuse the `pre-commit` linter config in the CI runners

At the moment, our CI runs linting jobs using a custom script `build_tools/lintin.sh` with manually pip installed dependencies in the Circle CI and Azure Pipelines CI configuration files.

Those linter tool versions are not always consistent with what we configure in `.pre-commit-config.yaml` and that can cause problems such as the following: https://github.com/scikit-learn/scikit-learn/pull/32499#issuecomment-3402244557.

I think we should refactor our CI config to reuse the `pre-commit` command, for instance by running:

```
pre-commit run --all-files
```

Instead of our custom `build_tools/linting.sh` script. However, this might not be that easy:

- there are probably linting failures to fix or config to update to ignore specific files or folders;
- there might be CI integration to update, in particular the `.github/workflows/bot-lint-comment.yml` and the `./build_tools/get_comment.py` script that generate a helpful user facing comments will need to be reworked and adapted to work with the output of the `pre-commit` command.

COMMENT:
Yeah, my reading of the scikit-learn maintainers' room is also against https://pre-commit.ci.

Regarding security aspects of pre-commit, I am like 🤷 for now. Sure the environment management is opaque and another layer of indirection which could obfuscate things and execute arbitrary code.

At the same time, when you are following the setup instructions of a new repo, it's not like you are checking the implications of running every single commands that you are told to run ... for example to install the environment to build our doc `pip install <this-sphinx-plugin-that-i-have-no-idea-who-maintains-it-and-why-we-need-it-but-it-looks-like-we-do>` could also run arbitrary code (not to mention the dependencies of the dependencies of the dependencies of this particular sphinx plugin) ...

Labels: Build / CI, help wanted
`cython-lint` failure on `main`

The `main` branch and some PRs synchronized with it are now failing with the following `cython-lint` error:

```
/home/vsts/work/1/s/sklearn/cluster/_hdbscan/_tree.pyx:786:19: unnecessary set + generator (just use a set comprehension)
Problems detected by cython-lint, please fix them
```

I think this is a consequence of the merge of #32489 but weirdly enough, that PR was green and, I cannot reproduce locally on the current `main`:

```
$ pre-commit run --files sklearn/cluster/_hdbscan/_tree.pyx
[INFO] Initializing environment for https://github.com/MarcoGorelli/cython-lint.
[INFO] Installing environment for https://github.com/MarcoGorelli/cython-lint.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
check yaml...........................................(no files to check)Skipped
fix end of files.........................................................Passed
trim trailing whitespace.................................................Passed
ruff check...........................................(no files to check)Skipped
ruff format..........................................(no files to check)Skipped
mypy.................................................(no files to check)Skipped
cython-lint..............................................................Passed
prettier.............................................(no files to check)Skipped
codespell................................................................Passed
```

COMMENT:
If I install the latest version of `cython-lint` from `pip` I can reproduce the problem by running the command manually:

```
cython-lint sklearn/cluster/_hdbscan/_tree.pyx
/Users/ogrisel/code/scikit-learn/sklearn/cluster/_hdbscan/_tree.pyx:786:19: unnecessary set + generator (just use a set comprehension)
```

Labels: Build / CI
`cython-lint` failure on `main`

The `main` branch and some PRs synchronized with it are now failing with the following `cython-lint` error:

```
/home/vsts/work/1/s/sklearn/cluster/_hdbscan/_tree.pyx:786:19: unnecessary set + generator (just use a set comprehension)
Problems detected by cython-lint, please fix them
```

I think this is a consequence of the merge of #32489 but weirdly enough, that PR was green and, I cannot reproduce locally on the current `main`:

```
$ pre-commit run --files sklearn/cluster/_hdbscan/_tree.pyx
[INFO] Initializing environment for https://github.com/MarcoGorelli/cython-lint.
[INFO] Installing environment for https://github.com/MarcoGorelli/cython-lint.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
check yaml...........................................(no files to check)Skipped
fix end of files.........................................................Passed
trim trailing whitespace.................................................Passed
ruff check...........................................(no files to check)Skipped
ruff format..........................................(no files to check)Skipped
mypy.................................................(no files to check)Skipped
cython-lint..............................................................Passed
prettier.............................................(no files to check)Skipped
codespell................................................................Passed
```

COMMENT:
cc @MarcoGorelli for his advice whether we should expect similar linting changes in the future for cython-lint. I know that as the saying goes: it's hard to make predictions, especially about the future 😉.

I guess until now we were a bit lazy and never needed to pin the cython-lint version but maybe we should start doing it?

Labels: Build / CI
`cython-lint` failure on `main`

The `main` branch and some PRs synchronized with it are now failing with the following `cython-lint` error:

```
/home/vsts/work/1/s/sklearn/cluster/_hdbscan/_tree.pyx:786:19: unnecessary set + generator (just use a set comprehension)
Problems detected by cython-lint, please fix them
```

I think this is a consequence of the merge of #32489 but weirdly enough, that PR was green and, I cannot reproduce locally on the current `main`:

```
$ pre-commit run --files sklearn/cluster/_hdbscan/_tree.pyx
[INFO] Initializing environment for https://github.com/MarcoGorelli/cython-lint.
[INFO] Installing environment for https://github.com/MarcoGorelli/cython-lint.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
check yaml...........................................(no files to check)Skipped
fix end of files.........................................................Passed
trim trailing whitespace.................................................Passed
ruff check...........................................(no files to check)Skipped
ruff format..........................................(no files to check)Skipped
mypy.................................................(no files to check)Skipped
cython-lint..............................................................Passed
prettier.............................................(no files to check)Skipped
codespell................................................................Passed
```

COMMENT:
Yeah  the precommit version is behind, I push a new version out today with some new checks

Fwiw I'd suggest running pre-commit run --all-files in ci, then there's a single command and everything stays in sync

Labels: Build / CI
Incorrect warning about data conversion when using `pairwise_distances_argmin` with `jaccard` and bool arrays

### Describe the bug

When using `pairwise_distances_argmin` with the `jaccard` metric, sklearn warns of data conversion even when both input arrays are of `dtype=np.bool`



### Steps/Code to Reproduce

```python
import numpy as np
from sklearn.metrics import pairwise_distances_argmin
x = np.ones((10, 10), dtype=np.bool)
y = np.ones((10, 10), dtype=np.bool)
argmin = pairwise_distances_argmin(x, y, metric="jaccard")
```

### Expected Results

No such warning should be emitted

### Actual Results

The following warning is emitted:
```
sklearn/metrics/pairwise.py:2462: DataConversionWarning: Data was converted to boolean for metric jaccard
  warnings.warn(msg, DataConversionWarning)
```

### Versions

```shell
System:
    python: 3.11.13 | packaged by conda-forge | (main, Jun  4 2025, 14:48:23) [GCC 13.3.0]
executable: /home/ipickering/Conda/envs/bblean/bin/python
   machine: Linux-6.14.0-33-generic-x86_64-with-glibc2.39

Python dependencies:
      sklearn: 1.7.2
          pip: 25.2
   setuptools: 80.9.0
        numpy: 2.3.0
        scipy: 1.16.2
       Cython: None
       pandas: 2.3.2
   matplotlib: 3.10.6
       joblib: 1.5.2
threadpoolctl: 3.6.0

Built with OpenMP: True

threadpoolctl info:
       user_api: blas
   internal_api: openblas
    num_threads: 12
         prefix: libopenblas
       filepath: /home/ipickering/Conda/envs/bblean/lib/libopenblasp-r0.3.30.so
        version: 0.3.30
threading_layer: pthreads
   architecture: Zen

       user_api: openmp
   internal_api: openmp
    num_threads: 12
         prefix: libgomp
       filepath: /home/ipickering/Conda/envs/bblean/lib/libgomp.so.1.0.0
        version: None
```

COMMENT:
Hi,

I've investigated this locally and can confirm the bug.

The root cause is in the `pairwise_distances` function, which issues a `DataConversionWarning` for boolean metrics without first checking if the input data is already a boolean type. My testing confirms this affects all boolean metrics (`jaccard`, `dice`,`rogerstanimoto`, `sokalmichene` etc.), and consequently, any function that relies on it, like `pairwise_distances_argmin`.

The fix is to wrap the warning in a conditional to check the array's `dtype` first. I am working on solution and am happy to submit a pull request if it sounds good. Let me know if I'm missing something. Thanks!

Labels: Bug