Talip7/scikit-learn-issues-multilabel huggingface dataset:
plot_learning_curve.py should not sort the fit time axis before plotting

Dears,

About 10 months ago, the `plot_learning_curve.py` example was changed by Mr. @thomasjpfan to sort the `fit_time` plot axis.

In my humble opinion, that's wrong because a learning curve is train-size ascending regardless the time it spent training.  For very small train sizes/intervals one can se a slightly smaller train size taking a slightly longer time to fit.

Furthermore, a computer performance may oscillates in any learning curve step which should not affect the actual order of the learning curve steps in the plot.

https://github.com/scikit-learn/scikit-learn/blob/ae6bf39b310ed5bb46349c831a05f55bba921dcc/examples/model_selection/plot_learning_curve.py#L168

COMMENT:
I assume we do so to have a more regular graph due to the resources used to run the example and build the documentation. Basically, we have a tiny sample size and we could be in the error bar and unluckily we could have a swap that would not be understandable in practice.

I agree with you that we should not do this in real-world cases. I am wondering if we could come up with a middle ground where we could add a comment to document this quirk.

Labels: Documentation
plot_learning_curve.py should not sort the fit time axis before plotting

Dears,

About 10 months ago, the `plot_learning_curve.py` example was changed by Mr. @thomasjpfan to sort the `fit_time` plot axis.

In my humble opinion, that's wrong because a learning curve is train-size ascending regardless the time it spent training.  For very small train sizes/intervals one can se a slightly smaller train size taking a slightly longer time to fit.

Furthermore, a computer performance may oscillates in any learning curve step which should not affect the actual order of the learning curve steps in the plot.

https://github.com/scikit-learn/scikit-learn/blob/ae6bf39b310ed5bb46349c831a05f55bba921dcc/examples/model_selection/plot_learning_curve.py#L168

COMMENT:
@mauriciocramos Do you wish contributing to fix this issue?

Labels: Documentation
plot_learning_curve.py should not sort the fit time axis before plotting

Dears,

About 10 months ago, the `plot_learning_curve.py` example was changed by Mr. @thomasjpfan to sort the `fit_time` plot axis.

In my humble opinion, that's wrong because a learning curve is train-size ascending regardless the time it spent training.  For very small train sizes/intervals one can se a slightly smaller train size taking a slightly longer time to fit.

Furthermore, a computer performance may oscillates in any learning curve step which should not affect the actual order of the learning curve steps in the plot.

https://github.com/scikit-learn/scikit-learn/blob/ae6bf39b310ed5bb46349c831a05f55bba921dcc/examples/model_selection/plot_learning_curve.py#L168

COMMENT:
The original sorting was done to resolve https://github.com/scikit-learn/scikit-learn/issues/21643

Maybe we can keep the order from the training sizes and use error bars instead?

```python
    # Plot fit_time vs score
    axes[2].grid()
    axes[2].errorbar(fit_times_mean, test_scores_mean, yerr=test_scores_std, ls="-")
    axes[2].set_xlabel("fit_times")
    axes[2].set_ylabel("Score")
    axes[2].set_title("Performance of the model")
```

