Skip to content
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.

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

docs: move to newest pyodide toolchain #2062

Merged
merged 22 commits into from
Jun 22, 2023
Merged

Conversation

agoose77
Copy link
Collaborator

@agoose77 agoose77 commented Jan 4, 2023

Our existing CI requires several pieces of information to remain in sync in order to setup WASM building. With the new pyodide release, we can compute this (e.g. EMSDK version) at build time. This PR creates an additional requirements-wasm.txt that contains only the pyodide-build dependency meaning that all dependencies are contained in requirements files.

This PR will fail whilst pyodide/pyodide#3415 is being fixed, modulo other bugs in the latest release.

@codecov
Copy link

codecov bot commented Jan 4, 2023

Codecov Report

Merging #2062 (3c6ee22) into main (2e4d3d9) will not change coverage.
The diff coverage is n/a.

❗ Current head 3c6ee22 differs from pull request most recent head 282920d. Consider uploading reports for the commit 282920d to get more accurate results

Additional details and impacted files

@agoose77
Copy link
Collaborator Author

agoose77 commented Jan 5, 2023

@henryiii just assigning you to this so I don't lose track.

@agoose77 agoose77 force-pushed the agoose77/docs-emsdk-version branch from e773070 to cc0e24f Compare January 29, 2023 01:07
@agoose77 agoose77 force-pushed the agoose77/docs-emsdk-version branch from cc0e24f to e3ebb79 Compare February 13, 2023 10:16
@agoose77
Copy link
Collaborator Author

@henryiii it looks like the pybind11 discovery is not succeeding in the venv that pyodide-build now creates. I'm not initially sure why this isn't working; if I add a sleep into the configure stage, I can inspect the venv and confirm that the $CMAKE_PREFIX_PATH/pybind11/share/cmake/pybind11/... CMake configuration files exist. Any ideas of where to look next?

@henryiii
Copy link
Member

OK to use the git version post scikit-build/scikit-build-core#205 or should I push a patch release?

@agoose77
Copy link
Collaborator Author

For debugging sure! We would probably prefer a patch once it comes to releasing, but this isn't time critical.

@henryiii
Copy link
Member

Yes, it would be nice to debug before release, so the release can contain fixes if needed. :)

@agoose77 agoose77 force-pushed the agoose77/docs-emsdk-version branch from 9fcae19 to be94ea8 Compare February 26, 2023 14:49
@agoose77
Copy link
Collaborator Author

With logging.level = "INFO", I see:

