Talip7/scikit-learn-issues-multilabel huggingface dataset:
Introduction of `Waiting for Second Reviewer` tag

## Motivation
PRs commonly fall into a state where they have one approval from an initial reviewer, however no attention from any secondary reviewers. Arguably the most effective mechanism of dissolving PR backlog is exactly to target these sorts of PRs, since the initial approval implies that the underlying quality/status of the work is not too far from mergeable, and the bottleneck lies with a lack of reviewer attention rather than the contributors/authors. Providing a specific tag for this will hopefully make it easier for core developers to find these PRs and push for final merger.

## Current Solution
Currently we have the `Waiting for Reviewer` tag, applied to ~40 PRs, which is generally used to indicate/request extra attention to a PR. This, in combination with a filter for PRs with existing approval ([for example](https://github.com/scikit-learn/scikit-learn/pulls?q=is%3Aopen+is%3Apr+label%3A%22Waiting+for+Reviewer%22+review%3Aapproved)) then results in PRs that, on paper, should also be mergeable with a single extra approval. However there are some flaws to this:

1. The `Waiting for Reviewer` tag is a bit redundant and ill-defined. Every PR, short of approval, is in some sense "waiting for a reviewer", unless it is very specifically waiting on an author to implement changes from existing review. This results in the `Waiting for Reviewer` tag's meaning being muddled.
2. Labeling with the `Waiting for Reviewer` tag is manual. While it could be automated, as it exists right now there is little sense in doing so since -- again -- every PR is in some sense "waiting for a reviewer". This means that coverage is not guaranteed to be thorough and that the label is not exhaustive/surjective.
3. Some PRs which satisfy the above filter cannot be easily merged by another reviewer. In particular, if a PR with the `Waiting for Reviewer` tag and one approval has a review which has requested _changes_ then it would be a false-positive in the sense that the review ought to be continued by the initial reviewer that requested changes once those requests have been addressed.

## Proposed Solution
A very specific `Waiting for Second Reviewer` tag. A PR is eligible for this tag when:
1. It has been approved by one reviewer with commit rights.
2. It has NOT been reviewed by a second reviewer with commit rights (including reviews with `approval`, `comments`, and `changes requested`)

The narrow scope of the tag would also make it relatively simple to automate (hopefully 😄).

## Next Steps (?)
I wonder if we should formalize/clarify the role of `Waiting for Reviewer`, with or without the acceptance of this new tag.

Note: This was discussed in the Drafting Meeting on 10/07/2022: [notes here](https://github.com/Micky774/administrative/blob/drafting_notes/drafting_meetings/2022-10-07)

COMMENT:
Maybe we should clarify that PRs that are tagged `Needs _` or `Stalled` won't be tagged with `Waiting for Second Reviewer` to make this more exclusive to PRs that only need a review before being merged

Labels: workflow
Introduction of `Waiting for Second Reviewer` tag

## Motivation
PRs commonly fall into a state where they have one approval from an initial reviewer, however no attention from any secondary reviewers. Arguably the most effective mechanism of dissolving PR backlog is exactly to target these sorts of PRs, since the initial approval implies that the underlying quality/status of the work is not too far from mergeable, and the bottleneck lies with a lack of reviewer attention rather than the contributors/authors. Providing a specific tag for this will hopefully make it easier for core developers to find these PRs and push for final merger.

## Current Solution
Currently we have the `Waiting for Reviewer` tag, applied to ~40 PRs, which is generally used to indicate/request extra attention to a PR. This, in combination with a filter for PRs with existing approval ([for example](https://github.com/scikit-learn/scikit-learn/pulls?q=is%3Aopen+is%3Apr+label%3A%22Waiting+for+Reviewer%22+review%3Aapproved)) then results in PRs that, on paper, should also be mergeable with a single extra approval. However there are some flaws to this:

1. The `Waiting for Reviewer` tag is a bit redundant and ill-defined. Every PR, short of approval, is in some sense "waiting for a reviewer", unless it is very specifically waiting on an author to implement changes from existing review. This results in the `Waiting for Reviewer` tag's meaning being muddled.
2. Labeling with the `Waiting for Reviewer` tag is manual. While it could be automated, as it exists right now there is little sense in doing so since -- again -- every PR is in some sense "waiting for a reviewer". This means that coverage is not guaranteed to be thorough and that the label is not exhaustive/surjective.
3. Some PRs which satisfy the above filter cannot be easily merged by another reviewer. In particular, if a PR with the `Waiting for Reviewer` tag and one approval has a review which has requested _changes_ then it would be a false-positive in the sense that the review ought to be continued by the initial reviewer that requested changes once those requests have been addressed.

## Proposed Solution
A very specific `Waiting for Second Reviewer` tag. A PR is eligible for this tag when:
1. It has been approved by one reviewer with commit rights.
2. It has NOT been reviewed by a second reviewer with commit rights (including reviews with `approval`, `comments`, and `changes requested`)

The narrow scope of the tag would also make it relatively simple to automate (hopefully 😄).

## Next Steps (?)
I wonder if we should formalize/clarify the role of `Waiting for Reviewer`, with or without the acceptance of this new tag.

Note: This was discussed in the Drafting Meeting on 10/07/2022: [notes here](https://github.com/Micky774/administrative/blob/drafting_notes/drafting_meetings/2022-10-07)

COMMENT:
I am closing this issue since the tag is already in use.

Labels: workflow
Does this line in `Classifier comparison' produce a test data leakage?

### Describe the issue linked to the documentation

I feel there's a line here:
```
X = StandardScaler().fit_transform(X)
    X_train, X_test, y_train, y_test = train_test_split(
        X, y, test_size=0.4, random_state=42
    )
 x_min, x_max = X[:, 0].min() - 0.5, X[:, 0].max() + 0.5
    y_min, y_max = X[:, 1].min() - 0.5, X[:, 1].max() + 0.5
```

from this page (https://scikit-learn.org/stable/auto_examples/classification/plot_classifier_comparison.html) that produces a test data leakage.

Shouldn't it be something like:
```
X_train, X_test, y_train, y_test = train_test_split(
        X, y, test_size=0.4, random_state=42
    )
    xscaler = StandardScaler().fit(X_train)
    X_train = xscaler.transform(X_train)
    X_test = xscaler.transform(X_test)
 x_min, x_max = X_train[:, 0].min() - 0.5, X_train[:, 0].max() + 0.5
    y_min, y_max = X_train[:, 1].min() - 0.5, X_train[:, 1].max() + 0.5
```
as in this page, https://scikit-learn.org/stable/modules/preprocessing.html.

> This class implements the Transformer API to compute the mean and standard deviation on a training set so as to be able to later re-apply the same transformation on the testing set. 

### Suggest a potential alternative/fix

```
X_train, X_test, y_train, y_test = train_test_split(
        X, y, test_size=0.4, random_state=42
    )
    xscaler = StandardScaler().fit(X_train)
    X_train = xscaler.transform(X_train)
    X_test = xscaler.transform(X_test)
 x_min, x_max = X_train[:, 0].min() - 0.5, X_train[:, 0].max() + 0.5
    y_min, y_max = X_train[:, 1].min() - 0.5, X_train[:, 1].max() + 0.5
```

COMMENT:
This has been solved in `main` already by introducing a `pipeline` where the first stage is a `StandardScaler` and is fitted only on the training data.

https://scikit-learn.org/dev/auto_examples/classification/plot_classifier_comparison.html

Labels: Documentation, Needs Triage
Does this line in `Classifier comparison' produce a test data leakage?

### Describe the issue linked to the documentation

I feel there's a line here:
```
X = StandardScaler().fit_transform(X)
    X_train, X_test, y_train, y_test = train_test_split(
        X, y, test_size=0.4, random_state=42
    )
 x_min, x_max = X[:, 0].min() - 0.5, X[:, 0].max() + 0.5
    y_min, y_max = X[:, 1].min() - 0.5, X[:, 1].max() + 0.5
```

from this page (https://scikit-learn.org/stable/auto_examples/classification/plot_classifier_comparison.html) that produces a test data leakage.

Shouldn't it be something like:
```
X_train, X_test, y_train, y_test = train_test_split(
        X, y, test_size=0.4, random_state=42
    )
    xscaler = StandardScaler().fit(X_train)
    X_train = xscaler.transform(X_train)
    X_test = xscaler.transform(X_test)
 x_min, x_max = X_train[:, 0].min() - 0.5, X_train[:, 0].max() + 0.5
    y_min, y_max = X_train[:, 1].min() - 0.5, X_train[:, 1].max() + 0.5
```
as in this page, https://scikit-learn.org/stable/modules/preprocessing.html.

> This class implements the Transformer API to compute the mean and standard deviation on a training set so as to be able to later re-apply the same transformation on the testing set. 

### Suggest a potential alternative/fix

```
X_train, X_test, y_train, y_test = train_test_split(
        X, y, test_size=0.4, random_state=42
    )
    xscaler = StandardScaler().fit(X_train)
    X_train = xscaler.transform(X_train)
    X_test = xscaler.transform(X_test)
 x_min, x_max = X_train[:, 0].min() - 0.5, X_train[:, 0].max() + 0.5
    y_min, y_max = X_train[:, 1].min() - 0.5, X_train[:, 1].max() + 0.5
```

COMMENT:
I don’t see any ‘main’ or ‘pipeline’ word in this ‘plot_classifier_comparison.py’ file. 

> On 19 Oct BE 2565, at 03:44, Guillaume Lemaitre ***@***.***> wrote:
> 
> ﻿
> Closed #24695 as completed.
> 
> —
> Reply to this email directly, view it on GitHub, or unsubscribe.
> You are receiving this because you authored the thread.

Labels: Documentation, Needs Triage
Does this line in `Classifier comparison' produce a test data leakage?

### Describe the issue linked to the documentation

I feel there's a line here:
```
X = StandardScaler().fit_transform(X)
    X_train, X_test, y_train, y_test = train_test_split(
        X, y, test_size=0.4, random_state=42
    )
 x_min, x_max = X[:, 0].min() - 0.5, X[:, 0].max() + 0.5
    y_min, y_max = X[:, 1].min() - 0.5, X[:, 1].max() + 0.5
```

from this page (https://scikit-learn.org/stable/auto_examples/classification/plot_classifier_comparison.html) that produces a test data leakage.

Shouldn't it be something like:
```
X_train, X_test, y_train, y_test = train_test_split(
        X, y, test_size=0.4, random_state=42
    )
    xscaler = StandardScaler().fit(X_train)
    X_train = xscaler.transform(X_train)
    X_test = xscaler.transform(X_test)
 x_min, x_max = X_train[:, 0].min() - 0.5, X_train[:, 0].max() + 0.5
    y_min, y_max = X_train[:, 1].min() - 0.5, X_train[:, 1].max() + 0.5
```
as in this page, https://scikit-learn.org/stable/modules/preprocessing.html.

> This class implements the Transformer API to compute the mean and standard deviation on a training set so as to be able to later re-apply the same transformation on the testing set. 

### Suggest a potential alternative/fix

```
X_train, X_test, y_train, y_test = train_test_split(
        X, y, test_size=0.4, random_state=42
    )
    xscaler = StandardScaler().fit(X_train)
    X_train = xscaler.transform(X_train)
    X_test = xscaler.transform(X_test)
 x_min, x_max = X_train[:, 0].min() - 0.5, X_train[:, 0].max() + 0.5
    y_min, y_max = X_train[:, 1].min() - 0.5, X_train[:, 1].max() + 0.5
```

COMMENT:
`main` is the branch. Basically, one need to check the `dev` documentation (the link that I pasted earlier).

The relevant line is:

```python
clf = make_pipeline(StandardScaler(), clf)
```

in the `for` loop

Labels: Documentation, Needs Triage
Does this line in `Classifier comparison' produce a test data leakage?

### Describe the issue linked to the documentation

I feel there's a line here:
```
X = StandardScaler().fit_transform(X)
    X_train, X_test, y_train, y_test = train_test_split(
        X, y, test_size=0.4, random_state=42
    )
 x_min, x_max = X[:, 0].min() - 0.5, X[:, 0].max() + 0.5
    y_min, y_max = X[:, 1].min() - 0.5, X[:, 1].max() + 0.5
```

from this page (https://scikit-learn.org/stable/auto_examples/classification/plot_classifier_comparison.html) that produces a test data leakage.

Shouldn't it be something like:
```
X_train, X_test, y_train, y_test = train_test_split(
        X, y, test_size=0.4, random_state=42
    )
    xscaler = StandardScaler().fit(X_train)
    X_train = xscaler.transform(X_train)
    X_test = xscaler.transform(X_test)
 x_min, x_max = X_train[:, 0].min() - 0.5, X_train[:, 0].max() + 0.5
    y_min, y_max = X_train[:, 1].min() - 0.5, X_train[:, 1].max() + 0.5
```
as in this page, https://scikit-learn.org/stable/modules/preprocessing.html.

> This class implements the Transformer API to compute the mean and standard deviation on a training set so as to be able to later re-apply the same transformation on the testing set. 

### Suggest a potential alternative/fix

```
X_train, X_test, y_train, y_test = train_test_split(
        X, y, test_size=0.4, random_state=42
    )
    xscaler = StandardScaler().fit(X_train)
    X_train = xscaler.transform(X_train)
    X_test = xscaler.transform(X_test)
 x_min, x_max = X_train[:, 0].min() - 0.5, X_train[:, 0].max() + 0.5
    y_min, y_max = X_train[:, 1].min() - 0.5, X_train[:, 1].max() + 0.5
```

COMMENT:
I see. The latest stable release still has this error but it has been corrected and will be included in the next release.

> On 19 Oct BE 2565, at 04:09, Guillaume Lemaitre ***@***.***> wrote:
> 
> ﻿
> main is the branch. Basically, one need to check the dev documentation (the link that I pasted earlier).
> 
> The relevant line is:
> 
> clf = make_pipeline(StandardScaler(), clf)
> in the for loop
> 
> —
> Reply to this email directly, view it on GitHub, or unsubscribe.
> You are receiving this because you authored the thread.

Labels: Documentation, Needs Triage
Does this line in `Classifier comparison' produce a test data leakage?

### Describe the issue linked to the documentation

I feel there's a line here:
```
X = StandardScaler().fit_transform(X)
    X_train, X_test, y_train, y_test = train_test_split(
        X, y, test_size=0.4, random_state=42
    )
 x_min, x_max = X[:, 0].min() - 0.5, X[:, 0].max() + 0.5
    y_min, y_max = X[:, 1].min() - 0.5, X[:, 1].max() + 0.5
```

from this page (https://scikit-learn.org/stable/auto_examples/classification/plot_classifier_comparison.html) that produces a test data leakage.

Shouldn't it be something like:
```
X_train, X_test, y_train, y_test = train_test_split(
        X, y, test_size=0.4, random_state=42
    )
    xscaler = StandardScaler().fit(X_train)
    X_train = xscaler.transform(X_train)
    X_test = xscaler.transform(X_test)
 x_min, x_max = X_train[:, 0].min() - 0.5, X_train[:, 0].max() + 0.5
    y_min, y_max = X_train[:, 1].min() - 0.5, X_train[:, 1].max() + 0.5
```
as in this page, https://scikit-learn.org/stable/modules/preprocessing.html.

> This class implements the Transformer API to compute the mean and standard deviation on a training set so as to be able to later re-apply the same transformation on the testing set. 

### Suggest a potential alternative/fix

```
X_train, X_test, y_train, y_test = train_test_split(
        X, y, test_size=0.4, random_state=42
    )
    xscaler = StandardScaler().fit(X_train)
    X_train = xscaler.transform(X_train)
    X_test = xscaler.transform(X_test)
 x_min, x_max = X_train[:, 0].min() - 0.5, X_train[:, 0].max() + 0.5
    y_min, y_max = X_train[:, 1].min() - 0.5, X_train[:, 1].max() + 0.5
```

COMMENT:
Yes we are going to release in the upcoming month the documentation and library code.

Labels: Documentation, Needs Triage
Does this line in `Classifier comparison' produce a test data leakage?

### Describe the issue linked to the documentation

I feel there's a line here:
```
X = StandardScaler().fit_transform(X)
    X_train, X_test, y_train, y_test = train_test_split(
        X, y, test_size=0.4, random_state=42
    )
 x_min, x_max = X[:, 0].min() - 0.5, X[:, 0].max() + 0.5
    y_min, y_max = X[:, 1].min() - 0.5, X[:, 1].max() + 0.5
```

from this page (https://scikit-learn.org/stable/auto_examples/classification/plot_classifier_comparison.html) that produces a test data leakage.

Shouldn't it be something like:
```
X_train, X_test, y_train, y_test = train_test_split(
        X, y, test_size=0.4, random_state=42
    )
    xscaler = StandardScaler().fit(X_train)
    X_train = xscaler.transform(X_train)
    X_test = xscaler.transform(X_test)
 x_min, x_max = X_train[:, 0].min() - 0.5, X_train[:, 0].max() + 0.5
    y_min, y_max = X_train[:, 1].min() - 0.5, X_train[:, 1].max() + 0.5
```
as in this page, https://scikit-learn.org/stable/modules/preprocessing.html.

> This class implements the Transformer API to compute the mean and standard deviation on a training set so as to be able to later re-apply the same transformation on the testing set. 

### Suggest a potential alternative/fix

```
X_train, X_test, y_train, y_test = train_test_split(
        X, y, test_size=0.4, random_state=42
    )
    xscaler = StandardScaler().fit(X_train)
    X_train = xscaler.transform(X_train)
    X_test = xscaler.transform(X_test)
 x_min, x_max = X_train[:, 0].min() - 0.5, X_train[:, 0].max() + 0.5
    y_min, y_max = X_train[:, 1].min() - 0.5, X_train[:, 1].max() + 0.5
```

COMMENT:
Best of lucks for users of these stable versions :). 

> On 19 Oct BE 2565, at 04:22, Guillaume Lemaitre ***@***.***> wrote:
> 
> ﻿
> Yes we are going to release in the upcoming month the documentation and library code.
> 
> —
> Reply to this email directly, view it on GitHub, or unsubscribe.
> You are receiving this because you authored the thread.

Labels: Documentation, Needs Triage
CI "no OpenMP" build environment actually has OpenMP

This avoided catching a regression where an unprotected `cimport openmp` was introduced.

As a side-comment: Pyodide build needs to be built without OpenMP.

From https://github.com/scikit-learn/scikit-learn/pull/24682#issuecomment-1281939439, there is OpenMP in the build environment:

```
❯ ag openmp build_tools/azure/pylatest_conda_mkl_no_openmp_osx-64_conda.lock 
9:https://repo.anaconda.com/pkgs/main/osx-64/intel-openmp-2021.4.0-hecd8cb5_3538.conda#65e79d0ffef79cbb8ebd3c71e74eb50a
15:https://repo.anaconda.com/pkgs/main/osx-64/llvm-openmp-14.0.6-h0dcd299_0.conda#b5804d32b87dc61ca94561ade33d5f2d
```

From https://github.com/scikit-learn/scikit-learn/pull/24682#issuecomment-1282012785

Looking at why we get OpenMP in the "no OpenMP" build:

- libopenblas can be compiled without openmp i.e. with pthreads for Linux and Windows, e.g. see [this](https://conda-forge.org/docs/user/announcements.html#conda-forge-is-building-openblas-with-both-pthreads-and-openmp-on-linux)
- there is no libopenblas with pthreads on OSX [anaconda.org/conda-forge/libopenblas/files?sort=basename&sort_order=desc](https://anaconda.org/conda-forge/libopenblas/files?sort=basename&sort_order=desc)

So it seems like if we want an "no OpenMP" build we need it to be Linux or Windows. Not sure whether there was a good reason to have it on OSX originally.

COMMENT:
> Not sure whether there was a good reason to have it on OSX originally.

The original reason for disability OpenMP was because the default compiler on OSX did not have OpenMP: https://github.com/scikit-learn/scikit-learn/pull/15174

I think using linux to test no OpenMP is sufficient and +1 on moving the CI over.

Labels: Build / CI
CI "no OpenMP" build environment actually has OpenMP

This avoided catching a regression where an unprotected `cimport openmp` was introduced.

As a side-comment: Pyodide build needs to be built without OpenMP.

From https://github.com/scikit-learn/scikit-learn/pull/24682#issuecomment-1281939439, there is OpenMP in the build environment:

```
❯ ag openmp build_tools/azure/pylatest_conda_mkl_no_openmp_osx-64_conda.lock 
9:https://repo.anaconda.com/pkgs/main/osx-64/intel-openmp-2021.4.0-hecd8cb5_3538.conda#65e79d0ffef79cbb8ebd3c71e74eb50a
15:https://repo.anaconda.com/pkgs/main/osx-64/llvm-openmp-14.0.6-h0dcd299_0.conda#b5804d32b87dc61ca94561ade33d5f2d
```

From https://github.com/scikit-learn/scikit-learn/pull/24682#issuecomment-1282012785

Looking at why we get OpenMP in the "no OpenMP" build:

- libopenblas can be compiled without openmp i.e. with pthreads for Linux and Windows, e.g. see [this](https://conda-forge.org/docs/user/announcements.html#conda-forge-is-building-openblas-with-both-pthreads-and-openmp-on-linux)
- there is no libopenblas with pthreads on OSX [anaconda.org/conda-forge/libopenblas/files?sort=basename&sort_order=desc](https://anaconda.org/conda-forge/libopenblas/files?sort=basename&sort_order=desc)

So it seems like if we want an "no OpenMP" build we need it to be Linux or Windows. Not sure whether there was a good reason to have it on OSX originally.

COMMENT:
In #25391 an unprotected `cimport openmp` will make the no-OpenMP build fail. I think this fixes the main issue.

Also, it does make sense to keep the OSX with system clang use case build, since this may be something that happens in practice for some users and we prefer that the build works with a warning. That means (at least for now) that the no-OpenMP build has OpenMP, which is a bit weird but oh well :shrug:.

For completeness I could not find a way to create a conda environment without OpenMP. For example on Linux as soon as you have `libgcc-ng` you will get `libgomp`, so for example creating an environment with `python` will bring in `libgomp`.

In a not so far future, we may have a Pyodide build, which will be a "no-OpenMP whatsoever" build.

Labels: Build / CI
CI "no OpenMP" build environment actually has OpenMP

This avoided catching a regression where an unprotected `cimport openmp` was introduced.

As a side-comment: Pyodide build needs to be built without OpenMP.

From https://github.com/scikit-learn/scikit-learn/pull/24682#issuecomment-1281939439, there is OpenMP in the build environment:

```
❯ ag openmp build_tools/azure/pylatest_conda_mkl_no_openmp_osx-64_conda.lock 
9:https://repo.anaconda.com/pkgs/main/osx-64/intel-openmp-2021.4.0-hecd8cb5_3538.conda#65e79d0ffef79cbb8ebd3c71e74eb50a
15:https://repo.anaconda.com/pkgs/main/osx-64/llvm-openmp-14.0.6-h0dcd299_0.conda#b5804d32b87dc61ca94561ade33d5f2d
```

From https://github.com/scikit-learn/scikit-learn/pull/24682#issuecomment-1282012785

Looking at why we get OpenMP in the "no OpenMP" build:

- libopenblas can be compiled without openmp i.e. with pthreads for Linux and Windows, e.g. see [this](https://conda-forge.org/docs/user/announcements.html#conda-forge-is-building-openblas-with-both-pthreads-and-openmp-on-linux)
- there is no libopenblas with pthreads on OSX [anaconda.org/conda-forge/libopenblas/files?sort=basename&sort_order=desc](https://anaconda.org/conda-forge/libopenblas/files?sort=basename&sort_order=desc)

So it seems like if we want an "no OpenMP" build we need it to be Linux or Windows. Not sure whether there was a good reason to have it on OSX originally.

COMMENT:
> That means (at least for now) that the no-OpenMP build has OpenMP, which is a bit weird but oh well 🤷.

To be precise, there's openmp in the env, but scikit-learn is not built against openmp.

Labels: Build / CI