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

adding h5py recipe #685

Closed
wants to merge 22 commits into from
Closed

Conversation

shimwell
Copy link

Attempting to add the h5py package to emscripten-forge.

I notice hdf5 is already on emscripten-forge perhaps it is therefore also good to add h5py.

I've not successful made a package for emscripten-forge before so I looked at a few others on the recipes folder and this is my first attempt.

Linking to the request as request on the h5py repo
h5py/h5py#2338

@shimwell shimwell marked this pull request as ready for review October 29, 2023 22:22
@DerThorsten
Copy link
Contributor

Thanks for the recipe!

@shimwell
Copy link
Author

shimwell commented Oct 31, 2023

Many thanks @DerThorsten for the commits

looks like I am getting a error on the build

the recipe on this pr looks similar to the conda-forge recipe so I'm guess that mamba can't find the same range of packages in in emscripten-forge.

RuntimeError: Solver could not find solution.Mamba failed to solve:
 - python 3.11.* *_cpython
 - pip 22.0.4*
 - numpy 1.25.2*
 - hdf5 1.10.6*
 - cython >=0.29.31,<1
 - pkgconfig
 - emscripten-abi 3.1.45.*
 - hdf5 >=1.10.6,<1.10.7.0a0
 - numpy >=1.25.2,<2.0a0

with channels:

The reported errors are:
- Encountered problems while solving:
-   - nothing provides requested hdf5 1.10.6*
-   - nothing provides requested cython >=0.29.31,<1
-   - nothing provides pkg-config needed by pkgconfig-1.5.5-pyhd8ed1ab_4
-   - nothing provides requested hdf5 >=1.10.6,<1.10.7.0a0

@DerThorsten
Copy link
Contributor

Jeah just try without any pinning in the beginning!

@DerThorsten DerThorsten self-assigned this Oct 31, 2023
@DerThorsten DerThorsten added the enhancement New feature or request label Oct 31, 2023
@shimwell
Copy link
Author

interesting, from the CI error it looks like I'm missing channels for the install

with channels:

The reported errors are:
- Encountered problems while solving:
-   - nothing provides requested hdf5 1.10.6*
-   - nothing provides requested cython
-   - nothing provides pkg-config needed by pkgconfig-1.5.5-pyhd8ed1ab_4
-   - nothing provides requested hdf5 >=1.10.6,<1.10.7.0a0

@DerThorsten
Copy link
Contributor

interesting, from the CI error it looks like I'm missing channels for the install

with channels:

The reported errors are:
- Encountered problems while solving:
-   - nothing provides requested hdf5 1.10.6*
-   - nothing provides requested cython
-   - nothing provides pkg-config needed by pkgconfig-1.5.5-pyhd8ed1ab_4
-   - nothing provides requested hdf5 >=1.10.6,<1.10.7.0a0

I updated the pinning of hdf5 in the global build config, this should fix that issue

@DerThorsten
Copy link
Contributor

@shimwell we probably need some patches. Probably these here https://github.com/pyodide/pyodide/tree/main/packages/h5py/patches

@shimwell
Copy link
Author

shimwell commented Nov 6, 2023

Thanks for working on this @DerThorsten I have had a go at adding the patch. I've never actually made use of a git patch before so hope I've done it correctly. Thanks for teaching me

@shimwell
Copy link
Author

Would it be possible for someone with permissions to approve the ci tests

@shimwell
Copy link
Author

Looks like the cython failed in the CI

Error compiling Cython file:

@shimwell
Copy link
Author

shimwell commented Nov 15, 2023

The Cythonizing errors look a bit beyond me, is this the sort of thing you were thinking about @aragilar and @tacaswell when you predicted changes in the code base / fixes upstream on this issue