![Screen Shot 2022-09-29 at 6 40 15 PM](https://user-images.githubusercontent.com/5402633/193154698-4e84d425-36e6-44f9-94a5-b82b9b598856.jpg)

**Edit** This would also not work nicely if `fit_times_mean` is not increasing:

```python
import matplotlib.pyplot as plt
plt.errorbar([0, 2, 1, 4], [1, 2, 3, 4], yerr=[0.1, 0.2, 0.3, 0.4])
```

![Screen Shot 2022-09-29 at 8 56 07 PM](https://user-images.githubusercontent.com/5402633/193167365-f6c1957d-d197-4251-8924-1a3d6058a97a.jpg)

Labels: Documentation
plot_learning_curve.py should not sort the fit time axis before plotting

Dears,

About 10 months ago, the `plot_learning_curve.py` example was changed by Mr. @thomasjpfan to sort the `fit_time` plot axis.

In my humble opinion, that's wrong because a learning curve is train-size ascending regardless the time it spent training.  For very small train sizes/intervals one can se a slightly smaller train size taking a slightly longer time to fit.

Furthermore, a computer performance may oscillates in any learning curve step which should not affect the actual order of the learning curve steps in the plot.

https://github.com/scikit-learn/scikit-learn/blob/ae6bf39b310ed5bb46349c831a05f55bba921dcc/examples/model_selection/plot_learning_curve.py#L168

COMMENT:
@glemaitre,

I appreciate your feedback.

IMHO, there would be no improvement other than change the example code not to sort the time vector in that example.  Despite it currently seems conceptually wrong, even with a big dataset (400K rows and thousands of columns) I've seen bigger sample sizes (100%) fitting in less time than smaller sample sizes (80%).  It frequently happens when the `learning_curve()` is working in the last (bigger) learning curve sample size.  I believe it emphasizes the time vector should not be sorted: The sequence of fit times are never guaranteed sorted but just expected.

Labels: Documentation
plot_learning_curve.py should not sort the fit time axis before plotting

Dears,

About 10 months ago, the `plot_learning_curve.py` example was changed by Mr. @thomasjpfan to sort the `fit_time` plot axis.

In my humble opinion, that's wrong because a learning curve is train-size ascending regardless the time it spent training.  For very small train sizes/intervals one can se a slightly smaller train size taking a slightly longer time to fit.

Furthermore, a computer performance may oscillates in any learning curve step which should not affect the actual order of the learning curve steps in the plot.

https://github.com/scikit-learn/scikit-learn/blob/ae6bf39b310ed5bb46349c831a05f55bba921dcc/examples/model_selection/plot_learning_curve.py#L168

COMMENT:
Hi @thomasjpfan,

Maybe I still don't understand the former issue https://github.com/scikit-learn/scikit-learn/issues/21643 but in my understandibg what it calls bad error regions, are really bad, but not wrong: the example would need more data to appears not bad.

Labels: Documentation
learning_curve() returning wrong (accumulated) times across parallel n_jobs

When running `learning_curve()` with parallel processing (`n_jobs` > 1) it wrongly returns `fit_times` and `score_times` as sums of their respective duration across all parallel jobs of `_fit_and_score()` rather than a meaningful, let's say, average.

This wrong aggregation seems to be caused by `_aggregate_score_dicts()` which is unpacking all parallel job times as a single vector.

https://github.com/scikit-learn/scikit-learn/blob/36958fb240fbe435673a9e3c52e769f01f36bec0/sklearn/model_selection/_validation.py#L1575

One solution could be to average fit and score times per `train_size`.

COMMENT:
The `_aggregate_score_dicts` only transforms a list of dictionaries into a dictionary of NumPy array. It does not alter or make any computation.

I don't think that there is any problem since the parallelism does not affect the fit or score time. We use parallelism to parallelize the cross-validation tasks. Thus, we would expect the fit and score time to remain the same whether we us `n_jobs=-1` or `n_jobs=1`.

Here is a small example with a large enough dataset to have a quite stable fit time estimation.

```python
from sklearn.datasets import make_classification
from sklearn.tree import DecisionTreeClassifier
from sklearn.model_selection import learning_curve
X, y = make_classification(n_samples=10_000, n_features=1_000, random_state=42)
tree = DecisionTreeClassifier(max_depth=100, random_state=42)
size, train_scores, test_scores, fit_time, score_time = learning_curve(
    tree, X, y, train_sizes=[0.3, 0.6, 0.9], return_times=True, cv=5, n_jobs=1
)
```

When `n_jobs=1`, we can check the average `fit_time` (in seconds):

```python
fit_time.mean(axis=1)
```

that outputs:

```
array([ 2.40500579,  7.81795702, 13.20850077])
```

Changing the `n_jobs=-1`, we get:

```
array([ 2.9412847 ,  9.49615512, 15.52491555])
```

Therefore, the fit times are quite close (they vary since I am not in a good benchmarking setting).

I could reproduce the same experiment for the score time.

Labels: Needs Triage
Reference for sklearn.feature_selection.chi2

### Describe the issue linked to the documentation

Hi folks,

I am somewhat in doubt that the `sklearn.feature_selection.chi2` function is implemented correctly. At least, checking the source code, it is entirely unclear to me why that kind of scoring would make sense.

Best,
Felix

### Suggest a potential alternative/fix

 Is there any reference that justifies the computation in the implemented way? Then it should be added to the documentation.

COMMENT:
As documented, the score makes sense to be used on the contingency matrix.
We need to correct the example that applies this feature selection to continuous features which do not make sense.

I close this issue because it would be a duplicate of https://github.com/scikit-learn/scikit-learn/issues/21455#issuecomment-977968841

Labels: Documentation, Needs Triage
Weekly CI run with NVidia GPU hardware

Now that #22554 was merged in `main`, it would be great to find a a way  to run a weekly scheduled job to run the scikit-learn `main` test on a CI worker with an NVidia GPU and CuPy.

In case of failure, it could create a report as [dedicated issues](https://github.com/scikit-learn/scikit-learn/issues?q=is%3Aissue+%22%E2%9A%A0%EF%B8%8F+ci+failed+on%22+) as we do for other scheduled jobs:

- https://github.com/scikit-learn/scikit-learn/blob/main/maint_tools/update_tracking_issue.py

Maybe @betatim has a plan and connections to do that? ;)

COMMENT:
Other opensource projects like dask and numba have accounts on https://gpuci.gpuopenanalytics.com/ for instance. Not sure what is the procedure to get scikit-learn accepted there (even with a limited quota of a few GPU hours per month).

Labels: Build / CI, help wanted, Array API
Weekly CI run with NVidia GPU hardware

Now that #22554 was merged in `main`, it would be great to find a a way  to run a weekly scheduled job to run the scikit-learn `main` test on a CI worker with an NVidia GPU and CuPy.

In case of failure, it could create a report as [dedicated issues](https://github.com/scikit-learn/scikit-learn/issues?q=is%3Aissue+%22%E2%9A%A0%EF%B8%8F+ci+failed+on%22+) as we do for other scheduled jobs:

- https://github.com/scikit-learn/scikit-learn/blob/main/maint_tools/update_tracking_issue.py

Maybe @betatim has a plan and connections to do that? ;)

COMMENT:
Ive started looking into this

Labels: Build / CI, help wanted, Array API
Weekly CI run with NVidia GPU hardware

Now that #22554 was merged in `main`, it would be great to find a a way  to run a weekly scheduled job to run the scikit-learn `main` test on a CI worker with an NVidia GPU and CuPy.

In case of failure, it could create a report as [dedicated issues](https://github.com/scikit-learn/scikit-learn/issues?q=is%3Aissue+%22%E2%9A%A0%EF%B8%8F+ci+failed+on%22+) as we do for other scheduled jobs:

- https://github.com/scikit-learn/scikit-learn/blob/main/maint_tools/update_tracking_issue.py

Maybe @betatim has a plan and connections to do that? ;)

COMMENT:
As discussed during the triaging meeting, we could use https://docs.cirun.io/ which is free for open source projects.

We would then need to register a credit card for our numfocus account on an AWS or Google Cloud.

If we do a few weekly runs, that should be cheap enough.

If we want to make it possible to trigger a GPU run from a git commit message in a PR we need to make sure that cirun would not spawn a GPU instance just to skip the build if the flag is not present in the git commit message, but it might be doable.

Labels: Build / CI, help wanted, Array API
Weekly CI run with NVidia GPU hardware

Now that #22554 was merged in `main`, it would be great to find a a way  to run a weekly scheduled job to run the scikit-learn `main` test on a CI worker with an NVidia GPU and CuPy.

In case of failure, it could create a report as [dedicated issues](https://github.com/scikit-learn/scikit-learn/issues?q=is%3Aissue+%22%E2%9A%A0%EF%B8%8F+ci+failed+on%22+) as we do for other scheduled jobs:

- https://github.com/scikit-learn/scikit-learn/blob/main/maint_tools/update_tracking_issue.py

Maybe @betatim has a plan and connections to do that? ;)

COMMENT:
Progress update as of a few weeks ago: I got `cirun.io` working on my fork, but I ran into an issue with it stalling with PyTorch. I suspect it was because I was using GPUs that were too old, but I ran out of time to debug.

To reduce cost:

- I had GitHub Actions build the wheel and ship it over to `cirun.io`.
- I had a prebuilt image with the cuda dependencies installed.

Labels: Build / CI, help wanted, Array API
Weekly CI run with NVidia GPU hardware

Now that #22554 was merged in `main`, it would be great to find a a way  to run a weekly scheduled job to run the scikit-learn `main` test on a CI worker with an NVidia GPU and CuPy.

In case of failure, it could create a report as [dedicated issues](https://github.com/scikit-learn/scikit-learn/issues?q=is%3Aissue+%22%E2%9A%A0%EF%B8%8F+ci+failed+on%22+) as we do for other scheduled jobs:

- https://github.com/scikit-learn/scikit-learn/blob/main/maint_tools/update_tracking_issue.py

Maybe @betatim has a plan and connections to do that? ;)

COMMENT:
Great news! Thanks very much for the progress report @thomasjpfan.

Labels: Build / CI, help wanted, Array API
Weekly CI run with NVidia GPU hardware

Now that #22554 was merged in `main`, it would be great to find a a way  to run a weekly scheduled job to run the scikit-learn `main` test on a CI worker with an NVidia GPU and CuPy.

In case of failure, it could create a report as [dedicated issues](https://github.com/scikit-learn/scikit-learn/issues?q=is%3Aissue+%22%E2%9A%A0%EF%B8%8F+ci+failed+on%22+) as we do for other scheduled jobs:

- https://github.com/scikit-learn/scikit-learn/blob/main/maint_tools/update_tracking_issue.py

Maybe @betatim has a plan and connections to do that? ;)

COMMENT:
Is there a way to help out/join in? I think having a CI sooner rather than later would be really useful.

I think cirun is our only option

Labels: Build / CI, help wanted, Array API
Weekly CI run with NVidia GPU hardware

Now that #22554 was merged in `main`, it would be great to find a a way  to run a weekly scheduled job to run the scikit-learn `main` test on a CI worker with an NVidia GPU and CuPy.

In case of failure, it could create a report as [dedicated issues](https://github.com/scikit-learn/scikit-learn/issues?q=is%3Aissue+%22%E2%9A%A0%EF%B8%8F+ci+failed+on%22+) as we do for other scheduled jobs:

- https://github.com/scikit-learn/scikit-learn/blob/main/maint_tools/update_tracking_issue.py

Maybe @betatim has a plan and connections to do that? ;)

COMMENT:
@thomasjpfan do you want someone else to try to takeover from your work in https://github.com/thomasjpfan/scikit-learn/tree/cirun_gpu if you don't have time to finalize this work yourself?

Labels: Build / CI, help wanted, Array API
Weekly CI run with NVidia GPU hardware

Now that #22554 was merged in `main`, it would be great to find a a way  to run a weekly scheduled job to run the scikit-learn `main` test on a CI worker with an NVidia GPU and CuPy.

In case of failure, it could create a report as [dedicated issues](https://github.com/scikit-learn/scikit-learn/issues?q=is%3Aissue+%22%E2%9A%A0%EF%B8%8F+ci+failed+on%22+) as we do for other scheduled jobs:

- https://github.com/scikit-learn/scikit-learn/blob/main/maint_tools/update_tracking_issue.py

Maybe @betatim has a plan and connections to do that? ;)

COMMENT:
The latest work is at https://github.com/thomasjpfan/scikit-learn/tree/cirun_gcp (GCP)

I'll take another took at it this weekend. If I do not have a PR by Monday, then let's schedule a call so I can do a handoff.

Labels: Build / CI, help wanted, Array API
Weekly CI run with NVidia GPU hardware

Now that #22554 was merged in `main`, it would be great to find a a way  to run a weekly scheduled job to run the scikit-learn `main` test on a CI worker with an NVidia GPU and CuPy.

In case of failure, it could create a report as [dedicated issues](https://github.com/scikit-learn/scikit-learn/issues?q=is%3Aissue+%22%E2%9A%A0%EF%B8%8F+ci+failed+on%22+) as we do for other scheduled jobs:

- https://github.com/scikit-learn/scikit-learn/blob/main/maint_tools/update_tracking_issue.py

Maybe @betatim has a plan and connections to do that? ;)

COMMENT:
After a few more iterations, it works now! https://github.com/thomasjpfan/scikit-learn/actions/runs/7820229387/job/21334434689

Next step is to to wrap it up into our workflow for triggering cirun.

Labels: Build / CI, help wanted, Array API
Weekly CI run with NVidia GPU hardware

Now that #22554 was merged in `main`, it would be great to find a a way  to run a weekly scheduled job to run the scikit-learn `main` test on a CI worker with an NVidia GPU and CuPy.

In case of failure, it could create a report as [dedicated issues](https://github.com/scikit-learn/scikit-learn/issues?q=is%3Aissue+%22%E2%9A%A0%EF%B8%8F+ci+failed+on%22+) as we do for other scheduled jobs:

- https://github.com/scikit-learn/scikit-learn/blob/main/maint_tools/update_tracking_issue.py

Maybe @betatim has a plan and connections to do that? ;)

COMMENT:
It also works with T4s: https://github.com/thomasjpfan/scikit-learn/actions/runs/7820372944/job/21334925253 it looks to spin up faster, because there are more T4s available.

Labels: Build / CI, help wanted, Array API
Weekly CI run with NVidia GPU hardware

Now that #22554 was merged in `main`, it would be great to find a a way  to run a weekly scheduled job to run the scikit-learn `main` test on a CI worker with an NVidia GPU and CuPy.

In case of failure, it could create a report as [dedicated issues](https://github.com/scikit-learn/scikit-learn/issues?q=is%3Aissue+%22%E2%9A%A0%EF%B8%8F+ci+failed+on%22+) as we do for other scheduled jobs:

- https://github.com/scikit-learn/scikit-learn/blob/main/maint_tools/update_tracking_issue.py

Maybe @betatim has a plan and connections to do that? ;)

COMMENT:
My understanding is that using custom Github runners exposes you to a lot of security issues, I am curious to know if there is any thoughts on how to mitigate this in scikit-learn, e.g. by only allowing a scheduled weekly run or some other way that make sure that running the GPU workflow is fully controlled by maintainers.

For exemple, according to [Hardening for self-hosted runners](https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions#hardening-for-self-hosted-runners) "self-hosted runners should almost never be used for public repositories".

Also using a custom github runner was a key aspect in the [PyTorch suppply-chain article](https://simonwillison.net/2024/Jan/14/supply-chain-attack-on-pytorch/)

I had a quick look at https://cirun.io and I could not find any mention about security there ...

Labels: Build / CI, help wanted, Array API
Weekly CI run with NVidia GPU hardware

Now that #22554 was merged in `main`, it would be great to find a a way  to run a weekly scheduled job to run the scikit-learn `main` test on a CI worker with an NVidia GPU and CuPy.

In case of failure, it could create a report as [dedicated issues](https://github.com/scikit-learn/scikit-learn/issues?q=is%3Aissue+%22%E2%9A%A0%EF%B8%8F+ci+failed+on%22+) as we do for other scheduled jobs:

- https://github.com/scikit-learn/scikit-learn/blob/main/maint_tools/update_tracking_issue.py

Maybe @betatim has a plan and connections to do that? ;)

COMMENT:
I'm +1 on at least having a weekly run.

The hard part comes from PRs. I proposed using a "run-gpu-ci" GitHub label to trigger the CI. Here are the details:

1. When a maintainer adds a "run-gpu-ci" label, it will trigger a wheel build on the PR itself.
2. The wheel is send over a "run on gpu" workflow using `workflow_dispatch`.
	- This workflow type runs in the context of the main branch, so the only adjustable input is the wheel itself.
	- With `workflow_dispatch` only the "cirun.io" configuration on the `main` branch is used. PRs can not configure the runner itself.
4. The "run gpu" removes the "run-gpu-ci" label and sends the wheel to `cirun` for testing. (`pytest --pyargs sklearn`)

With the above approach, it requires a maintainer to add a "run-gpu-ci" label each time they want to trigger a gpu ci run. In the end, it is using labels to implement "Require approval for all outside collaborators’" just for the GPU runner.

There are two security concerns:

1. We make sure that only the scikit-learn team can add the "run-gpu-ci" label.
2. Before adding the label, the reviewer needs to make sure the code does not do anything malicious. Specifically, one can still run arbitrary code in a test.

I say 2 is the bigger concern, because we can miss something. @aktech What are the security concerns with running arbitrary code using `cirun.io`?

Labels: Build / CI, help wanted, Array API
Weekly CI run with NVidia GPU hardware

Now that #22554 was merged in `main`, it would be great to find a a way  to run a weekly scheduled job to run the scikit-learn `main` test on a CI worker with an NVidia GPU and CuPy.

In case of failure, it could create a report as [dedicated issues](https://github.com/scikit-learn/scikit-learn/issues?q=is%3Aissue+%22%E2%9A%A0%EF%B8%8F+ci+failed+on%22+) as we do for other scheduled jobs:

- https://github.com/scikit-learn/scikit-learn/blob/main/maint_tools/update_tracking_issue.py

Maybe @betatim has a plan and connections to do that? ;)

COMMENT:
Thanks @thomasjpfan for the ping. I am more than happy to address any concerns and help implement CI for sckit-learn.

> @aktech What are the security concerns with running arbitrary code using cirun.io?

There aren't any security concerns that you wouldn't have on GitHub hosted runners. The runners created via cirun are ephemeral and are destroyed right after job completion. The most information one can know is the GCP project name, that's the basic metadata on the VM. The github token for runner provision is destroyed right after it's connected to GitHub and is ephemeral as well.

If you like you can also restrict runner creation by users/teams/permissions in the GitHub orgs via: https://docs.cirun.io/reference/access-control - conda-forge [uses this](https://github.com/conda-forge/.cirun) to restrict usage and access control CI.

> Something I don't quite grasp yet is why custom runners are much more dangerous/susceptible than the GH provided runners. If we can understand this difference we will probably get some good ideas for how to harden our custom runner.

@betatim checkout the second link [2]

As per GitHub docs

> This is not an issue with GitHub-hosted runners because each GitHub-hosted runner is always a clean isolated virtual machine, and it is destroyed at the end of the job execution.

Which is true for isolated VMs created by cirun as well.

Some ref:
- [1] https://docs.cirun.io/security
- [2] https://docs.github.com/en/actions/hosting-your-own-runners/managing-self-hosted-runners/about-self-hosted-runners#self-hosted-runner-security

Labels: Build / CI, help wanted, Array API