Talip7/scikit-learn-issues-multilabel huggingface dataset:
MNT Check the variable initialization in Cython pxd files

So it turns out variable initialization in pxd files doesn't do anything see https://github.com/scikit-learn/scikit-learn/pull/32259#issuecomment-3337478228.

It would be good to check that we are not doing it in other places than the ones we have already fixed (see https://github.com/scikit-learn/scikit-learn/pull/32291 for an example of a PR fixing one of these)

```
git grep -C10 -P '\w+\s*=\s*\d' **/*.pxd
```

Some of them are inside `enum` and my guess is that it works fine (to be double-checked ...) but at least one of them is not:
```
sklearn/utils/_random.pxd:cdef inline uint32_t DEFAULT_SEED = 1
```

The value is set in the `.pyx` so I guess in this case it's fine to remove the initialization from the `pxd` (and I guess also the type declaration in the `pyx`).

_Originally posted by @lesteve in https://github.com/scikit-learn/scikit-learn/issues/32259#issuecomment-3338224571_
            
Another way to check is to use Cython development version which has added a warning since https://github.com/cython/cython/pull/7176.

COMMENT:
In my opinion,   in Cython, initializing variables in (.pxd) files is usually ineffective; you should only declare their type and perform the actual initialization in (.pyx) files to ensure correct runtime  behavior  .

Labels: cython
MNT Check the variable initialization in Cython pxd files

So it turns out variable initialization in pxd files doesn't do anything see https://github.com/scikit-learn/scikit-learn/pull/32259#issuecomment-3337478228.

It would be good to check that we are not doing it in other places than the ones we have already fixed (see https://github.com/scikit-learn/scikit-learn/pull/32291 for an example of a PR fixing one of these)

```
git grep -C10 -P '\w+\s*=\s*\d' **/*.pxd
```

Some of them are inside `enum` and my guess is that it works fine (to be double-checked ...) but at least one of them is not:
```
sklearn/utils/_random.pxd:cdef inline uint32_t DEFAULT_SEED = 1
```

The value is set in the `.pyx` so I guess in this case it's fine to remove the initialization from the `pxd` (and I guess also the type declaration in the `pyx`).

_Originally posted by @lesteve in https://github.com/scikit-learn/scikit-learn/issues/32259#issuecomment-3338224571_
            
Another way to check is to use Cython development version which has added a warning since https://github.com/cython/cython/pull/7176.

COMMENT:
Thanks for opening the issue @lesteve. 

I thought the recommended fix was to add a `const` qualifier if these are indeed variable constants?

Labels: cython
MNT Check the variable initialization in Cython pxd files

So it turns out variable initialization in pxd files doesn't do anything see https://github.com/scikit-learn/scikit-learn/pull/32259#issuecomment-3337478228.

It would be good to check that we are not doing it in other places than the ones we have already fixed (see https://github.com/scikit-learn/scikit-learn/pull/32291 for an example of a PR fixing one of these)

```
git grep -C10 -P '\w+\s*=\s*\d' **/*.pxd
```

Some of them are inside `enum` and my guess is that it works fine (to be double-checked ...) but at least one of them is not:
```
sklearn/utils/_random.pxd:cdef inline uint32_t DEFAULT_SEED = 1
```

The value is set in the `.pyx` so I guess in this case it's fine to remove the initialization from the `pxd` (and I guess also the type declaration in the `pyx`).

_Originally posted by @lesteve in https://github.com/scikit-learn/scikit-learn/issues/32259#issuecomment-3338224571_
            
Another way to check is to use Cython development version which has added a warning since https://github.com/cython/cython/pull/7176.

COMMENT:
> I thought the recommended fix was to add a const qualifier if these are indeed variable constants?

It does feel like the simplest fix indeed in this case. So probably we can use a `const` for consistency in the `cdef inline uint32_t DEFAULT_SEED = 1` case. I guess `inline` is similar to a C macro so not exactly the same as a const variable but I don't think this matters much in practice ... and trying to reduce the number of quirky patterns we used in Cython is probably a good idea 😅.

Labels: cython
MNT Check the variable initialization in Cython pxd files

So it turns out variable initialization in pxd files doesn't do anything see https://github.com/scikit-learn/scikit-learn/pull/32259#issuecomment-3337478228.

It would be good to check that we are not doing it in other places than the ones we have already fixed (see https://github.com/scikit-learn/scikit-learn/pull/32291 for an example of a PR fixing one of these)

```
git grep -C10 -P '\w+\s*=\s*\d' **/*.pxd
```

Some of them are inside `enum` and my guess is that it works fine (to be double-checked ...) but at least one of them is not:
```
sklearn/utils/_random.pxd:cdef inline uint32_t DEFAULT_SEED = 1
```

The value is set in the `.pyx` so I guess in this case it's fine to remove the initialization from the `pxd` (and I guess also the type declaration in the `pyx`).

_Originally posted by @lesteve in https://github.com/scikit-learn/scikit-learn/issues/32259#issuecomment-3338224571_
            
Another way to check is to use Cython development version which has added a warning since https://github.com/cython/cython/pull/7176.

COMMENT:
I am going to reopen this. After #32537 was merged, it would be nice to have another look at the `git grep` again and make sure we don't have remaining questionable patterns
```
git grep -C10 -P '\w+\s*=\s*\d' **/*.pxd
```

Labels: cython
MNT Check the variable initialization in Cython pxd files

So it turns out variable initialization in pxd files doesn't do anything see https://github.com/scikit-learn/scikit-learn/pull/32259#issuecomment-3337478228.

It would be good to check that we are not doing it in other places than the ones we have already fixed (see https://github.com/scikit-learn/scikit-learn/pull/32291 for an example of a PR fixing one of these)

```
git grep -C10 -P '\w+\s*=\s*\d' **/*.pxd
```

Some of them are inside `enum` and my guess is that it works fine (to be double-checked ...) but at least one of them is not:
```
sklearn/utils/_random.pxd:cdef inline uint32_t DEFAULT_SEED = 1
```

The value is set in the `.pyx` so I guess in this case it's fine to remove the initialization from the `pxd` (and I guess also the type declaration in the `pyx`).

_Originally posted by @lesteve in https://github.com/scikit-learn/scikit-learn/issues/32259#issuecomment-3338224571_
            
Another way to check is to use Cython development version which has added a warning since https://github.com/cython/cython/pull/7176.

COMMENT:
I've completed the search for variable initializations in .pxd files using the git grep command. Here are my findings:

Files checked:

sklearn/neighbors/_quad_tree.pxd: DEBUGFLAG = 0 - Inside enum (safe)

sklearn/tree/_utils.pxd: RAND_R_MAX = 2147483647 - Inside enum (safe)

sklearn/utils/_cython_blas.pxd: NoTrans = 110, Trans = 116 - Inside enum (safe)

sklearn/utils/_random.pxd: DEFAULT_SEED = 1 - Already fixed with const approach

Conclusion: All variable initializations found are properly contained within enum blocks, which is the correct Cython pattern. There are no remaining questionable variable initialization patterns in the .pxd files.

Labels: cython
MNT Check the variable initialization in Cython pxd files

So it turns out variable initialization in pxd files doesn't do anything see https://github.com/scikit-learn/scikit-learn/pull/32259#issuecomment-3337478228.

It would be good to check that we are not doing it in other places than the ones we have already fixed (see https://github.com/scikit-learn/scikit-learn/pull/32291 for an example of a PR fixing one of these)

```
git grep -C10 -P '\w+\s*=\s*\d' **/*.pxd
```

Some of them are inside `enum` and my guess is that it works fine (to be double-checked ...) but at least one of them is not:
```
sklearn/utils/_random.pxd:cdef inline uint32_t DEFAULT_SEED = 1
```

The value is set in the `.pyx` so I guess in this case it's fine to remove the initialization from the `pxd` (and I guess also the type declaration in the `pyx`).

_Originally posted by @lesteve in https://github.com/scikit-learn/scikit-learn/issues/32259#issuecomment-3338224571_
            
Another way to check is to use Cython development version which has added a warning since https://github.com/cython/cython/pull/7176.

COMMENT:
Thanks for your input @Nitin-Prata! I did indeed double-check and the remaining initializations in pxd are `enum` or `const`, let's close this one.

Labels: cython
Unclear if multiple cores are used in MiniBatchKMeans

### Describe the issue linked to the documentation

Currently, the documentation states the following when describing the batch size: 

> Size of the mini batches. For faster computations, you can set the batch_size greater than 256 * number of cores to enable parallelism on all cores

There, however, is no ability to control the number of cores as far as I can see. Is this potentially outdated?

### Suggest a potential alternative/fix

_No response_

COMMENT:
`KMeans` uses OpenMP so I guess `MiniBatchKMeans` too? If that's the case, you can set `OMP_NUM_THREADS` to control the number of cores used OpenMP or `threadpoolctl`, see [doc](https://scikit-learn.org/stable/computing/parallelism.html#lower-level-parallelism-with-openmp).

Labels: Documentation
Unclear if multiple cores are used in MiniBatchKMeans

### Describe the issue linked to the documentation

Currently, the documentation states the following when describing the batch size: 

> Size of the mini batches. For faster computations, you can set the batch_size greater than 256 * number of cores to enable parallelism on all cores

There, however, is no ability to control the number of cores as far as I can see. Is this potentially outdated?

### Suggest a potential alternative/fix

_No response_

COMMENT:
I guess we could document better in estimators when OpenMP is used for parallelism? There are probably previous discussions in the issue tracker related to this ...

@KennethEnevoldsen if you find some discussions, it would be super useful to add a comment with a link to the discussions 🙏.

cc @jeremiedbb in case you remember some discussions off the top of your head about documenting OpenMP usage in estimators.

Labels: Documentation
Unclear if multiple cores are used in MiniBatchKMeans

### Describe the issue linked to the documentation

Currently, the documentation states the following when describing the batch size: 

> Size of the mini batches. For faster computations, you can set the batch_size greater than 256 * number of cores to enable parallelism on all cores

There, however, is no ability to control the number of cores as far as I can see. Is this potentially outdated?

### Suggest a potential alternative/fix

_No response_

COMMENT:
Thanks for the response, @lesteve. This was exactly what I was looking for.

I looked over the issues, but couldn't find any (they might be there; you have a lot of issues after all). For now, though, I just added a link to that specific documentation. This would have been enough for me to find it, and it is a minimal change to the docs. If you'd like to approach it differently, please feel free to close my PR.

(@Samoed, this might be what caused the slight difference in results)

Labels: Documentation
Unclear if multiple cores are used in MiniBatchKMeans

### Describe the issue linked to the documentation

Currently, the documentation states the following when describing the batch size: 

> Size of the mini batches. For faster computations, you can set the batch_size greater than 256 * number of cores to enable parallelism on all cores

There, however, is no ability to control the number of cores as far as I can see. Is this potentially outdated?

### Suggest a potential alternative/fix

_No response_

COMMENT:
> Size of the mini batches. For faster computations, you can set the batch_size greater than 256 * number of cores to enable parallelism on all cores

@KennethEnevoldsen note that parallelism is enabled by default and that MiniBatchKMeans will try to use as many cores as possible, more precisely ``min(batch_size // 256, n_cores)``. Using the env var ``OMP_NUM_THREADS`` or ``threadpoolctl`` to force a higher number will either have no effect or degrade performances due to oversubscription.

Labels: Documentation
Unclear if multiple cores are used in MiniBatchKMeans

### Describe the issue linked to the documentation

Currently, the documentation states the following when describing the batch size: 

> Size of the mini batches. For faster computations, you can set the batch_size greater than 256 * number of cores to enable parallelism on all cores

There, however, is no ability to control the number of cores as far as I can see. Is this potentially outdated?

### Suggest a potential alternative/fix

_No response_

COMMENT:
Closed by #32520.

Labels: Documentation
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:
Before starting to work on it, I think we need to make sure that other @scikit-learn/core-devs agree that this is a worthy thing to do.

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:
Simplest fix/work-around would be to `pip install cython-lint==<manually-fixed-version>` which matches the `.pre-commit-config.yaml`, which we are doing for `ruff` (and I think we did for some time with black)?

Side-comment: some time ago I looked at using a lock-file for the linters, except that `pre-commit` has its own opinionated way of managing the environment and I was not able to find anything. Maybe worth another look today to see if things have changed.

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:
The fact that pre-commit has its own way of managing things and that you can't run `ruff` from your dev environment directly but need to go via pre-commit doesn't make me happy (mostly because I don't really know how to drive pre-commit). So I am a little "against" committing ourselves to pre-commit more.

However, I think having to keep things in sync or tracking down why you sometimes get an error and sometimes not is pretty annoying and uses up a lot of time (with little benefit to the person investing the time).

Maybe Loïc's suggestion is a good compromise?

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 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.

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:
There is also the question: do we want to tell contributors through the lint bot comment
1. use `pre-commit` to fix all the linting issues or
2. stick with the current status quo, "let's not require contributors to install pre-commit". The main argument for the status quo is that pre-commit behaviour can be confusing when you need to commit twice

No strong opinion on this personally.

By the way `pre-commit run --all-files` currently fails for me. I guess because we actually never have run it fully but only on changed files through pre-commit hooks, not 100% sure. This kind of drift could well happen in the future as well?

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:
`pre-commit run --all-files` was passing, not sure why it's failing now.

For me, if a contributor doesn't want to install pre-commit, I don't want to spend time telling them the alternative route. If they want the alternative, they can figure it out themselves. In practice, this has been pretty much the response on PRs. We just tell people to do pre-commit.

Labels: Build / CI, help wanted