WASM Bindings collapse #87
Conversation
jerinphilip
added
mod: wasm
jerinphilip
force-pushed
the
jp/response-builder
branch
from
32285aa to
d0a907e
Compare
jerinphilip
force-pushed
the
jp/collapse-wasm-bindings
branch
from
9d80a08 to
fcc285e
Compare
|
Looks good to me, @abhi-agg would it break your workflow? |
|
@jerinphilip Since this PR affects the bindings, I would like to be a reviewer of this PR. Therefore, adding myself as a reviewer. I hope that is fine with you 👍 |
| } | ||
|
|
||
| auto results = model.translate(std::move(texts), translationRequest); | ||
| auto results = model.translateMultiple(std::move(texts), translationRequest); |
There was a problem hiding this comment.
Just saying. This change will require a change in the extension code otherwise extension code will not work out of box.
There was a problem hiding this comment.
Haven't I aliased translate -> Service::translateMultiple in bindings? I'm hoping this is the change you're talking about.
Build is failing in Extension's CircleCI (https://app.circleci.com/pipelines/github/mozilla-extensions/bergamot-browser-extension/204/workflows/ab129537-ad34-403b-b31d-40c626c19707/jobs/206/steps). But I'm guessing these are expected due to the issues discussed in mozilla-extensions/firefox-translations#83.
Can you confirm?
There was a problem hiding this comment.
Well manually downloaded XPI, seems working. Edit: I take that back, didn't overwrite old extension, oops. ^_^
There was a problem hiding this comment.
@jerinphilip We can test it with the wasm test page because that is using the latest byte based API. If it works with that then we are good to go 👍
There was a problem hiding this comment.
I have no clue what is happening, but I am assuming bindings transfer is intact, given I/O works?
There was a problem hiding this comment.
@jerinphilip We can test it with the wasm test page because that is using the latest byte based API. If it works with that then we are good to go 👍
@jerinphilip Let me help you with testing. I will test it at my end and confirm 👍
There was a problem hiding this comment.
@jerinphilip I tested your change with the fix in #125 and it works 👍
There was a problem hiding this comment.
I have pulled #125 into a test build, still translates to "schwanger". I guess the transfer is intact from your response and this is something to resolve at my end maybe.
The PRs which were a prerequisite for this are now complete (#79, #85, #80).
WASM bindings meanwhile have updated with passing memory than filenames (#117). The branch / PR has been brought forward to synchronize with these changes as well.
(A force push cleans the commit history, which was bloated before. Translation* files are deleted)
Checklist (ideally before merge):
Sync changes with Attempt to generate artifacts through CircleCI mozilla/bergamot-translator#39 to generate CircleCI artifact.Use artifact once again in Updating bergamot-translator to a collapsed version mozilla-extensions/firefox-translations#83 to ensure nothing breaks at extension.