* Creating virtualenv isolated environment...
* Installing packages in isolated environment... (pybind11, scikit-build-core[pyproject] @ git+https://github.com/scikit-build/scikit-build-core.git)
* Installing packages in isolated environment... (cython, pythran, setuptools<65.6.0)
* Getting dependencies for wheel...
* Installing packages in isolated environment... (ninja>=1.5, pathspec, pyproject_metadata)
* Installing packages in isolated environment... (cython, pythran, setuptools<65.6.0)
* Building wheel...
2023-02-26 15:09:22,349 - scikit_build_core - INFO - CMake version: 3.25.0
*** scikit-build-core 0.2.1.dev5+g084f457 using CMake 3.25.0
2023-02-26 15:09:22,352 - scikit_build_core - INFO - Build directory: /home/angus/Git/awkward/awkward-cpp/build/cpython-310
*** Configurating CMake...
configure: cmake -DCMAKE_C_COMPILER=/tmp/tmpa69l03b_/cc -DCMAKE_CXX_COMPILER=/tmp/tmpa69l03b_/c++ -DCMAKE_AR=/tmp/tmpa69l03b_/ar -DCMAKE_C_COMPILER_AR=/tmp/tmpa69l03b_/ar -DCMAKE_CXX_COMPILER_AR=/tmp/tmpa69l03b_/ar --fresh -S. -Bbuild/cpython-310 -Cbuild/cpython-310/CMakeInit.txt -DCMAKE_BUILD_TYPE=Release -DCMAKE_MODULE_PATH:PATH=/tmp/build-env-e_umlp2f/lib/python3.10/site-packages/scikit_build_core/resources/find_python -DCMAKE_PREFIX_PATH:PATH=/tmp/build-env-e_umlp2f/lib/python3.10/site-packages "-DEMSCRIPTEN=1" -DCMAKE_CROSSCOMPILING_EMULATOR=/home/angus/Git/emsdk/node/14.18.2_64bit/bin/node;--experimental-wasm-bulk-memory;--experimental-wasm-threads
CMake Warning:
  Ignoring extra path from command line:

   ""-DEMSCRIPTEN=1""


loading initial cache file build/cpython-310/CMakeInit.txt
-- CMake version 3.25.0
-- CMAKE_BUILD_TYPE = Release
-- Using Emscripten
CMake Error at CMakeLists.txt:109 (find_package):
  Could not find a package configuration file provided by "pybind11" with any
  of the following names:

    pybind11Config.cmake
    pybind11-config.cmake

  Add the installation prefix of "pybind11" to CMAKE_PREFIX_PATH or set
  "pybind11_DIR" to a directory containing one of the above files.  If
  "pybind11" provides a separate development package or SDK, be sure it has
  been installed.


-- Configuring incomplete, errors occurred!
See also "/home/angus/Git/awkward/awkward-cpp/build/cpython-310/CMakeFiles/CMakeOutput.log".
emcmake: error: 'cmake -DCMAKE_C_COMPILER=/tmp/tmpa69l03b_/cc -DCMAKE_CXX_COMPILER=/tmp/tmpa69l03b_/c++ -DCMAKE_AR=/tmp/tmpa69l03b_/ar -DCMAKE_C_COMPILER_AR=/tmp/tmpa69l03b_/ar -DCMAKE_CXX_COMPILER_AR=/tmp/tmpa69l03b_/ar --fresh -S. -Bbuild/cpython-310 -Cbuild/cpython-310/CMakeInit.txt -DCMAKE_BUILD_TYPE=Release -DCMAKE_MODULE_PATH:PATH=/tmp/build-env-e_umlp2f/lib/python3.10/site-packages/scikit_build_core/resources/find_python -DCMAKE_PREFIX_PATH:PATH=/tmp/build-env-e_umlp2f/lib/python3.10/site-packages "-DEMSCRIPTEN=1" -DCMAKE_CROSSCOMPILING_EMULATOR=/home/angus/Git/emsdk/node/14.18.2_64bit/bin/node;--experimental-wasm-bulk-memory;--experimental-wasm-threads' failed (returned 1)

The contents of /tmp/build-env-e_umlp2f/lib/python3.10/site-packages is:

__pycache__
_cffi_backend.cpython-310-x86_64-linux-gnu.so
_distutils_hack
_sysconfigdata__emscripten_wasm32-emscripten.py
_virtualenv.pth
_virtualenv.py
beniget
beniget-0.4.1.dist-info
cffi
cffi-1.15.1.dist-info
Cython
Cython-0.29.33.dist-info
cython.py
distutils-precedence.pth
exceptiongroup
exceptiongroup-1.1.0.dist-info
gast
gast-0.5.3.dist-info
ninja
ninja-1.11.1.dist-info
numpy
numpy-1.24.1.dist-info
numpy.libs
omp
packaging
packaging-23.0.dist-info
pathspec
pathspec-0.11.0.dist-info
pip
pip-22.3.1.dist-info
pip-22.3.1.virtualenv
pkg_resources
ply
ply-3.11.dist-info
pybind11
pybind11-2.10.3.dist-info
pycparser
pycparser-2.21.dist-info
pyproject_metadata
pyproject_metadata-0.7.1.dist-info
pythran
pythran-0.12.1.dist-info
pyximport
scikit_build_core
scikit_build_core-0.2.1.dev5+g084f457.dist-info
scipy
scipy-1.9.3.dist-info
scipy.libs
setuptools
setuptools-65.5.1.dist-info
tomli
tomli-2.0.1.dist-info

which includes pybind11, and the contents of /tmp/build-env-e_umlp2f/lib/python3.10/site-packages/pybind11 are

├── __init__.py
├── __main__.py
├── __pycache__
│  ├── __init__.cpython-310.pyc
│  ├── __main__.cpython-310.pyc
│  ├── _version.cpython-310.pyc
│  ├── commands.cpython-310.pyc
│  └── setup_helpers.cpython-310.pyc
├── _version.py
├── commands.py
├── include
│  └── pybind11
│     ├── attr.h
│     ├── buffer_info.h
│     ├── cast.h
│     ├── chrono.h
│     ├── common.h
│     ├── complex.h
│     ├── detail
│     │  ├── class.h
│     │  ├── common.h
│     │  ├── descr.h
│     │  ├── init.h
│     │  ├── internals.h
│     │  ├── type_caster_base.h
│     │  └── typeid.h
│     ├── eigen
│     │  ├── matrix.h
│     │  └── tensor.h
│     ├── eigen.h
│     ├── embed.h
│     ├── eval.h
│     ├── functional.h
│     ├── gil.h
│     ├── iostream.h
│     ├── numpy.h
│     ├── operators.h
│     ├── options.h
│     ├── pybind11.h
│     ├── pytypes.h
│     ├── stl
│     │  └── filesystem.h
│     ├── stl.h
│     └── stl_bind.h
├── py.typed
├── setup_helpers.py
└── share
   ├── cmake
   │  └── pybind11
   │     ├── FindPythonLibsNew.cmake
   │     ├── pybind11Common.cmake
   │     ├── pybind11Config.cmake
   │     ├── pybind11ConfigVersion.cmake
   │     ├── pybind11NewTools.cmake
   │     ├── pybind11Targets.cmake
   │     └── pybind11Tools.cmake
   └── pkgconfig
      └── pybind11.pc

So, it looks like the pybind11 files are in the places you described. I checked whether moving the .cmake files into share/cmake worked over share/cmake/pybind11, but alas no luck.

@agoose77
Copy link
Collaborator Author

agoose77 commented Mar 7, 2023

@henryiii just a ping to see if you've had any more thoughts or tips on this! Let me know if you don't have availability, and I can try and push forward.

@agoose77
Copy link
Collaborator Author

agoose77 commented Mar 9, 2023

@henryiii here's the full output from a local build:
output.txt

I can confirm that the site-packages directory does contain pybind11:

$ ls /tmp/build-env-098k4w59/lib/python3.10/site-packages/pybind11 --tree 
/tmp/build-env-098k4w59/lib/python3.10/site-packages/pybind11
├── __init__.py
├── __main__.py
├── __pycache__
│  ├── __init__.cpython-310.pyc
│  ├── __main__.cpython-310.pyc
│  ├── _version.cpython-310.pyc
│  ├── commands.cpython-310.pyc
│  └── setup_helpers.cpython-310.pyc
├── _version.py
├── commands.py
├── include
│  └── pybind11
│     ├── attr.h
│     ├── buffer_info.h
│     ├── cast.h
│     ├── chrono.h
│     ├── common.h
│     ├── complex.h
│     ├── detail
│     │  ├── class.h
│     │  ├── common.h
│     │  ├── descr.h
│     │  ├── init.h
│     │  ├── internals.h
│     │  ├── type_caster_base.h
│     │  └── typeid.h
│     ├── eigen
│     │  ├── matrix.h
│     │  └── tensor.h
│     ├── eigen.h
│     ├── embed.h
│     ├── eval.h
│     ├── functional.h
│     ├── gil.h
│     ├── iostream.h
│     ├── numpy.h
│     ├── operators.h
│     ├── options.h
│     ├── pybind11.h
│     ├── pytypes.h
│     ├── stl
│     │  └── filesystem.h
│     ├── stl.h
│     └── stl_bind.h
├── py.typed
├── setup_helpers.py
└── share
   ├── cmake
   │  └── pybind11
   │     ├── FindPythonLibsNew.cmake
   │     ├── pybind11Common.cmake
   │     ├── pybind11Config.cmake
   │     ├── pybind11ConfigVersion.cmake
   │     ├── pybind11NewTools.cmake
   │     ├── pybind11Targets.cmake
   │     └── pybind11Tools.cmake
   └── pkgconfig
      └── pybind11.pc

@henryiii
Copy link
Member

henryiii commented Mar 9, 2023

Yes, I haven't figured out why it is having a problem yet. There's a toolchain file that sets CMAKE_SYSTEM_PREFIX_PATH, but that doesn't override CMAKE_PREFIX_PATH.

@agoose77
Copy link
Collaborator Author

Just saw this: pyodide/pyodide#3569

Not sure if it's related!

docs/requirements.txt Outdated Show resolved Hide resolved
docs/requirements.txt Outdated Show resolved Hide resolved
docs/requirements.txt Outdated Show resolved Hide resolved
@agoose77 agoose77 temporarily deployed to docs-preview April 26, 2023 08:27 — with GitHub Actions Inactive
@agoose77 agoose77 temporarily deployed to docs-preview April 26, 2023 18:35 — with GitHub Actions Inactive
@agoose77
Copy link
Collaborator Author

This PR now builds, and pulls in the correct tooling. The built C++ wheel doesn't seem to load properly, though; the pybind11 module doesn't import.

@henryiii
Copy link
Member

Can you try the native tooling? I think the problem is in pybind11 trying to "fix" the SO extension incorrectly. (Last two commits here: pybind/scikit_build_example#84)

@henryiii
Copy link
Member

henryiii commented May 5, 2023

Broken due to emscripten-core/emscripten#19301. Hoping this can be patched somehow in pyodide.

@agoose77 agoose77 added the pr-on-hold This PR is inactive due to a pending decision or other constraint label Jun 16, 2023
agoose77 and others added 2 commits June 22, 2023 16:17
Co-authored-by: Henry Schreiner <henry.fredrick.schreiner@cern.ch>
docs/requirements.txt Outdated Show resolved Hide resolved
Co-authored-by: Henry Schreiner <henry.fredrick.schreiner@cern.ch>
@agoose77 agoose77 temporarily deployed to docs-preview June 22, 2023 15:36 — with GitHub Actions Inactive
@agoose77 agoose77 marked this pull request as ready for review June 22, 2023 15:39
@agoose77 agoose77 temporarily deployed to docs-preview June 22, 2023 16:30 — with GitHub Actions Inactive
@agoose77 agoose77 merged commit 02fb9e4 into main Jun 22, 2023
@agoose77 agoose77 deleted the agoose77/docs-emsdk-version branch June 22, 2023 17:24
@agoose77 agoose77 removed the pr-on-hold This PR is inactive due to a pending decision or other constraint label Jun 22, 2023
agoose77 added a commit that referenced this pull request Jun 22, 2023
Co-authored-by: Henry Schreiner <henry.fredrick.schreiner@cern.ch>
agoose77 added a commit that referenced this pull request Jun 22, 2023
Co-authored-by: Henry Schreiner <henry.fredrick.schreiner@cern.ch>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants