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

[EXP] Make wasm features more JS-like #1625

Closed
wants to merge 3 commits into from
Closed

Conversation

luizirber
Copy link
Member

Fixes #1622

The current sourmash NPM package is a direct upload from the results of wasm-pack and is not very ergonomic for direct usage in JS. The arrow-wasm project does something else, opting for wrapping the wasm-pack output into a more palatable JS package. So I'm trying the same approach here =]

This means removing the src/core/src/wasm.rs file and moving most of the wasm parts into src/wasm, a new Rust crate with the code to make a nice JS package.

ping @gustavo-salazar, once I have something more complete can we review together how the JS API looks like (and how to test in a way that doesn't break in the future for you)?

TODO

  • bump version of the Rust crate
  • add a CI check to guarantee that the Rust/wasm/JS versions are synced
  • src/wasm/lib.rs has some commented code because we can't do a impl for types defined outside the crate (since now they tread sourmash as a third-party dep. Need to either create a newtype and dispatch calls to the sourmash types, or something like that.

@codecov
Copy link

codecov bot commented Jun 22, 2021

Codecov Report

Merging #1625 (66b00a7) into latest (a5a52b1) will decrease coverage by 0.05%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           latest    #1625      +/-   ##
==========================================
- Coverage   81.91%   81.85%   -0.06%     
==========================================
  Files         112      112              
  Lines       11438    11446       +8     
  Branches     1429     1429              
==========================================
  Hits         9369     9369              
- Misses       1814     1822       +8     
  Partials      255      255              
Flag Coverage Δ
python 89.33% <ø> (ø)
rust 66.28% <0.00%> (-0.15%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/core/src/cmd.rs 86.88% <ø> (ø)
src/core/src/encodings.rs 77.77% <ø> (ø)
src/core/src/lib.rs 100.00% <ø> (ø)
src/core/src/signature.rs 58.18% <ø> (ø)
src/core/src/sketch/minhash.rs 86.81% <ø> (ø)
src/wasm/src/lib.rs 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a5a52b1...66b00a7. Read the comment docs.

@luizirber luizirber mentioned this pull request Jun 23, 2021
@luizirber luizirber force-pushed the explicit_wasm_pkg branch 2 times, most recently from 9d3ff8e to 31f5ad9 Compare June 25, 2021 15:22
@luizirber luizirber force-pushed the explicit_wasm_pkg branch from 31f5ad9 to 66b00a7 Compare June 25, 2021 15:33
@luizirber
Copy link
Member Author

@gustavo-salazar: I started generating packages for the new sourmash version, they are contained in the Artifacts section for each CI run. (annoyingly, there is a .tgz file which is the NPM package inside the artifact.zip file...)

I tried to use it with https://github.com/EBI-Metagenomics/mgnify-sourmash-component but had issues because webpack is not copying the wasm file to the proper place. The package is also missing the .d.ts file, working on fixing it. Can you take a look if you have some time? =]

@luizirber luizirber changed the title [WIP] Make wasm features more JS-like [EXP] Make wasm features more JS-like Jun 27, 2021
@luizirber
Copy link
Member Author

luizirber commented Jun 27, 2021

Reconsidering this approach (hence the new [EXP] tag).

Current solution (building the NPM package directly from core):

Benefits:

  • Earlier detection of things that might not work in wasm (because it will fail compilation)
  • Access to pub(crate) methods, can depend on methods we might not want to expose in the public API (this can be a drawback too)

Drawbacks:

  • Need to annotate which methods to export (with #[wasm_bindgen]), which polutes a lot the core crate.
  • Limit the types of parameters in functions to values that can be passed from Wasm to JS (needing to annotate even more types around the crate)
    • I ended up created some "shims" around the real Rust method because of that...

Solution in this PR

Benefits:

  • Cleaner API for JS users: we can pull more types from web_sys without conflicting with the core methods (they still act as shims, tho)
  • Clean up the core crate: a lot of annotations (the #[wasm_bindgen] stuff) can be removed, since they are declared in the wasm crate now.

Drawbacks:

  • Using the arrow-wasm approach led to issues with properly using rollup to package everything (like missing the .d.ts files)
  • It is a separate crate, so depends more on CI to make sure everything is tested/synchronized with original core code
  • It redeclares types, much like what happens in Python (the MinHash class in Python, for example)
  • Implementation can only use public methods in core (this can also be a benefit)

Proposed plan

Short term, fix the current package in #1643 and release a new package to NPM.

In the medium/long term, do the approach in this PR (separate wasm crate) but without using rollup for packaging (so, removing parts of the arrow-wasm approach). Slowly expand the JS API as needed for MGNify sourmash component. This includes using needletail for FASTA/Q parsing (take a File in the JS API and do everything in Rust, instead of parsing sequences in JS like it is now) and niffler for working with compressed files.

@gustavo-salazar
Copy link

Hey @luizirber
I did a quick test, taking the artifact and using npm link, I also copied the types files in the folder just to avoid that sort of issues.
It seems to be working OK, I'm able to generate the sketches.

I had to add url as dependency and use the ProvidePlugin to make it available via webpack. After that it worked

@luizirber
Copy link
Member Author

Closing in favor of #1643

@luizirber luizirber closed this Jul 8, 2021
@luizirber luizirber deleted the explicit_wasm_pkg branch September 23, 2021 00:03
@luizirber luizirber restored the explicit_wasm_pkg branch September 23, 2021 00:05
@luizirber luizirber deleted the explicit_wasm_pkg branch September 23, 2021 00:06
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.

sourmash_bg missing in npm package
2 participants