✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

_{Generated for commit: a7bb1e6. Link to the linter CI: here}

This currently doesn't work and raises an obscure error (workflow logs) from within the Pyodide xbuildenv: ModuleNotFoundError: No module named 'js.process' for some reason with Pyodide xbuildenv versions 0.26.1/0.26.2/0.27.0a2 (tested in a PR on my fork). I am currently investigating this, and it should be resolvable, hopefully. cc: @hoodmane and @ryanking13

However, at the same time, creating a Pyodide virtual environment in a freshly spun-up Pyodide Docker container (and after removing a few conflicting, irrelevant conftest.py files temporarily):

.venv-pyodide/lib/python3.12/site-packages/sklearn/utils/tests/test_estimator_html_repr.py::test_get_visual_block_voting PASSED
.venv-pyodide/lib/python3.12/site-packages/sklearn/utils/tests/test_estimator_html_repr.py::test_get_visual_block_column_transformer PASSED
.venv-pyodide/lib/python3.12/site-packages/sklearn/utils/tests/test_estimator_html_repr.py::test_estimator_html_repr_pipeline PASSED
.venv-pyodide/lib/python3.12/site-packages/sklearn/utils/tests/test_estimator_html_repr.py::test_stacking_classifier[None] PASSED
.venv-pyodide/lib/python3.12/site-packages/sklearn/utils/tests/test_estimator_html_repr.py::test_stacking_classifier[final_estimator1] PASSED
.venv-pyodide/lib/python3.12/site-packages/sklearn/utils/tests/test_estimator_html_repr.py::test_stacking_regressor[None] PASSED
.venv-pyodide/lib/python3.12/site-packages/sklearn/utils/tests/test_estimator_html_repr.py::test_stacking_regressor[final_estimator1] PASSED
.venv-pyodide/lib/python3.12/site-packages/sklearn/utils/tests/test_estimator_html_repr.py::test_birch_duck_typing_meta PASSED
.venv-pyodide/lib/python3.12/site-packages/sklearn/utils/tests/test_estimator_html_repr.py::test_ovo_classifier_duck_typing_meta PASSED
.venv-pyodide/lib/python3.12/site-packages/sklearn/utils/tests/test_estimator_html_repr.py::test_duck_typing_nested_estimator PASSED
# ...
# [...] (truncated output)
# ...
('logisticregression',LogisticRegression())]),param_grid={'logisticregression__C':[0.1,1.0]})-check_supervised_y_2d] - DataConversionWarning not caught
XPASS .venv-pyodide/lib/python3.12/site-packages/sklearn/tests/test_common.py::test_search_cv[HalvingGridSearchCV(cv=2,error_score='raise',estimator=Pipeline(steps=[('pca',PCA()),('logisticregression',LogisticRegression())]),min_resources='smallest',param_grid={'logisticregression__C':[0.1,1.0]},random_state=0)-check_supervised_y_2d] - DataConversionWarning not caught
XPASS .venv-pyodide/lib/python3.12/site-packages/sklearn/tests/test_common.py::test_search_cv[RandomizedSearchCV(cv=2,error_score='raise',estimator=Pipeline(steps=[('pca',PCA()),('logisticregression',LogisticRegression())]),param_distributions={'logisticregression__C':[0.1,1.0]},random_state=0)-check_supervised_y_2d] - DataConversionWarning not caught
XPASS .venv-pyodide/lib/python3.12/site-packages/sklearn/tests/test_common.py::test_search_cv[HalvingRandomSearchCV(cv=2,error_score='raise',estimator=Pipeline(steps=[('pca',PCA()),('logisticregression',LogisticRegression())]),param_distributions={'logisticregression__C':[0.1,1.0]},random_state=0)-check_supervised_y_2d] - DataConversionWarning not caught
XPASS .venv-pyodide/lib/python3.12/site-packages/sklearn/utils/tests/test_testing.py::test_create_memmap_backed_data - memmap not fully supported
FAILED .venv-pyodide/lib/python3.12/site-packages/sklearn/tests/test_build.py::test_openmp_parallelism_enabled - AssertionError: 
==================================== 1 failed, 28699 passed, 4846 skipped, 761 xfailed, 52 xpassed, 3541 warnings in 787.87s (0:13:07) =====================================
(.venv-pyodide) agriyakhetarpal@c1f89ae6ff12:/src$ 

and all tests pass (the failing one can be ignored by enabling the SKLEARN_SKIP_OPENMP_TEST environment variable, of course). This suggests to me that there's a missing file in the xbuildenv that exists in the Pyodide repository/Docker container for it. The only related piece of code I found was in pyodide/pyodide#3189.

However, I don't see why import sklearn as a Pythonic statement would require this? I have recently implemented similar changes for statsmodels and scikit-image, and for other projects, too – and their test suites run without a hitch.

I figured out the error 😅

2024-09-05T18:42:07.5543835Z # possible namespace for /home/runner/work/scikit-learn/scikit-learn/doc/js
2024-09-05T18:42:07.5545980Z import 'js' # <_frozen_importlib_external.NamespaceLoader object at 0xfa1be8>
2024-09-05T18:42:07.5625899Z PythonError: # zipimport: zlib available

For circumstances unexplained to me, I found that it was trying to import a JavaScript file/module from doc/js/ as if it were a Python one, which I had switched to, to avoid running tests from the sklearn/ directory (where things wouldn't be compiled). I've switched to a different directory, i.e., maint_tools/, and import sklearn now works.

I think we should fix this in Pyodide upstream – import js in Python shouldn't try to import something in js/ (but I don't know if we can avoid this cleanly).

