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

WASM build #262

Draft
wants to merge 21 commits into
base: main
Choose a base branch
from
Draft

WASM build #262

wants to merge 21 commits into from

Conversation

agriyakhetarpal
Copy link

@agriyakhetarpal agriyakhetarpal commented Feb 27, 2025

Closes #234

  • Build
    • libgmp
    • libmpfr
    • flint
    • python-flint
  • Test

@agriyakhetarpal
Copy link
Author

I've been testing it a bit through a PR in my fork, and it all builds well, up to one point in the Cython sources where it fails in step 107/114: https://github.com/agriyakhetarpal/python-flint/actions/runs/13576857716/job/37954995494?pr=1

with the following error trace:

FAILED: src/flint/types/_gr.cpython-312-wasm32-emscripten.so.p/meson-generated_src_flint_types__gr.pyx.c.o 
/tmp/tmp8t79rr74/cc -Isrc/flint/types/_gr.cpython-312-wasm32-emscripten.so.p -Isrc/flint/types -I../src/flint/types -I/opt/hostedtoolcache/Python/3.12.9/x64/include/python3.12 -I/home/runner/work/python-flint/python-flint/wasm-library-dir/include -fvisibility=hidden -fdiagnostics-color=always -DNDEBUG -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -O3 -fPIC -MD -MQ src/flint/types/_gr.cpython-312-wasm32-emscripten.so.p/meson-generated_src_flint_types__gr.pyx.c.o -MF src/flint/types/_gr.cpython-312-wasm32-emscripten.so.p/meson-generated_src_flint_types__gr.pyx.c.o.d -o src/flint/types/_gr.cpython-312-wasm32-emscripten.so.p/meson-generated_src_flint_types__gr.pyx.c.o -c src/flint/types/_gr.cpython-312-wasm32-emscripten.so.p/src/flint/types/_gr.pyx.c
src/flint/types/_gr.cpython-312-wasm32-emscripten.so.p/src/flint/types/_gr.pyx.c:19051:65: error: incompatible integer to pointer conversion passing 'mp_limb_t' (aka 'unsigned long') to parameter of type 'const fmpz *' (aka 'const long *') [-Wint-conversion]
 19051 |   gr_ctx_init_fq_nmod(__pyx_v_ctx->__pyx_base.__pyx_base.ctx_t, __pyx_v_p, __pyx_v_d, __pyx_v_name);
       |                                                                 ^~~~~~~~~
/home/runner/work/python-flint/python-flint/wasm-library-dir/include/flint/gr.h:1366:53: note: passing argument to parameter 'p' here
 1366 | void gr_ctx_init_fq_nmod(gr_ctx_t ctx, const fmpz_t p, slong d, const char * var);
      |                                                     ^
src/flint/types/_gr.cpython-312-wasm32-emscripten.so.p/src/flint/types/_gr.pyx.c:19150:65: error: incompatible integer to pointer conversion passing 'mp_limb_t' (aka 'unsigned long') to parameter of type 'const fmpz *' (aka 'const long *') [-Wint-conversion]
 19150 |   gr_ctx_init_fq_zech(__pyx_v_ctx->__pyx_base.__pyx_base.ctx_t, __pyx_v_p, __pyx_v_d, __pyx_v_name);
       |                                                                 ^~~~~~~~~
/home/runner/work/python-flint/python-flint/wasm-library-dir/include/flint/gr.h:1367:53: note: passing argument to parameter 'p' here
 1367 | void gr_ctx_init_fq_zech(gr_ctx_t ctx, const fmpz_t p, slong d, const char * var);
      |                                                     ^
2 errors generated.

@agriyakhetarpal
Copy link
Author

FWIW; I hardly have any Cython development experience, so I will have to wrap my head around this a bit, unless someone has an idea for a fix. The basis for the failure here is that WASM is stricter about type conversions than compilation for other platforms, which is why this mismatch is causing errors.

@agriyakhetarpal
Copy link
Author

Okay – the cause of the error, more specifically, is that gr_fq_nmod_ctx and gr_fq_zech_ctx pass a ulong parameter p

@cython.no_gc
cdef class gr_fq_zech_ctx(gr_scalar_ctx):
cdef ulong p
cdef slong d
@staticmethod
cdef inline gr_fq_zech_ctx _new(ulong p, slong d, char* name):
cdef gr_fq_zech_ctx ctx
ctx = gr_fq_zech_ctx.__new__(gr_fq_zech_ctx)
ctx.p = p
ctx.d = d
gr_ctx_init_fq_zech(ctx.ctx_t, p, d, name)
ctx._init = True
return ctx