/home/runner/micromamba/envs/ci/conda-bld/h5py-0_1700087974754/work/h5py/defs.c:10431:85: warning: incompatible pointer types passing '__pyx_t_5numpy_uint32_t *' (aka 'unsigned long *') to parameter of type 'uint32_t *' (aka 'unsigned int *') [-Wincompatible-pointer-types]
   10431 |         __pyx_v_r = H5Dread_chunk(__pyx_v_dset_id, __pyx_v_dxpl_id, __pyx_v_offset, __pyx_v_filters, __pyx_v_buf);
         |                                                                                     ^~~~~~~~~~~~~~~
  /home/runner/micromamba/envs/ci/conda-bld/h5py-0_1700087974754/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_place/include/H5Dpublic.h:1011:92: note: passing argument to parameter 'filters' here
   1011 | H5_DLL herr_t H5Dread_chunk(hid_t dset_id, hid_t dxpl_id, const hsize_t *offset, uint32_t *filters,
        |                                                                                            ^
  /home/runner/micromamba/envs/ci/conda-bld/h5py-0_1700087974754/work/h5py/defs.c:26223:75: error: unknown type name 'H5FD_ros3_fapl_t'; did you mean 'H5FD_hdfs_fapl_t'?
   26223 | static herr_t __pyx_f_4h5py_4defs_H5Pget_fapl_ros3(hid_t __pyx_v_fapl_id, H5FD_ros3_fapl_t *__pyx_v_fa_out) {
         |                                                                           ^~~~~~~~~~~~~~~~
         |                                                                           H5FD_hdfs_fapl_t
  /home/runner/micromamba/envs/ci/conda-bld/h5py-0_1700087974754/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_place/include/H5FDhdfs.h:107:3: note: 'H5FD_hdfs_fapl_t' declared here
    107 | } H5FD_hdfs_fapl_t;
        |   ^
  /home/runner/micromamba/envs/ci/conda-bld/h5py-0_1700087974754/work/h5py/defs.c:26251:15: error: call to undeclared function 'H5Pget_fapl_ros3'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
   26251 |   __pyx_v_r = H5Pget_fapl_ros3(__pyx_v_fapl_id, __pyx_v_fa_out);
         |               ^
  /home/runner/micromamba/envs/ci/conda-bld/h5py-0_1700087974754/work/h5py/defs.c:26251:15: note: did you mean 'H5Pget_fapl_core'?
  /home/runner/micromamba/envs/ci/conda-bld/h5py-0_1700087974754/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_place/include/H5FDcore.h:92:15: note: 'H5Pget_fapl_core' declared here
     92 | H5_DLL herr_t H5Pget_fapl_core(hid_t fapl_id, size_t *increment /*out*/, hbool_t *backing_store /*out*/);
        |               ^
  /home/runner/micromamba/envs/ci/conda-bld/h5py-0_1700087974754/work/h5py/defs.c:26353:75: error: unknown type name 'H5FD_ros3_fapl_t'; did you mean 'H5FD_hdfs_fapl_t'?
   26353 | static herr_t __pyx_f_4h5py_4defs_H5Pset_fapl_ros3(hid_t __pyx_v_fapl_id, H5FD_ros3_fapl_t *__pyx_v_fa) {
         |                                                                           ^~~~~~~~~~~~~~~~
         |                                                                           H5FD_hdfs_fapl_t
  /home/runner/micromamba/envs/ci/conda-bld/h5py-0_1700087974754/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_place/include/H5FDhdfs.h:107:3: note: 'H5FD_hdfs_fapl_t' declared here
    107 | } H5FD_hdfs_fapl_t;
        |   ^
  /home/runner/micromamba/envs/ci/conda-bld/h5py-0_1700087974754/work/h5py/defs.c:26381:15: error: call to undeclared function 'H5Pset_fapl_ros3'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
   26381 |   __pyx_v_r = H5Pset_fapl_ros3(__pyx_v_fapl_id, __pyx_v_fa);
         |               ^
  /home/runner/micromamba/envs/ci/conda-bld/h5py-0_1700087974754/work/h5py/defs.c:26511:15: error: call to undeclared function 'H5Pget_fapl_direct'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
   26511 |   __pyx_v_r = H5Pget_fapl_direct(__pyx_v_fapl_id, __pyx_v_boundary, __pyx_v_block_size, __pyx_v_cbuf_size);
         |               ^
  /home/runner/micromamba/envs/ci/conda-bld/h5py-0_1700087974754/work/h5py/defs.c:26511:15: note: did you mean 'H5Pget_fapl_core'?
  /home/runner/micromamba/envs/ci/conda-bld/h5py-0_1700087974754/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_place/include/H5FDcore.h:92:15: note: 'H5Pget_fapl_core' declared here
     92 | H5_DLL herr_t H5Pget_fapl_core(hid_t fapl_id, size_t *increment /*out*/, hbool_t *backing_store /*out*/);
        |               ^
  /home/runner/micromamba/envs/ci/conda-bld/h5py-0_1700087974754/work/h5py/defs.c:26641:15: error: call to undeclared function 'H5Pset_fapl_direct'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
   26641 |   __pyx_v_r = H5Pset_fapl_direct(__pyx_v_fapl_id, __pyx_v_alignment, __pyx_v_block_size, __pyx_v_cbuf_size);
         |               ^
  /home/runner/micromamba/envs/ci/conda-bld/h5py-0_1700087974754/work/h5py/defs.c:27941:13: warning: assigning to 'void *' from 'const void *' discards qualifiers [-Wincompatible-pointer-types-discards-qualifiers]
   27941 |   __pyx_v_r = H5Pget_driver_info(__pyx_v_plist_id);
         |             ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  2 warnings and 6 errors generated.

@DerThorsten
Copy link
Contributor

fyi for complicated debugging, it is best to install the building machinery locally.
I recently wrote this section https://github.com/emscripten-forge/recipes#local-builds (hope I did not forget any step ;) )

@tacaswell
Copy link

You may need to pin cython to <3 for the build to work.

@shimwell
Copy link
Author

shimwell commented Nov 20, 2023

Adding the export H5PY_ROS3="0" to the build.sh helped with the errors. It reduce from 6 errors to 2 🎉

Current errors are now ...

  shared:INFO: (Emscripten: Running sanity checks)
  /home/runner/micromamba/envs/ci/conda-bld/h5py-0_1700504180033/work/h5py/defs.c:10425:85: warning: incompatible pointer types passing '__pyx_t_5numpy_uint32_t *' (aka 'unsigned long *') to parameter of type 'uint32_t *' (aka 'unsigned int *') [-Wincompatible-pointer-types]
   10425 |         __pyx_v_r = H5Dread_chunk(__pyx_v_dset_id, __pyx_v_dxpl_id, __pyx_v_offset, __pyx_v_filters, __pyx_v_buf);
         |                                                                                     ^~~~~~~~~~~~~~~
  /home/runner/micromamba/envs/ci/conda-bld/h5py-0_1700504180033/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_place/include/H5Dpublic.h:1011:92: note: passing argument to parameter 'filters' here
   1011 | H5_DLL herr_t H5Dread_chunk(hid_t dset_id, hid_t dxpl_id, const hsize_t *offset, uint32_t *filters,
        |                                                                                            ^
  /home/runner/micromamba/envs/ci/conda-bld/h5py-0_1700504180033/work/h5py/defs.c:26245:15: error: call to undeclared function 'H5Pget_fapl_direct'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
   26245 |   __pyx_v_r = H5Pget_fapl_direct(__pyx_v_fapl_id, __pyx_v_boundary, __pyx_v_block_size, __pyx_v_cbuf_size);
         |               ^
  /home/runner/micromamba/envs/ci/conda-bld/h5py-0_1700504180033/work/h5py/defs.c:26245:15: note: did you mean 'H5Pget_fapl_core'?
  /home/runner/micromamba/envs/ci/conda-bld/h5py-0_1700504180033/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_place/include/H5FDcore.h:92:15: note: 'H5Pget_fapl_core' declared here
     92 | H5_DLL herr_t H5Pget_fapl_core(hid_t fapl_id, size_t *increment /*out*/, hbool_t *backing_store /*out*/);
        |               ^
  /home/runner/micromamba/envs/ci/conda-bld/h5py-0_1700504180033/work/h5py/defs.c:26375:15: error: call to undeclared function 'H5Pset_fapl_direct'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
   26375 |   __pyx_v_r = H5Pset_fapl_direct(__pyx_v_fapl_id, __pyx_v_alignment, __pyx_v_block_size, __pyx_v_cbuf_size);
         |               ^
  /home/runner/micromamba/envs/ci/conda-bld/h5py-0_1700504180033/work/h5py/defs.c:27675:13: warning: assigning to 'void *' from 'const void *' discards qualifiers [-Wincompatible-pointer-types-discards-qualifiers]
   27675 |   __pyx_v_r = H5Pget_driver_info(__pyx_v_plist_id);
         |             ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  2 warnings and 2 errors generated.

class HDF5LibWrapper:

def __init__(self, libdirs):
- self._load_hdf5_lib(libdirs)

Choose a reason for hiding this comment

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

I have not chased through how, but I think that this patch is breaking all of h5py's auto-detection of what capabilities the wrapped libhdf5 has.

Co-authored-by: Thomas A Caswell <[email protected]>
@shimwell
Copy link
Author

Nice, that H5PY_DIRECT_VFD solved the 2 errors. There are still some warnings and something is breaking somewhere but it did help get to the next error

@bmaranville
Copy link

h5py is adding runtime_library_dirs to the build_ext command when os.name != 'nt', which not compatible with emscripten. I added another patch, and activated the patch in the recipe.yaml ... with the patch below, the h5py library builds, though there is an error during testing at the end due to deprecated usage of ruamel in boa (?)

diff --git a/recipes/recipes_emscripten/h5py/patches/configure.patch b/recipes/recipes_emscripten/h5py/patches/configure.patch
index 29bcada..382f800 100644
--- a/recipes/recipes_emscripten/h5py/patches/configure.patch
+++ b/recipes/recipes_emscripten/h5py/patches/configure.patch
@@ -60,6 +60,27 @@ index c16ddeef..1180d3fa 100644
  
      def has_direct_vfd_support(self):
          return self.has_functions("H5Pget_fapl_direct", "H5Pset_fapl_direct")
+diff --git a/setup_build.py b/setup_build.py
+index 966fdc97..ecdb6738 100644
+--- a/setup_build.py
++++ b/setup_build.py
+@@ -12,6 +12,7 @@ except ImportError:
+ from distutils.command.build_ext import build_ext
+ import sys
+ import os
++import platform
+ import os.path as op
+ from pathlib import Path
+ 
+@@ -112,7 +113,7 @@ class h5py_build_ext(build_ext):
+             settings['include_dirs'] += [mpi4py.get_include()]
+ 
+         # TODO: should this only be done on UNIX?
+-        if os.name != 'nt':
++        if platform.system() == 'Linux':
+             settings['runtime_library_dirs'] = settings['library_dirs']
+ 
+         def make_extension(module):
 -- 
 2.35.1
 
diff --git a/recipes/recipes_emscripten/h5py/recipe.yaml b/recipes/recipes_emscripten/h5py/recipe.yaml
index c31af93..cda0586 100644
--- a/recipes/recipes_emscripten/h5py/recipe.yaml
+++ b/recipes/recipes_emscripten/h5py/recipe.yaml
@@ -10,6 +10,8 @@ source:
   - url: https://github.com/h5py/h5py/archive/{{ version }}.tar.gz
     sha256: 9cac7838ab139ec71adf721a1305b954e18efa4f80c82fcfe34e1eb9639ded29
   - path: setup.py
+  - patches:
+    - patches/configure.patch
 
 build:
   number: 0
@@ -38,7 +40,7 @@ about:
   home: https://www.h5py.org/
   license: BSD 3-Clause
   license_family: BSD 3-Clause
-  license_file: LICENSE.txt
+  license_file: LICENSE
   summary: Read and write HDF5 files from Python
   description: |
     The h5py package is a Pythonic interface to the HDF5 binary data format

@bmaranville
Copy link

bmaranville commented Nov 27, 2023

The error message during testing:

AttributeError:
"safe_load()" has been removed, use

yaml = YAML(typ='safe', pure=True)
yaml.load(...)

instead of file "/home/bbm/miniforge3/envs/emscripten-forge/lib/python3.10/site-packages/boa/core/test.py", line 67

        d = ruamel.yaml.safe_load(fi)

@shimwell
Copy link
Author

I see the CI is making use of a boa fork which has the line you mentioned

https://github.com/DerThorsten/boa/blob/5c29ce83f5fa3cb4b032a97c08f5a5a9bb0957f1/boa/core/test.py#L55

The boa repo has a different section that uses the YAML code you mentioned

https://github.com/mamba-org/boa/blob/c3d50dccab317bcff55145114d8d5f87084305ff/boa/core/test.py#L67-L68

@bmaranville
Copy link

I can confirm that using a stock version of boa (installed v0.17 from mamba) that the build completes successfully, but no tests are run (I'm guessing that the patched version of boa does this correctly?)

Here is the h5py library created:
h5py-3.10.0-py311heb368a2_0.zip

@DerThorsten
Copy link
Contributor

I can confirm that using a stock version of boa (installed v0.17 from mamba) that the build completes successfully, but no tests are run (I'm guessing that the patched version of boa does this correctly?)

Here is the h5py library created: h5py-3.10.0-py311heb368a2_0.zip

jeah we use this custom version of boa only for the purpose of being able to run the tests (since they run in a headless browser, its all a bit complicated)

@shimwell
Copy link
Author

I just updated this branch and the CI failed with what I think is a new error. I can't see "safe_load" in the CI failing logs. I think this is the error now but I'm not totally sure.

Failed to build h5py
Exception information:
Traceback (most recent call last):
  File "$BUILD_PREFIX/venv/lib/python3.11/site-packages/pip/_internal/cli/base_command.py", line 167, in exc_logging_wrapper
    status = run_func(*args)
             ^^^^^^^^^^^^^^^
  File "$BUILD_PREFIX/venv/lib/python3.11/site-packages/pip/_internal/cli/req_command.py", line 205, in wrapper
    return func(self, options, args)
           ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "$BUILD_PREFIX/venv/lib/python3.11/site-packages/pip/_internal/commands/install.py", line 375, in run
    raise InstallationError(
pip._internal.exceptions.InstallationError: Could not build wheels for h5py, which is required to install pyproject.toml-based projects

@tacaswell
Copy link

I think that it is now failing on a link step:

  wasm-ld: error: unknown argument: -R/home/runner/micromamba/envs/ci/conda-bld/h5py-0_1705414808718/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_place/lib

@talmo talmo mentioned this pull request Sep 29, 2024
@shimwell
Copy link
Author

Closing in favour of #1354
Thanks for all the effort on this PR, some of it has been useful for the linked PR so not lost effort

@shimwell shimwell closed this Sep 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants