Talip7/scikit-learn-issues-multilabel huggingface dataset:
Replace confusing buildtype=debugoptimized by buildtype=release in meson.build

So we use `buildtype=debugoptimized` mostly because I took it from the other projects I looked at (numpy, scipy, scikit-image) without understanding too much what this meant.

https://github.com/scikit-learn/scikit-learn/blob/70a84ea2db68d3cab1df30ed374445be2ac67dd4/meson.build#L8

All the scikit-learn developers are going through `pip` to build scikit-learn and I have recently realised that when you go through `pip` meson-python sets the buildtype=release see https://mesonbuild.com/meson-python/explanations/default-options.html. This makes buildtype=debugoptimized a bit confusing since it is actually almost never used, so should be use `buildtype=release` instead? For it to be used you would need to do `meson setup` + `ninja` manually which happens super rarely in a scikit-learn context.

Contrast this with most other projects that historically (my crude understanding at least) use out-of-tree build i.e. for example `spin build` + set `PYTHONPATH` when doing `spin test` (although some of the developers use editable installs for example because better integration into IDE/editors I think).

For these other projects, it may make sense to have developer build in debugoptimized mode and let `meson-python` build in release for wheels. I guess that's a possible reason for this, but I am not sure ....

cc @rgommers in case you have some advice about this.

cc @adam2392 because you asked the question in our Discord https://discord.com/channels/731163543038197871/1046822941586898974/1263909669181722655

COMMENT:
> Is there a way to pass extra command-line arguments to `pip install --editable` so that the user can decide between the two without having to edit a version controlled file in the source tree?

There is: add `-Csetup-args=-Dbuildtype=debugoptimized` to the `pip` install command. Important: on a clean build only (not an incremental rebuild); any such flags passed, and env vars like CFLAGS used, are only evaluated when the full configure stage is first run.

Labels: Build / CI
Replace confusing buildtype=debugoptimized by buildtype=release in meson.build

So we use `buildtype=debugoptimized` mostly because I took it from the other projects I looked at (numpy, scipy, scikit-image) without understanding too much what this meant.

https://github.com/scikit-learn/scikit-learn/blob/70a84ea2db68d3cab1df30ed374445be2ac67dd4/meson.build#L8

All the scikit-learn developers are going through `pip` to build scikit-learn and I have recently realised that when you go through `pip` meson-python sets the buildtype=release see https://mesonbuild.com/meson-python/explanations/default-options.html. This makes buildtype=debugoptimized a bit confusing since it is actually almost never used, so should be use `buildtype=release` instead? For it to be used you would need to do `meson setup` + `ninja` manually which happens super rarely in a scikit-learn context.

Contrast this with most other projects that historically (my crude understanding at least) use out-of-tree build i.e. for example `spin build` + set `PYTHONPATH` when doing `spin test` (although some of the developers use editable installs for example because better integration into IDE/editors I think).

For these other projects, it may make sense to have developer build in debugoptimized mode and let `meson-python` build in release for wheels. I guess that's a possible reason for this, but I am not sure ....

cc @rgommers in case you have some advice about this.

cc @adam2392 because you asked the question in our Discord https://discord.com/channels/731163543038197871/1046822941586898974/1263909669181722655

COMMENT:
Thanks Ralf for the details, this is super helpful!

> I'd probably choose to not specify any default in meson.build so that there's no divergence between editable installs and spin or plain meson setup usage.

I opened #29594 with this suggestion.

> between editable installs and spin or plain meson setup usage.

Small precision about this part, you can use spin to get editable install through `spin install` so there is:
1. one one side: editable install which you can get through `spin install`
2. on the other side: `spin build` or plain meson setup

Mostly a FYI for Ralf: using `spin` is possible since recently in scikit-learn #29012 and we only have `spin install` which is very similar as the `pip install --editable` command we are used to. In other words there is no `spin build` in scikit-learn to avoid adding another option that we don't really need. We'll see how this small spin experiment turns out, but as of today I would guess I am the only one using spin for my daily scikit-learn work :wink:.

Labels: Build / CI
MAINT Replace `unsigned char` with `uint8_t` in codebase

In https://github.com/scikit-learn/scikit-learn/issues/25572, we defined typedefs for commonly used Cython types throughout the codebase. Running `grep -rl "unsigned char" ./sklearn`, I found the following files contain `unsigned char`, which could be replaced with `uint8_t` from  https://github.com/scikit-learn/scikit-learn/blob/main/sklearn/utils/_typedefs.pxd.

