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

Translations produced by test_page are (very, hilariously) wrong #206

Closed
lhk opened this issue Jul 15, 2021 · 10 comments · Fixed by #215
Closed

Translations produced by test_page are (very, hilariously) wrong #206

lhk opened this issue Jul 15, 2021 · 10 comments · Fixed by #215
Assignees

Comments

@lhk
Copy link

lhk commented Jul 15, 2021

I've followed your instructions to the letter, regarding the building of wasm, hosting the test page and running chrome with the correct command line arguments.

But when I open the test translation page and try it on some simple sentences, it outputs translations that are not even close to anything reasonable. In case you wonder, "schwanger" means "pregnant":

image

Similar problems appear for the other languages. At least "schwanger" is actually a German word. English to Spanish translates that phrase as "engineering engineering engineering engineering ...."

@XapaJIaMnu
Copy link
Collaborator

This is an issue with the GEMM, because either the WASM wormhole was not enabled (have you checked if it's enabled?) or if you are running apple M1 hardware, which is not supported.

@jerinphilip
Copy link
Contributor

For what it's worth, I had previously reported this on var, which is not M1 hardware. Haven't managed to fix this till now. I downloaded a nightly launched locally and spawned via ssh -X and set prefs accordingly. I remember double-checking the instructions. A reproducible example should be available on a verbose run of a Makefile here, hopefully (make server, after configuring a few parameter variable names).

#87 (comment)

@lhk
Copy link
Author

lhk commented Jul 17, 2021

@XapaJIaMnu how do I check whether its enabled?

I did run the patch script. And this is executed in chromium, with the command line arguments that are given in the readme.

@lhk
Copy link
Author

lhk commented Jul 17, 2021

Also, it's not on M1

@XapaJIaMnu
Copy link
Collaborator

Did you use the Chrome canary 90+ with the appropriate flag:

https://github.com/browsermt/bergamot-translator/tree/main/wasm#demo

--js-flags="--experimental-wasm-simd"

@abhi-agg
Copy link
Contributor

@lhk Currently, this feature doesn't work on Google Chrome browser (it used to). We are working on making it run there again.

In the meanwhile, you can try running it on Firefox Nightly where the translation speeds will be higher compared to running it on Chrome.

I will update the README to reflect the latest status 👍

@kpu
Copy link
Member

kpu commented Jul 19, 2021

Root cause is WebAssembly. Specifically it does not support 8-bit integer multiplication anywhere near efficiently WebAssembly/relaxed-simd#9 .
To make WebAssembly slightly less slow, we bypassed the standard to allow the Intel 8-bit instruction in Firefox https://bugzilla.mozilla.org/show_bug.cgi?id=1672160 . We're upgrading this to native 8-bit matrix multiply on Intel and ARM https://bugzilla.mozilla.org/show_bug.cgi?id=1715704 .

As mitigations, we should check that the 8-bit instruction is working first (by running it) and bail with an error when that happens, ideally one presented to the user. We could emulate with 16-bit integer instructions though this will make it even slower.

Full resolution entails not using WebAssembly at all for 12x speedup: https://translatelocally.com/

@lhk
Copy link
Author

lhk commented Jul 19, 2021

@XapaJIaMnu I'm running chromium 90+, over the command line and specifying that flag.

@abhi-agg can I run the code without this feature then? What if I don't apply the patch script?

@kpu interesting, thanks a lot for the details.

@abhi-agg
Copy link
Contributor

abhi-agg commented Jul 19, 2021

@abhi-agg can I run the code without this feature then? What if I don't apply the patch script?

@lhk Unfortunately not because this feature was statically included for a faster experimentation. However, as I mentioned, we are working on a solution (should be landed soon) that will allow it to perform inference on Chrome as well (without this feature).

@kpu
Copy link
Member

kpu commented Jul 28, 2021

If you want a working version but care even less about speed, compile all the WASM stuff with https://github.com/kpu/intgemm/blob/master/CMakeLists.txt#L82-L85 with -DWORMHOLE=OFF

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 a pull request may close this issue.

5 participants