but flint/gr.h has fmpz_t in version 3.0.1: https://github.com/flintlib/flint/blob/c05007be863b1aae7221f11a9c135d673d968638/src/gr.h#L1366-L1367

and this has apparently been fixed in version v3.1.0. I'll try updating flint to v3.1.0 (before proceeding to try out v3.1.2, if I do)

@agriyakhetarpal
Copy link
Author

Successful build: https://github.com/agriyakhetarpal/python-flint/actions/runs/13577635441?pr=1

The next task is to run tests, which I can take a look at later in the day.

@oscarbenjamin
Copy link
Collaborator

I haven't looked through this yet but thanks!

@oscarbenjamin
Copy link
Collaborator

but flint/gr.h has fmpz_t in version 3.0.1:

Best to keep with latest FLINT version for the pyodide build. There is no need to support a range of versions in pyodide. Generally python-flint tracks the latest version but has some #ifdef type stuff for older versions so that someone could build against them. No one is going to build against old versions in WASM though.

@agriyakhetarpal
Copy link
Author

Sounds good to me! I think the tests will have a bunch of function signature mismatches, though 😬 based on what I noticed through my fork's run. Also, building itself takes ~20 minutes in total – we could get to half of that if we have prebuilt WASM binaries. I guess they could be hosted somewhere in a GitHub release when things are ready.

@agriyakhetarpal
Copy link
Author

This can take a bit of time. I'm disabling tests and running it in on my fork to see how widespread the signature mismatches are, and I managed to get test_all.py to run till the end – now we have some more in test_docstrings.py. My next tasks will be to debug these mismatches, which I haven't had fun doing, as, i. compilation of these libraries to WASM is messy on a macOS and ii. a test here contains several assert statements, instead of several tests containing a few of them. Please bear with me as I proceed with this slowly and steadily. We could also wholly ignore test_docstrings.py if you're not too keen on that being tested. (I think it's good to test, though).

@oscarbenjamin
Copy link
Collaborator

I don't quite understand what the problem with the tests is. Is it just caused by the Cython/C signatures not matching?

There is now FLINT 3.2.0-rc1. We could perhaps update all the Cython declarations to that version and use that for the WASM build. I'm not sure when 3.2.0 final will be released but we would want to update to that straight away when it is.

@agriyakhetarpal
Copy link
Author

agriyakhetarpal commented Mar 3, 2025

I don't quite understand what the problem with the tests is. Is it just caused by the Cython/C signatures not matching?

Yes, majorly. If there's even a slight mismatch between the upstream signatures for Flint and the ones we have here, WASM's type safety guarantees that it will terminate the program, which makes Pyodide raise a fatal error. These can happen at the time of linking (like when it happened with version 3.0.1 above), or at runtime, like we are facing now. I have to drop these completely instead of xfailing them as the running Pyodide interpreter is no longer possible to be used after it has encountered fatal errors.

There are so many of these with SciPy, especially from its wrapping of Fortran libraries that we can't compile to WASM directly without having to f2c them over. I hope the new Flang from LLVM-19 can do this better: pyodide/pyodide#5268

There is now FLINT 3.2.0-rc1. We could perhaps update all the Cython declarations to that version and use that for the WASM build. I'm not sure when 3.2.0 final will be released but we would want to update to that straight away when it is.

Yes, this sounds good to me, and I hope they've fixed a few of these upstream. I could start another PR for this when I have enough time to do so; I assume it will help me learn Cython a bit. :)

@oscarbenjamin
Copy link
Collaborator

I've opened gh-264 to bump the FLINT version to 3.2.0-rc1 and update all of the Cython declarations.

The Cython declarations are set from the FLINT docs by running the bin/all_rst_to_pxd.py script. There can often be mismatches resulting from parsing the signatures from the docs rather than the actual C code e.g. the docs may be inaccurate but usually in the Cython -> C -> compiled extension modules the differences even out largely because a lot of FLINT's functions are not actually called directly by python-flint.

@oscarbenjamin
Copy link
Collaborator

I've opened gh-264 to bump the FLINT version to 3.2.0-rc1 and update all of the Cython declarations.

I've just merged this to main. If you rebase/merge with main then this PR will get the updated declarations.

There might still be some wrong declarations that we can fix manually for now but then upstream to FLINT afterwards.

@agriyakhetarpal
Copy link
Author