- [ ] ./sklearn/metrics/_pairwise_distances_reduction/_radius_neighbors_classmode.pyx.tp
- [ ] ./sklearn/ensemble/_hist_gradient_boosting/common.pxd
- [ ] ./sklearn/ensemble/_hist_gradient_boosting/_bitset.pyx
- [ ] ./sklearn/ensemble/_hist_gradient_boosting/_predictor.pyx
- [ ] ./sklearn/ensemble/_hist_gradient_boosting/splitting.pyx
- [ ] ./sklearn/ensemble/_hist_gradient_boosting/histogram.pyx
- [ ] ./sklearn/ensemble/_hist_gradient_boosting/_binning.pyx
- [ ] ./sklearn/ensemble/_hist_gradient_boosting/_bitset.pxd
- [ ] ./sklearn/linear_model/_sgd_fast.pyx.tp

~Is there any reason to leave these as `unsigned char` vs replacing them with the `uint8_t` to consolidate our types?~

To address this issue, it would be best to open up a PR one-by-one and implement the said change and cimport statement, and then link to the issue here.

COMMENT:
> Is there any reason to leave these as unsigned char vs replacing them with the uint8_t to consolidate our types?

I see no reason and I find `uint8_t` more explicit and therefore easier to reason about the range of admissible values [0-255]. So +1 for the change on my end.

Labels: good first issue, help wanted, cython
MAINT Replace `unsigned char` with `uint8_t` in codebase

In https://github.com/scikit-learn/scikit-learn/issues/25572, we defined typedefs for commonly used Cython types throughout the codebase. Running `grep -rl "unsigned char" ./sklearn`, I found the following files contain `unsigned char`, which could be replaced with `uint8_t` from  https://github.com/scikit-learn/scikit-learn/blob/main/sklearn/utils/_typedefs.pxd.

- [ ] ./sklearn/metrics/_pairwise_distances_reduction/_radius_neighbors_classmode.pyx.tp
- [ ] ./sklearn/ensemble/_hist_gradient_boosting/common.pxd
- [ ] ./sklearn/ensemble/_hist_gradient_boosting/_bitset.pyx
- [ ] ./sklearn/ensemble/_hist_gradient_boosting/_predictor.pyx
- [ ] ./sklearn/ensemble/_hist_gradient_boosting/splitting.pyx
- [ ] ./sklearn/ensemble/_hist_gradient_boosting/histogram.pyx
- [ ] ./sklearn/ensemble/_hist_gradient_boosting/_binning.pyx
- [ ] ./sklearn/ensemble/_hist_gradient_boosting/_bitset.pxd
- [ ] ./sklearn/linear_model/_sgd_fast.pyx.tp

~Is there any reason to leave these as `unsigned char` vs replacing them with the `uint8_t` to consolidate our types?~

To address this issue, it would be best to open up a PR one-by-one and implement the said change and cimport statement, and then link to the issue here.

COMMENT:
Hello, I've changed the first file according to: "It would be best to open up a PR one-by-one and implement the said change and cimport statement, and then link to the issue here."

This is my first time contributing, let me know if there are any issues. Thanks

Labels: good first issue, help wanted, cython
MAINT Replace `unsigned char` with `uint8_t` in codebase

In https://github.com/scikit-learn/scikit-learn/issues/25572, we defined typedefs for commonly used Cython types throughout the codebase. Running `grep -rl "unsigned char" ./sklearn`, I found the following files contain `unsigned char`, which could be replaced with `uint8_t` from  https://github.com/scikit-learn/scikit-learn/blob/main/sklearn/utils/_typedefs.pxd.

- [ ] ./sklearn/metrics/_pairwise_distances_reduction/_radius_neighbors_classmode.pyx.tp
- [ ] ./sklearn/ensemble/_hist_gradient_boosting/common.pxd
- [ ] ./sklearn/ensemble/_hist_gradient_boosting/_bitset.pyx
- [ ] ./sklearn/ensemble/_hist_gradient_boosting/_predictor.pyx
- [ ] ./sklearn/ensemble/_hist_gradient_boosting/splitting.pyx
- [ ] ./sklearn/ensemble/_hist_gradient_boosting/histogram.pyx
- [ ] ./sklearn/ensemble/_hist_gradient_boosting/_binning.pyx
- [ ] ./sklearn/ensemble/_hist_gradient_boosting/_bitset.pxd
- [ ] ./sklearn/linear_model/_sgd_fast.pyx.tp

