feat(wasm): add support for wasm compilation without Javascript API #783
Conversation
|
Thank you for your pull request. We require contributors to sign our Contributor License Agreement / Terms and Conditions, and we don't seem to have the users @PedroMBB on file. In order for us to review and merge your code, please sign:
If you already signed one of this document, just wait to be added to the bot config. |
|
Hello @PedroMBB thanks for the patch, as you can see you will need to sign the Contributor License Agreement before we can proceed with the review process. See the automated bot answer here: #783 (comment) Cheers |
I already signed the Agreement. Do I need to do anything else for the bot to update the CLA status? |
I’ll check internally then, thanks for the info |
|
Thank you for your pull request. We require contributors to sign our Contributor License Agreement / Terms and Conditions, and we don't seem to have the users @PedroMBB on file. In order for us to review and merge your code, please sign:
If you already signed one of this document, just wait to be added to the bot config. |
|
As an FYI we use a rebase workflow, we don’t do merge commits. Normally as you signed the CLA we should be able to get on with the PR on Monday once business resumes. Likely won’t be merged for the 0.5 release |
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
PedroMBB
changed the title
tfhe/Cargo.toml
Outdated
| wasm = [ | ||
| "dep:getrandom", | ||
| ] |
There was a problem hiding this comment.
Use a traget_arch style dependency with target_arch = "wasm32"
There was a problem hiding this comment.
This is required to have getrandom as a dependency for the wasm build
There was a problem hiding this comment.
Yes, you are correct! It looks like during the rebase I lost that change, I will add it back in the squashed commit
There was a problem hiding this comment.
hey @PedroMBB looks like the pre commit check failed for the build
https://github.com/zama-ai/tfhe-rs/actions/runs/7627525779/job/20776459136?pr=783#step:4:349
For the Cargo.toml I think you need
# Remove this line
getrandom = { version = "0.2.8", optional = true }
bytemuck = "1.13.1"
# add
[target.'cfg(target_arch = "wasm32")'.dependencies.getrandom]
version = "0.2.8"and remove the dep:getrandom from the __wasm_api feature
if you want to make sure the check passes you can run make pcc locally before pushing
There was a problem hiding this comment.
ah wait that's something a bit different... hmm
is it ok if we use your current patch and give co-authorship and we can look at getting this in the code base ?
There was a problem hiding this comment.
otherwise I think we'll go for your first solution as it seems our CI tooling is not ready to handle the proper way of doing things (sorry) and I don't want to go for a lot of rounds getting the CI fixed, let me know if you just want to revert to the first version of this
There was a problem hiding this comment.
ah wait that's something a bit different... hmm
is it ok if we use your current patch and give co-authorship and we can look at getting this in the code base ?
Yes, it is okay for me. Do you need me to do anything for that to happen?
There was a problem hiding this comment.
no we will add you as a co-author on the commit and we will tag you on the PR :)
There was a problem hiding this comment.
I migt be able to update this branch, will let you know
|
@PedroMBB thanks for the changes, the commits are not compliant with the commit format it should be of the form feat(wasm): added support for wasm compilation without Javascript API for example. I think you are still missing the target based configuration in the toml otherwise you will be missing the source of randomness I believe ? I think it's fine to squash your commits into one and adding the cfg based dependency see #783 (comment) in the Cargo.toml otherwise it won't work. |
|
@slab-ci cpu_wasm_test |
|
hello @PedroMBB can you check if https://github.com/zama-ai/tfhe-rs/tree/am/fix/pedro-mbb-wasm is doing what you need ? you can build using make build_tfhe_full_wasm I feel like there is no way to check if it works properly and the rayon threading is likely going to be broken, so we may not add support for wasm to the computing primitives for now as it's a much bigger work than "just making it compile" and it's not on our roadmap at the moment. |
|
@IceTDrinker do I have push permissions for that branch? (If not I don't mind you changing it) I've noticed a potential bug in the Makefile at line 93. The current line is: .PHONY: install_check_toolchain_wasm32_target # Install the wasm32 target in addition to the check toolchain
install_rs_check_toolchain_wasm32_target: install_rs_check_toolchain
@( rustup target add --toolchain "$(RS_CHECK_TOOLCHAIN)" wasm32-unknown-unknown && \
( echo "Unable to add wasm32 target for $(RS_CHECK_TOOLCHAIN) toolchain, check your rustup installation. \
Rustup can be downloaded at https://rustup.rs/" && exit 1 )But I believe it should be modified to: .PHONY: install_check_toolchain_wasm32_target # Install the wasm32 target in addition to the check toolchain
install_rs_check_toolchain_wasm32_target: install_rs_check_toolchain
@rustup target add --toolchain "$(RS_CHECK_TOOLCHAIN)" wasm32-unknown-unknown || \
( echo "Unable to add wasm32 target for $(RS_CHECK_TOOLCHAIN) toolchain, check your rustup installation. \
Rustup can be downloaded at https://rustup.rs/" && exit 1 )This change involves removing the first "(" and replacing the "&&" on the third line with "||". Regarding the js feature being enabled by default for WebAssembly support, I have mixed feelings. The documentation at getrandom docs clearly states that the js feature should not be enabled by default in libraries. However, not enabling this feature will likely lead to issues raised about the error when compiling for WASM. On the other hand, enabling it by default complicates compilation against non-JS targets. One solution could be to run the tests using the |
@PedroMBB fixed the issue, had the patch locally forgot to push, as for the js feature, the getrandom crate had a static assertion and crashed the build if it was not enabled, are the target you indicate supported toolchains by the rust compiler ? |
Yes, the crash occurs because the crate cannot infer the appropriate interface to use based solely on the target. As stated in their documentation:
As far as I know, the |
|
Thanks for the info @PedroMBB wasm is not part of our current target for computation targets, if we were able to build and make it work without too much hassle and be sure it works ok we might go for it, I'll keep this PR open and let you know how things evolve, if you find ways to make the test suite run then that would settle the question I guess ! |
|
Ok I see the toolchains are tier 2.5 like the wasm32-unknown-unknown, will keep you updated on if we have resources for that |
|
@PedroMBB forgot to get back to you here, we don't really have the resources to support wasm, as you must have guessed, for now so we'll take contributions but they need to be tested Cheers |
|
coming back to this as we have now enabled some of our dependencies to run on wasm in a js (browser/node) environment I don't think we can make TFHE-rs strictly compatible with the wasm32 target given we rely on rayon for some of the parallel processing and it is not supported by wasm32 out of the box |
Building TFHE-rs on the
wasm32-unknown-unknowntarget requires the__wasm_apifeature to be enabled, enabling a Wasm JS binding API. However, such behavior is not required when using TFHE-rs directly from Web Assembly.In the changes suggested an intermediate feature
wasmis proposed, and it would only allowwasm32-unknown-unknowncompilation. The__wasm_apifeature would continue to exist, and so no breaking changes are introduced.