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

Package STT wasm for npm publishing #2265

Merged
merged 3 commits into from
Jul 28, 2022
Merged

Conversation

ccoreilly
Copy link
Collaborator

@ccoreilly ccoreilly commented Jul 21, 2022

No description provided.

@CLAassistant
Copy link

CLAassistant commented Jul 21, 2022

CLA assistant check
All committers have signed the CLA.

native_client/BUILD Show resolved Hide resolved
@@ -0,0 +1,14 @@
{
"name" : "$(PROJECT_NAME)",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you test the packaged library to see if it's possible to use it from within an NPM-based project?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did test it with an example I would like to contribute eventually to stt-examples but I think it is a good idea to test it in the CI pipeline as well. I'll add a test step for it.

linkopts = select({
":wasm_es6": DECODER_LINKOPTS_WASM + [
"-sEXPORT_ES6=1",
"-sSINGLE_FILE=1",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would you kindly add a comment here explaining why, even though we have SINGLE_FILE here, it's producing multiple files? (because of the "pthread" story you mentioned, right?)

Copy link
Collaborator Author

@ccoreilly ccoreilly Jul 23, 2022

Choose a reason for hiding this comment

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

SINGLE_FILE exists since before pthread support and it currently only inlines the wasm bytecode as a base64 string inside the main JS. Pthreads are implemented as separate (smaller) workers which actually load the main JS from within. There are some requests to inline or merge both files when using SINGLE_FILE (see emscripten#9796 and emscripten#14329)

Generally, as noted in the references above, using SINGLE_FILE generates a large asset (larger than the sum of both stt_wasm.js and stt_wasm.wasm), which means longer download and startup times. Blinded by the original goal of making it available via CDN, I thought it would be necessary to inline the wasm binary for ease of use as a library, but I just tested it and webpack can handle the separate asset perfectly (rollup should also be fine; I am not sure about parcel, but I guess it'd also work.)

I will thus remove this flag. At this point we could consider dropping es5 support (ES6 is necessary for bundlers, see emscripten#14135) but ES6 imports are still not supported in workers under Firefox (reference) so I would still add it to the github releases.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Awesome, I agree with what you outlined. Thank you for the thorough explaination !

Copy link
Collaborator

@reuben reuben left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I have a couple of comments inline, and would like to get answers to the questions by @Dexterp37 as well.

.github/workflows/build-and-test.yml Outdated Show resolved Hide resolved
native_client/wasm/package.json.in Outdated Show resolved Hide resolved
native_client/wasm/Makefile Outdated Show resolved Hide resolved
@Dexterp37 Dexterp37 requested a review from reuben July 23, 2022 13:11
@reuben
Copy link
Collaborator

reuben commented Jul 23, 2022

$ ls -lh package/dist
total 320
-r-xr-xr-x@ 1 reuben  staff   155K Oct 26  1985 stt_wasm.js
-r-xr-xr-x@ 1 reuben  staff   2.9K Oct 26  1985 stt_wasm.worker.js
$ ls -lh libstt
total 6920
-rw-r--r--@ 1 reuben  staff    26K Jul 23 14:37 COPYING
-r-xr-xr-x@ 1 reuben  staff   155K Jul 23 14:50 stt_wasm.js
-r-xr-xr-x@ 1 reuben  staff   3.2M Jul 23 14:50 stt_wasm.wasm
-r-xr-xr-x@ 1 reuben  staff   3.0K Jul 23 14:50 stt_wasm.worker.js

Is the ES6 package supposed to be so much smaller than the other one?

@ccoreilly
Copy link
Collaborator Author

No, I forgot to push a change to the Makefile to include the wasm binary. I can do it later.

@ccoreilly ccoreilly marked this pull request as ready for review July 25, 2022 21:22
@ccoreilly
Copy link
Collaborator Author

I added a test step for the wasm npm module but struggled with testing the library itself (it currently only tests that it is installed). I am only familiar with jest, which runs tests inside node and thus ES6 modules need to be transpiled to CommonJS (I tried unsuccessfully to run the ES6 modules with the experimental --experimental-vm-modules flag).
Unfortunately, there must be a bug in babel because it adds a space between ! and -- in the following line of the stt_wasm.js:

L1582               if (! --numWorkersToLoad) removeRunDependency("wasm-instantiate");

which breaks the module and does not manage to load successfully. That is why I had to skip the test that actually tests that the library loads and can be used.

I also tried the jsdom environment in jest, which emulates a browser but inside stt_wasm.js the code checks if it is running in node or the browser and it concludes (correctly) that it is running inside node but the emulated environment does not have the expected variables such as __dirname so it crashes.

Maybe someone else can extend the tests in the future (maybe it should be tested with something such as cypress?). As I wrote, for now it only tests that the library is installed and can be imported.

@reuben
Copy link
Collaborator

reuben commented Jul 28, 2022

Opened a clone of this PR here to test my fixes.

@reuben reuben merged commit 025861b into coqui-ai:main Jul 28, 2022
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.

4 participants