~Is there any reason to leave these as `unsigned char` vs replacing them with the `uint8_t` to consolidate our types?~

To address this issue, it would be best to open up a PR one-by-one and implement the said change and cimport statement, and then link to the issue here.

COMMENT:
@norgera you can change it all at once actually. I noticed the diff shouldn't be too big.

Labels: good first issue, help wanted, cython
MAINT Replace `unsigned char` with `uint8_t` in codebase

In https://github.com/scikit-learn/scikit-learn/issues/25572, we defined typedefs for commonly used Cython types throughout the codebase. Running `grep -rl "unsigned char" ./sklearn`, I found the following files contain `unsigned char`, which could be replaced with `uint8_t` from  https://github.com/scikit-learn/scikit-learn/blob/main/sklearn/utils/_typedefs.pxd.

- [ ] ./sklearn/metrics/_pairwise_distances_reduction/_radius_neighbors_classmode.pyx.tp
- [ ] ./sklearn/ensemble/_hist_gradient_boosting/common.pxd
- [ ] ./sklearn/ensemble/_hist_gradient_boosting/_bitset.pyx
- [ ] ./sklearn/ensemble/_hist_gradient_boosting/_predictor.pyx
- [ ] ./sklearn/ensemble/_hist_gradient_boosting/splitting.pyx
- [ ] ./sklearn/ensemble/_hist_gradient_boosting/histogram.pyx
- [ ] ./sklearn/ensemble/_hist_gradient_boosting/_binning.pyx
- [ ] ./sklearn/ensemble/_hist_gradient_boosting/_bitset.pxd
- [ ] ./sklearn/linear_model/_sgd_fast.pyx.tp

~Is there any reason to leave these as `unsigned char` vs replacing them with the `uint8_t` to consolidate our types?~

To address this issue, it would be best to open up a PR one-by-one and implement the said change and cimport statement, and then link to the issue here.

COMMENT:
Notice https://github.com/scikit-learn/scikit-learn/pull/29596/files#diff-dcb6bf0d5b5d5eff1396b54e36697cf407823cbc89cd30bae426b8fdb1141881 won't work as you need to cimport the relevant type.

Labels: good first issue, help wanted, cython
MAINT Replace `unsigned char` with `uint8_t` in codebase

In https://github.com/scikit-learn/scikit-learn/issues/25572, we defined typedefs for commonly used Cython types throughout the codebase. Running `grep -rl "unsigned char" ./sklearn`, I found the following files contain `unsigned char`, which could be replaced with `uint8_t` from  https://github.com/scikit-learn/scikit-learn/blob/main/sklearn/utils/_typedefs.pxd.

- [ ] ./sklearn/metrics/_pairwise_distances_reduction/_radius_neighbors_classmode.pyx.tp
- [ ] ./sklearn/ensemble/_hist_gradient_boosting/common.pxd
- [ ] ./sklearn/ensemble/_hist_gradient_boosting/_bitset.pyx
- [ ] ./sklearn/ensemble/_hist_gradient_boosting/_predictor.pyx
- [ ] ./sklearn/ensemble/_hist_gradient_boosting/splitting.pyx
- [ ] ./sklearn/ensemble/_hist_gradient_boosting/histogram.pyx
- [ ] ./sklearn/ensemble/_hist_gradient_boosting/_binning.pyx
- [ ] ./sklearn/ensemble/_hist_gradient_boosting/_bitset.pxd
- [ ] ./sklearn/linear_model/_sgd_fast.pyx.tp

~Is there any reason to leave these as `unsigned char` vs replacing them with the `uint8_t` to consolidate our types?~

To address this issue, it would be best to open up a PR one-by-one and implement the said change and cimport statement, and then link to the issue here.

COMMENT:
Yes I realized that quickly, thank you for the clarification. I will change the rest of the files now

Labels: good first issue, help wanted, cython
MAINT Replace `unsigned char` with `uint8_t` in codebase

In https://github.com/scikit-learn/scikit-learn/issues/25572, we defined typedefs for commonly used Cython types throughout the codebase. Running `grep -rl "unsigned char" ./sklearn`, I found the following files contain `unsigned char`, which could be replaced with `uint8_t` from  https://github.com/scikit-learn/scikit-learn/blob/main/sklearn/utils/_typedefs.pxd.

