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.

Sign up for GitHub

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

Jump to bottom

Improve script to patch wasm artifacts and load EN->DE vocabulary in wasm test #125

Merged
merged 2 commits into from
May 3, 2021
Merged

Improve script to patch wasm artifacts and load EN->DE vocabulary in wasm test #125

merged 2 commits into from
May 3, 2021

Conversation

abhi-agg
Copy link
Contributor

@abhi-agg abhi-agg commented May 3, 2021 •
edited
Loading

The PR:

  • Improves the script that patches wasm artifacts to enable simd wormhole
  • Fixes wasm test page for loading EN->DE vocabulary properly

@abhi-agg abhi-agg mentioned this pull request May 3, 2021
Merged
4 tasks
@jerinphilip
Copy link
Contributor

image

This branch gives me the above error.

@XapaJIaMnu
Copy link
Collaborator

Sorry, I'm not qualified to review the wasm bits. If other people seem OK with it and you need another approval I can give it to you

abhi-agg added 2 commits May 3, 2021 15:50
@abhi-agg
Improved script that patches wasm artifacts to enable wormhole
9660acc 
 - Made the regex pattern ignore multiple whitespaces b/w words of
   the matching pattern
@abhi-agg
Fix for loading EN->DE vocabularies in wasm test page
0e674a1 
 - Loading vocabularies for EN->DE was failing because of
   the new structure of bergamot-models
@abhi-agg abhi-agg requested a review from jerinphilip May 3, 2021 13:55
@abhi-agg
Copy link
Contributor Author

abhi-agg commented May 3, 2021

This branch gives me the above error.

@jerinphilip I am not sure what is wrong at your end. The latest status of this branch is passing for the wasm test page. See attached result:

Screenshot 2021-05-03 at 15 58 19

@abhi-agg
Copy link
Contributor Author

abhi-agg commented May 3, 2021 •
edited
Loading

Sorry, I'm not qualified to review the wasm bits. If other people seem OK with it and you need another approval I can give it to you

@XapaJIaMnu This PR doesn't introduce any change in wasm bits. You can approve this change as I have tested it (the current branch is in sync with the latest main branch). Without merging this change, the current main branch is not usable for wasm 👍

@XapaJIaMnu XapaJIaMnu merged commit 1a4add1 into browsermt:main May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@XapaJIaMnu XapaJIaMnu XapaJIaMnu approved these changes

@jerinphilip jerinphilip jerinphilip approved these changes

@motin motin Awaiting requested review from motin

@andrenatal andrenatal Awaiting requested review from andrenatal

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@abhi-agg @jerinphilip @XapaJIaMnu