The build now fails with a conftest. py-related error, which shouldn't be too difficult; I'll take a look at it either later in the day or tomorrow.

So, we will have tempita from Cython when cross-compiling, of course, but having it inside the sklearn folder is a bit troublesome: Cython is only used at build-time with Meson, and not during runtime – the from Cython import Tempita as tempita statement in sklearn/_build_utils/tempita.py breaks the sklearn import in Pyodide (I'm unsure how this has been working in the Node.js-based wrapper script). I did install Cython temporarily with micropip because it has a pure Python wheel to fix the import, but the better way would be to move the tempita CLI elsewhere so that it is accessed somewhere outside the sklearn/ folder (such that it does not break the sklearn import, but can still be retrieved by Meson and be used as a generator).

Just four errors are left to fix:

../.venv-pyodide/lib/python3.12/site-packages/sklearn/datasets/tests/test_openml.py::test_fetch_openml_verify_checksum[True-liac-arff] ERROR
../.venv-pyodide/lib/python3.12/site-packages/sklearn/datasets/tests/test_openml.py::test_fetch_openml_verify_checksum[False-liac-arff] ERROR
../.venv-pyodide/lib/python3.12/site-packages/sklearn/datasets/tests/test_openml.py::test_fetch_openml_verify_checksum[True-pandas] ERROR
../.venv-pyodide/lib/python3.12/site-packages/sklearn/datasets/tests/test_openml.py::test_fetch_openml_verify_checksum[False-pandas] ERROR

which are most likely coming from the difference between the Node.js runner and the Pyodide virtual environment, because the latter uses the conftest.py file in the root directory, while the former did not. Once done, I can start migrating the workflow to Azure Pipelines and clean up the changes.

I have triggered both the Pyodide venv and the Azure tests, and test_fetch_openml_verify_checksum is skipped for the latter. We now collect a slightly bigger number of tests ("collected 36856 items / 2 skipped"), whereas the Azure job collects/collected fewer items ("collected 36288 items / 1 skipped"). I pushed 382c8ba to remove an unused argument that was being perceived as a fixture – if that doesn't work, we could potentially skip the test to keep the same behaviour.

I added the "upload to anaconda.org" logic, @agriyakhetarpal feel free to have a closer look. I tested it on my fork (build log) and it seems to work OK (except the actual upload because I didn't set up the secrets).

It looks good to me, thank you, Loïc!

@ogrisel do you have an opinion on whether we should pin actions by commit hash in the new Pyodide workflow, in particular pypa/cibuildwheel and scientific-python/upload-nightly-action?

I am not too sure what is the worst that could happen if a compromised Pyodide wheel was uploaded.

I guess if scientific-python/upload-nightly-action was compromised they could steal our anaconda.org secrets and upload vanilla Python (i.e. not Pyodide only) wheels to anaconda.org and attack users/projects that use our development wheel.

While I'm not on the scikit-learn board, the general advice I will have from my NumFOCUS Security Committee member hat is that all actions should be pinned, even if it makes updating them troublesome. Dependabot or Renovate usually help with it, and current limitations will be overcome when github/roadmap#592 lands. zizmor should also handle it, and will bicker about it if you use the --pedantic option.

I would agree with your assessment – this is another point of attack. If there's something that came out of the PyTorch nightly build dependency compromise, it is that there are definitely a bunch of end users of the nightly wheels, outside of CI.

While I'm not on the scikit-learn board, the general advice I will have from my NumFOCUS Security Committee member hat is that all actions should be pinned, even if it makes updating them troublesome

I remember this was discussed in the context of SPEC 8 for example #29203 (comment) and the picture was not so clear-cut at the time.

SPEC 8 is saying "Pin GitHub Actions release workflows to their full release commit SHAs" which is why we are pinning the pypa/gh-action-pypi-publish by commit hash.

Now the question is whether the publishing of the Pyodide dev wheel to anaconda.org can be considered a "release workflow", maybe it is to be honest and we should at least pin pypa/cibuildwheel and scientific-python/upload-nightly-action.

@ogrisel do you have an opinion on whether we should pin actions by commit hash in the new Pyodide workflow, in particular pypa/cibuildwheel and scientific-python/upload-nightly-action?

I think we said in the past that we should pin by commit hash for package generating workflows only.

EDIT: "release critical" -> "package generating" workflows.

Thanks, @ogrisel for the review – I've addressed your comments in 4831d6d

Edit: I won't be able to get a working Anaconda.org upload of the wheels, though; I'm contributing through a fork. It should kick in once the change is pushed to main.

Edit two: the workslow is passing here: https://github.com/scikit-learn/scikit-learn/actions/runs/14068121558

I have pushed the following changes:

• use major version pinning for Github actions
• revert cuda-ci cibuildwheel hash pin. I will open a separate PR about this. To be honest I am not too sure whether we need to pin it by commit hash. The created wheel is only used internally in the CI. You could as a user download download the github artifact and pip install the downloaded file but this is a bit far-fetched ...

Let's merge this one, thanks @agriyakhetarpal!

I have triggered manually a Pyodide wheel build, let's see what happens: https://github.com/scikit-learn/scikit-learn/actions/runs/14079229690

It failed because the token is empty? I'm not sure why that happened.

Yeah me neither, I need to look into it closer ...

Maybe #31078 would fix it by using the right Github environment to be able to access the secret.