Skip to content

CI Fix pyodide wheel testing #31145

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 6 commits into from
Apr 4, 2025
Merged

CI Fix pyodide wheel testing #31145

merged 6 commits into from
Apr 4, 2025

Conversation

lesteve
Copy link
Member

@lesteve lesteve commented Apr 3, 2025 •
edited
Loading

Not sure why this started happening recently, see build log.

I get a similar error locally when trying to run from the scikit-learn root folder and cding out of it seems to fix it ...

+ python -m pytest -svra --pyargs sklearn --durations 20 --showlocals
  ImportError while loading conftest '/home/runner/work/scikit-learn/scikit-learn/sklearn/conftest.py'.
  sklearn/__init__.py:69: in <module>
      from . import (  # noqa: F401 E402
  sklearn/__check_build/__init__.py:54: in <module>
      raise_build_error(e)
  sklearn/__check_build/__init__.py:35: in raise_build_error
      raise ImportError(
  E   ImportError: No module named 'sklearn.__check_build._check_build'
  E   ___________________________________________________________________________
  E   Contents of /home/runner/work/scikit-learn/scikit-learn/sklearn/__check_build:
  E   __init__.py               _check_build.pyx          meson.build
  E   ___________________________________________________________________________
  E   It seems that scikit-learn has not been built correctly.
  E   
  E   If you have installed scikit-learn from source, please do not forget
  E   to build the package before using it. For detailed instructions, see:
  E   https://scikit-learn.org/dev/developers/advanced_installation.html#building-from-source
  E   
  E   If you have used an installer, please check that it is suited for your
  E   Python version, your operating system and your platform.
  Traceback (most recent call last):
    File "/home/runner/work/_temp/cibw/bin/cibuildwheel", line 8, in <module>
      sys.exit(main())
               ^^^^^^
    File "/home/runner/work/_temp/cibw/lib/python3.12/site-packages/cibuildwheel/__main__.py", line 62, in main
      main_inner(global_options)
    File "/home/runner/work/_temp/cibw/lib/python3.12/site-packages/cibuildwheel/__main__.py", line 203, in main_inner
      build_in_directory(args)
    File "/home/runner/work/_temp/cibw/lib/python3.12/site-packages/cibuildwheel/__main__.py", line 382, in build_in_directory
      platform_module.build(options, tmp_path)
    File "/home/runner/work/_temp/cibw/lib/python3.12/site-packages/cibuildwheel/platforms/pyodide.py", line 431, in build
      shell(test_command_prepared, cwd=test_cwd, env=virtualenv_env)
    File "/home/runner/work/_temp/cibw/lib/python3.12/site-packages/cibuildwheel/util/cmd.py", line 83, in shell
      subprocess.run(command, env=env, cwd=cwd, shell=True, check=True)
    File "/opt/hostedtoolcache/Python/3.12.9/x64/lib/python3.12/subprocess.py", line 573, in run
      raise CalledProcessError(retcode, process.args,
  subprocess.CalledProcessError: Command 'python -m pytest -svra --pyargs sklearn --durations 20 --showlocals' returned non-zero exit status 4.

@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 3, 2025 •
edited
Loading

✔️ Linting Passed

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

Generated for commit: ac25f13. Link to the linter CI: here

@lesteve lesteve added the Quick Review For PRs that are quick to review label Apr 4, 2025
@lesteve
Copy link
Member Author

lesteve commented Apr 4, 2025

Pyodide build passed see build log.

@lesteve
Copy link
Member Author

lesteve commented Apr 4, 2025

cc @agriyakhetarpal just because the need to cd might happen in other places with Pyodide and cibuildwheel.

I think it was needed at one point in #29791 (comment) but switching to cibuildwheel removed this need and now we need to do it again 🤷

@agriyakhetarpal
Copy link
Contributor

Thanks for letting me know of this, @lesteve! I think not having to switch directories for testing was one of the standout features of cibuildwheel; would we be wrong if we were to call it a regression?

@lesteve
Copy link
Member Author

lesteve commented Apr 4, 2025 •
edited
Loading

would we be wrong if we were to call it a regression?

So I am not sure why this started happening to be honest, likely the dependabot PR https://github.com/scikit-learn/scikit-learn/pull/31125/files#diff-78a7bc1bdd4aae56dc69fba13e39ebb7e6e794ba5e7398020c0696f69f6758f8 a few days ago. Looks like the commit hash changed for the same cibuildwheel version 😱, I don't understand why to be honest.

@lesteve
Copy link
Member Author

lesteve commented Apr 4, 2025 •
edited
Loading

In my last commit I fix the wrong commit hash for cibuildwheel 2.23.2 and it seems to work fine.

That may mean that there is a regression in cibuildwheel main not entirely sure to be honest.

Not too sure why dependabot opened a PR with the wrong commit hash, maybe there was a tag mishap on the cibuildwheel side for a small time window and we were unlucky enough that the dependabot PR was opened during this small time window.

@lesteve
Copy link
Member Author

lesteve commented Apr 4, 2025

The Pyodide build seems fine in my last commit see build log

thomasjpfan
thomasjpfan approved these changes Apr 4, 2025
View reviewed changes
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dependabot not using the correct hash is concerning.

@ogrisel ogrisel mentioned this pull request Apr 4, 2025
Open
@ogrisel
Copy link
Member

ogrisel commented Apr 4, 2025

I agree that the buggy dependabot update is concerning. I opened pypa/cibuildwheel#2348 to investigate what could have caused this. If the tag wasn't pushed several times we should open a follow-up issue on the dependabot repo itself.

@ogrisel ogrisel merged commit ff82bda into scikit-learn:main Apr 4, 2025
36 checks passed
@henryiii
Copy link

henryiii commented Apr 4, 2025

I get a similar error locally when trying to run from the scikit-learn root folder and cding out of it seems to fix it

Thanks for letting me know of this, @lesteve! I think not having to switch directories for testing was one of the standout features of cibuildwheel; would we be wrong if we were to call it a regression?

This is a commit from the cibuildwheel 3.0 development (dependabot bug? It was week(s) after the release, and in a different branch), which changes the way it runs tests. You really should use a src folder (notice it is broken locally too!), but if you don't, you can use the new test-sources option to ensure you don't import files that should not be importable. The cd technique would not support iOS/Android, and had issues even with regular packages, and complicated running tests. You could set PYTHONSAFEPATH for Python 3.11+ (I wonder if we should do that?)

(I think this was expected, though I wasn't following this change closely.)

@lesteve lesteve deleted the fix-pyodide branch April 7, 2025 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ogrisel ogrisel ogrisel approved these changes

@thomasjpfan thomasjpfan thomasjpfan approved these changes

Assignees
No one assigned
Labels
Build / CI Quick Review For PRs that are quick to review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lesteve @agriyakhetarpal @ogrisel @henryiii @thomasjpfan