Thank you! Let's see how this goes; I've improved the caching for the CI job, though I've kept the failing tests skipped. I have been facing trouble building GMP locally with my installed Emscripten toolchain, let alone the rest of the libraries, so CI is my testbed for now.

@oscarbenjamin
Copy link
Collaborator

It is expected that the ARM Linux jobs will fail after gh-264.

I think that the "Test coverage meson build" job is failing here because of this line that should be deleted:

print(srcpath)

Somehow the changes here to test_all.py are causing that print to happen at a different time.

@agriyakhetarpal
Copy link
Author

The WASM job says

configure: error: Cannot determine label suffix

coming from

emconfigure: error: './configure --disable-dependency-tracking --disable-shared --prefix=/home/runner/work/python-flint/python-flint/wasm-library-dir --with-gmp=/home/runner/work/python-flint/python-flint/wasm-library-dir --with-mpfr=/home/runner/work/python-flint/python-flint/wasm-library-dir' failed (returned 1)

Comment on lines +105 to +112
cd flint-3.2.0-rc1

emconfigure ./configure \
--disable-dependency-tracking \
--disable-shared \
--prefix=${{ env.WASM_LIBRARY_DIR }} \
--with-gmp=${{ env.WASM_LIBRARY_DIR }} \
--with-mpfr=${{ env.WASM_LIBRARY_DIR }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you need to set something like --disable-assembly or --host=... here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know what the host triple is for WASM

@agriyakhetarpal
Copy link
Author

Thanks, the host triple should be wasm32-unknown-emscripten. Let me try that, or I can switch to a CMake-based build instead if that doesn't work.

Co-Authored-By: Oscar Benjamin <[email protected]>
@oscarbenjamin
Copy link
Collaborator

The CMake build is not recommended except for Windows. I don't know if it would work.

@oscarbenjamin
Copy link
Collaborator

The WASM job says

configure: error: Cannot determine label suffix

The label suffix is used by the macro preprocessor that runs on the assembly code. I don't think that there is any assembly code for WASM. This is why I suggested --disable-assembly.

@agriyakhetarpal
Copy link
Author

Ah, wait, missed that comment

@agriyakhetarpal
Copy link
Author

I think it wouldn't help, as we have a warning as follows:

configure: WARNING: Currently no assembly available for 32-bit systems. Disabling assembly...

so --disable-assembly might be redundant. I can manually inspect the diff between the Makefiles for both versions if that brings something, or downgrade to version 3.1.3. Building version 3.2.0rc1 would be ideal, though.

@agriyakhetarpal
Copy link
Author

The most rudimentary reason for this is because the compiler is misconfigured – or it's probably coming from flintlib/flint@76c90d5.

@oscarbenjamin
Copy link
Collaborator

@albinahlback do you know what options need to be passed to configure to build for emscripten/WASM?

Currently configure fails with:

checking if system can use FLINT's fft_small module... no
configure: WARNING: Currently no assembly available for 32-bit systems. Disabling assembly...
checking how to switch to text section... .text
configure: error: Cannot determine label suffix
emconfigure: error: './configure --disable-dependency-tracking --disable-shared --prefix=/home/runner/work/python-flint/python-flint/wasm-library-dir --with-gmp=/home/runner/work/python-flint/python-flint/wasm-library-dir --with-mpfr=/home/runner/work/python-flint/python-flint/wasm-library-dir' failed (returned 1)
checking for assembler label suffix... 

This was working before with FLINT 3.1 but now fails with 3.2.0-rc1.

As I understand it determining the label suffix is not needed when building for WASM since assembly will be disabled anyway. Maybe that has changed since I last looked at it though...

@albinahlback
Copy link

This was working before with FLINT 3.1 but now fails with 3.2.0-rc1.

As I understand it determining the label suffix is not needed when building for WASM since assembly will be disabled anyway. Maybe that has changed since I last looked at it though...

It is needed for MPN_IORD_U.

@albinahlback
Copy link

Disable assembly does not mean disabling assembly completely, it just means disable compiling assembly files. Perhaps this could be made more clear, but that's the meaning behind it.

@oscarbenjamin
Copy link
Collaborator

It is needed for MPN_IORD_U.

From what I see it looks like this is only defined for __amd64__ or __aarch64__ so for WASM this MACRO doesn't exist.

What should we do in the WASM case?

@albinahlback
Copy link

I'll fix this before releasing 3.2.0. Please open up an issue at FLINT.

@oscarbenjamin
Copy link
Collaborator

I've opened flintlib/flint#2247 for this.

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.

Nightly wheels and WASM/pyodide builds
3 participants