- [ ] ./sklearn/metrics/_pairwise_distances_reduction/_radius_neighbors_classmode.pyx.tp
- [ ] ./sklearn/ensemble/_hist_gradient_boosting/common.pxd
- [ ] ./sklearn/ensemble/_hist_gradient_boosting/_bitset.pyx
- [ ] ./sklearn/ensemble/_hist_gradient_boosting/_predictor.pyx
- [ ] ./sklearn/ensemble/_hist_gradient_boosting/splitting.pyx
- [ ] ./sklearn/ensemble/_hist_gradient_boosting/histogram.pyx
- [ ] ./sklearn/ensemble/_hist_gradient_boosting/_binning.pyx
- [ ] ./sklearn/ensemble/_hist_gradient_boosting/_bitset.pxd
- [ ] ./sklearn/linear_model/_sgd_fast.pyx.tp

~Is there any reason to leave these as `unsigned char` vs replacing them with the `uint8_t` to consolidate our types?~

To address this issue, it would be best to open up a PR one-by-one and implement the said change and cimport statement, and then link to the issue here.

COMMENT:
@adam2392 I believe I have made the necessary changes. Let me know if there's anything I missed

Sorry making 2 pr's and multiple commits . I am new to open source and have learned a lot from this.

Labels: good first issue, help wanted, cython
Truly parallel execution of pairwise_kernels and pairwise_distances

### Describe the workflow you want to enable

Both `pairwise_kernels` and `pairwise_distances` functions call `_parallel_pairwise` function, which is (contrary to its name) not parallel as it enforces the threading backend. Therefore, these functions are terribly slow, especially for computationally expensive user-defined metrics. I understand that the reasons for the threading backend are possibly large memory demands and data communication overhead but I suggest a different approach. Also, the documentation for these functions talks about parallel execution and processes which is currently simply not true.

### Describe your proposed solution

The memory and data communication issues can be reduced by a smarter distribution of the input data to individual processes. Right now, only `Y` is sliced in the `_parallel_pairwise` function which is suboptimal for parallel processing. Both `X` and `Y` should be sliced to lower the demands for multiprocessing. For example for 100x100 `X` and `Y` distributed to 100 processes, we have to copy 100+1 inputs to every process when slicing only `Y` while only 10+10 when slicing both `X` and `Y`. As a result, multiprocessing can be allowed. Also, joblib does automatic memmapping in some cases.

Alternatively, at least the documentation for  `pairwise_kernels` and `pairwise_distances` should be corrected.

### Describe alternatives you've considered, if relevant

_No response_

### Additional context

_No response_

COMMENT:
>> Furthermore, CPython 3.13 will come with an optional build flag that should a allow to run without any GIL (also known as the "free threading" mode of Python)

> That would be super nice to have. The question is how long it might take to make it practically usable in sklearn.

This is already usable in the sense that we have a CI for CPython 3.13 free-threaded and all the scikit-learn tests pass. I you have time to try CPython 3.13 free-threaded on your particular use case, feed-back would be super useful and welcome :pray:.

As of today, you have a few different ways to install CPython 3.13 free-threaded locally (through conda, your Linux distribution package manager, python.org download, etc ...) see the [py-free-threading.github.io doc](https://py-free-threading.github.io/installing_cpython/). numpy, scipy, scikit-learn, and other projects have development wheels for CPython 3.13 free-threaded see [this doc](https://py-free-threading.github.io/tracking/).

With this kind of "ongoing work" things, there may be some caveats along the road, for example my understanding is that there is currently a hit on single-threaded performance in CPython 3.13 free-threaded, but this will be improved in the future. Not sure about the details, this may be quickly mentioned at one point in [Anthony Shaw's PyCon US 2024 talk video](https://www.youtube.com/watch?v=lCjsy7tb420&t=1825).

Labels: New Feature
Truly parallel execution of pairwise_kernels and pairwise_distances

### Describe the workflow you want to enable

Both `pairwise_kernels` and `pairwise_distances` functions call `_parallel_pairwise` function, which is (contrary to its name) not parallel as it enforces the threading backend. Therefore, these functions are terribly slow, especially for computationally expensive user-defined metrics. I understand that the reasons for the threading backend are possibly large memory demands and data communication overhead but I suggest a different approach. Also, the documentation for these functions talks about parallel execution and processes which is currently simply not true.

### Describe your proposed solution

The memory and data communication issues can be reduced by a smarter distribution of the input data to individual processes. Right now, only `Y` is sliced in the `_parallel_pairwise` function which is suboptimal for parallel processing. Both `X` and `Y` should be sliced to lower the demands for multiprocessing. For example for 100x100 `X` and `Y` distributed to 100 processes, we have to copy 100+1 inputs to every process when slicing only `Y` while only 10+10 when slicing both `X` and `Y`. As a result, multiprocessing can be allowed. Also, joblib does automatic memmapping in some cases.

Alternatively, at least the documentation for  `pairwise_kernels` and `pairwise_distances` should be corrected.

### Describe alternatives you've considered, if relevant

_No response_

### Additional context

_No response_

COMMENT:
Thank you very much for your analysis. Looking forward to see the results with free-threading Python if someone has time to set it up.

Labels: New Feature
Truly parallel execution of pairwise_kernels and pairwise_distances

### Describe the workflow you want to enable

Both `pairwise_kernels` and `pairwise_distances` functions call `_parallel_pairwise` function, which is (contrary to its name) not parallel as it enforces the threading backend. Therefore, these functions are terribly slow, especially for computationally expensive user-defined metrics. I understand that the reasons for the threading backend are possibly large memory demands and data communication overhead but I suggest a different approach. Also, the documentation for these functions talks about parallel execution and processes which is currently simply not true.

### Describe your proposed solution

The memory and data communication issues can be reduced by a smarter distribution of the input data to individual processes. Right now, only `Y` is sliced in the `_parallel_pairwise` function which is suboptimal for parallel processing. Both `X` and `Y` should be sliced to lower the demands for multiprocessing. For example for 100x100 `X` and `Y` distributed to 100 processes, we have to copy 100+1 inputs to every process when slicing only `Y` while only 10+10 when slicing both `X` and `Y`. As a result, multiprocessing can be allowed. Also, joblib does automatic memmapping in some cases.

Alternatively, at least the documentation for  `pairwise_kernels` and `pairwise_distances` should be corrected.

### Describe alternatives you've considered, if relevant

_No response_

### Additional context

_No response_

COMMENT:
@stepan-srsen could you add the output of `python -c 'import sklearn; sklearn.show_versions()'` to your earlier comment i.e. https://github.com/scikit-learn/scikit-learn/issues/29587#issuecomment-2260837369, this could be useful for further reference :pray:.

I am trying to run your snippet and full-disclosure for now I am getting weird results on vanilla Python, so I must be doing something wrong I need to have a a closer look ...

Labels: New Feature
Truly parallel execution of pairwise_kernels and pairwise_distances

### Describe the workflow you want to enable

Both `pairwise_kernels` and `pairwise_distances` functions call `_parallel_pairwise` function, which is (contrary to its name) not parallel as it enforces the threading backend. Therefore, these functions are terribly slow, especially for computationally expensive user-defined metrics. I understand that the reasons for the threading backend are possibly large memory demands and data communication overhead but I suggest a different approach. Also, the documentation for these functions talks about parallel execution and processes which is currently simply not true.

### Describe your proposed solution

The memory and data communication issues can be reduced by a smarter distribution of the input data to individual processes. Right now, only `Y` is sliced in the `_parallel_pairwise` function which is suboptimal for parallel processing. Both `X` and `Y` should be sliced to lower the demands for multiprocessing. For example for 100x100 `X` and `Y` distributed to 100 processes, we have to copy 100+1 inputs to every process when slicing only `Y` while only 10+10 when slicing both `X` and `Y`. As a result, multiprocessing can be allowed. Also, joblib does automatic memmapping in some cases.

Alternatively, at least the documentation for  `pairwise_kernels` and `pairwise_distances` should be corrected.

### Describe alternatives you've considered, if relevant

_No response_

### Additional context

_No response_

COMMENT:
@stepan-srsen just to make sure you do set `MKL_NUM_THREADS=1` (vanilla Python run since you are using MKL in your environment) or `OPENBLAS_NUM_THREADS=1` (free-threaded Python since you are using OpenBLAS here)?

I think not setting `MKL_NUM_THREADS=1` was one of the reason I was seeing surprising results originally.

I was also wondering how the outer-loop parallelism performance (i.e. what you are showing) with the BLAS library in single-threaded mode, compares to no parallellism in the outer loop and let the BLAS library use all the cores, but I did not have time to look at this ...

Labels: New Feature