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

Allow per-input options #346

Merged
merged 8 commits into from
Feb 11, 2022
Merged

Conversation

jerinphilip
Copy link
Contributor

@jerinphilip jerinphilip commented Feb 10, 2022

Changes signature of BlockingService::{translate,pivot}Multiple
functions to take per input options, so a mix of HTML and plaintext
can be sent from the extension. Templating over testing is adjusted
to allow for continuous evaluations by modifying the test code.

Updates WebAssembly bindings to reflect the change in signature
and the javascript test-page to work with the new bindings.

This change lacks an accompanying test specific to the mixed HTML
and plaintext inputs.

Fixes: #345
See also: mozilla/firefox-translations#94

@jerinphilip
Copy link
Contributor Author

jerinphilip commented Feb 10, 2022

Check c5cea4b (#346) for a quick confirmation this feature works. In the interest of speeding up Mozilla, I'm proposing to merge this at the earliest.

For inputs mixed sent with corresponding HTML response-options:

std::vector<std::string> plainTexts = {
      "Hello World!",                                 //
      "The quick brown fox jumps over the lazy dog."  //
  };

  std::vector<std::string> htmlTexts = {
      "<a href=\"#\">Hello</a> world.",                                                   //
      "The quick brown <b id=\"fox\">fox</b> jumps over the lazy <i id=\"dog\">dog</i>."  //
  };

The library code works:

Hallo Welt!
Der schnelle braune Fuchs springt über den faulen Hund.
<a href="#">Hallo</a> Welt.
Der schnelle braune <b id="fox">Fuchs</b> springt über den faul<i id="dog">en Hund.</i>

The final tests will follow at a later stage when I can access var and can be designed better.

@jerinphilip jerinphilip marked this pull request as ready for review February 10, 2022 17:02
src/translator/service.h Outdated Show resolved Hide resolved
@abhi-agg
Copy link
Contributor

@jerinphilip Do you intend to make a similar change to Async service as well?

src/tests/wasm.cpp Outdated Show resolved Hide resolved
@jerinphilip
Copy link
Contributor Author

Do you intend to make a similar change to Async service as well?

Not sure how this is relevant to Mozilla efforts, but I will try to answer. AsyncService was meant to operate at one input which allows one option setting, not wait and go forward. Batching will happen by queueing and not blocking. Also figured it's easier to provide cancellation handles per request than for a batch of requests. Maybe we will change it later if we need the concept of a "BatchJob", but not right now. Where such a change relevant is perhaps python, but I don't see an immediate need to do it unless there's a client.

@jerinphilip
Copy link
Contributor Author

jerinphilip commented Feb 11, 2022

@jelmervdl points out (in internal messaging) without VectorResponseOptions at WASM bindings the code is broken for use in JS, consequently, JS is broken as well. But the required C++ API changes have been completed.

@abhi-agg Please note that I expect you to experiment with and bring changes to worker.js and bindings necessary as you would usually do in the verification step before communicating to extension dev.

@jerinphilip
Copy link
Contributor Author

@jelmervdl has been kind enough to edit the WASM/HTML bindings as well. Requesting @abhi-agg to review now.

@abhi-agg abhi-agg self-requested a review February 11, 2022 13:03
Copy link
Contributor

@abhi-agg abhi-agg left a comment

Choose a reason for hiding this comment

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

This change lacks an accompanying test specific to the mixed HTML
and plaintext inputs.

Approving this change per agreement that the corresponding test cases would be added in a separate PR soon.

@jerinphilip jerinphilip merged commit ec46919 into browsermt:main Feb 11, 2022
@jerinphilip jerinphilip deleted the per-input-opts branch February 11, 2022 13: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.

HTML/Non-HTML translation per item of Batch Translation API
3 participants