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

feat(wasm): add support for wasm compilation without Javascript API #783

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

PedroMBB
Copy link

Building TFHE-rs on the wasm32-unknown-unknown target requires the __wasm_api feature 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 wasm is proposed, and it would only allow wasm32-unknown-unknown compilation. The __wasm_api feature would continue to exist, and so no breaking changes are introduced.

Copy link

cla-bot bot commented Jan 17, 2024

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:

  • For individual contribution: our CLA
  • for Bounty submission, if you are an individual: our T&C
  • for Bounty submission, if you are a company: our T&C
    to get yourself added.

If you already signed one of this document, just wait to be added to the bot config.

@IceTDrinker
Copy link
Member

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

@PedroMBB
Copy link
Author

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?

@IceTDrinker
Copy link
Member

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

Copy link

cla-bot bot commented Jan 19, 2024

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:

  • For individual contribution: our CLA
  • for Bounty submission, if you are an individual: our T&C
  • for Bounty submission, if you are a company: our T&C
    to get yourself added.

If you already signed one of this document, just wait to be added to the bot config.

@IceTDrinker
Copy link
Member

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

@aquint-zama
Copy link
Contributor

@cla-bot check

@cla-bot cla-bot bot added the cla-signed label Jan 22, 2024
Copy link

cla-bot bot commented Jan 22, 2024

The cla-bot has been summoned, and re-checked this pull request!

@PedroMBB PedroMBB changed the title feat(wasm): add support for wasm complication without Javascript API feat(wasm): add support for wasm compilation without Javascript API Jan 22, 2024
Copy link
Member

@IceTDrinker IceTDrinker left a comment

Choose a reason for hiding this comment

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

A few comments, mainly things which were not done "right" on our end, please rebase on top of main, we do not accept merge commits in the main branch history.

Thank you for you PR

tfhe/Cargo.toml Outdated
Comment on lines 103 to 105
wasm = [
"dep:getrandom",
]
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

This is required to have getrandom as a dependency for the wasm build

Copy link
Author

Choose a reason for hiding this comment

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

Yes, you are correct! It looks like during the rebase I lost that change, I will add it back in the squashed commit

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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 ?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

no we will add you as a co-author on the commit and we will tag you on the PR :)

Copy link
Member

Choose a reason for hiding this comment

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

I migt be able to update this branch, will let you know

tfhe/Cargo.toml Outdated Show resolved Hide resolved
tfhe/src/boolean/engine/bootstrapping.rs Outdated Show resolved Hide resolved
@IceTDrinker
Copy link
Member

@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.

@IceTDrinker
Copy link
Member

@slab-ci cpu_wasm_test

@IceTDrinker
Copy link
Member

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.

@PedroMBB
Copy link
Author

PedroMBB commented Jan 24, 2024

@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 wasm32-wasi or wasm32-unknown-emscripten targets to ensure broader compatibility.

@IceTDrinker
Copy link
Member

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 wasm32-wasi or wasm32-unknown-emscripten targets to ensure broader compatibility.

@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 ?

@PedroMBB
Copy link
Author

PedroMBB commented Jan 24, 2024

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 wasm32-wasi or wasm32-unknown-emscripten targets to ensure broader compatibility.

@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:

This crate fully supports the wasm32-wasi and wasm32-unknown-emscripten targets. However, the wasm32-unknown-unknown target (i.e. the target used by wasm-pack) is not automatically supported since, from the target name alone, we cannot deduce which JavaScript interface is in use (or if JavaScript is available at all).

As far as I know, the wasm32-wasi target is indeed supported by the Rust compiler. The decision to use this target in the tests should depend on whether supporting non-JS wasm targets aligns with the project's goals. For my use case, where the JS environment is present, each option works just fine.

@IceTDrinker
Copy link
Member

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 !

@IceTDrinker
Copy link
Member

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

@IceTDrinker
Copy link
Member

@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

@IceTDrinker
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants