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

HTML/Non-HTML translation per item of Batch Translation API #345

Closed
abhi-agg opened this issue Feb 10, 2022 · 4 comments · Fixed by #346
Closed

HTML/Non-HTML translation per item of Batch Translation API #345

abhi-agg opened this issue Feb 10, 2022 · 4 comments · Fixed by #346

Comments

@abhi-agg
Copy link
Contributor

abhi-agg commented Feb 10, 2022

Based on the discussions in mozilla/firefox-translations#51, the translation API will require some changes to be able to specify whether each entry in the batch of text to be translated is HTML or not. This could mean (as @jelmervdl also pointed in mozilla/firefox-translations#51 (comment)) accepting a ResponseOptions object per entry of the input batch.

Opening this issue so that it can be discussed here and appropriate changes can be done. Taking BlockingService::translateMultiple API as example, the new API could look like:

std::vector<Response> translateMultiple(std::shared_ptr<TranslationModel> translationModel,
                                          std::vector<std::string> &&source, const std::vector<ResponseOptions> &responseOptions);

Please note that this means all ResponseOptions could be different for each entry of source and therefore, each Response will have to be constructed accordingly.

cc @jerinphilip @kpu @andrenatal

@jerinphilip
Copy link
Contributor

@abhi-agg This is a welcome change. Adding that we will also need to adapt BlockingService::pivotMultiple towards the same goals.

My suggestion is to do this in two phases (a) Get the API change in - there will be some adjustments required for the bridge at tests for both service apps initially (b) Work out a cleaner place to place tests for the longer term (looking at https://github.com/browsermt/bergamot-translator/blob/34786520cd90a4fd3cb006a60648d57d4a5ed56c/src/tests/wasm.cpp).

This would mean you get (a) immediately to experiment with at the extension, while the stability things come later. Let me know if this is acceptable, in which case I can bring the required changes shortly.

@abhi-agg
Copy link
Contributor Author

abhi-agg commented Feb 10, 2022

@jerinphilip I agree with your proposal. Let's go ahead with it 👍🏾

Btw, do you have a timeline on when the change in the API could be available so that we can start experiment in the extension? I am asking this so that we can plan better for this feature at our end.

jerinphilip added a commit that referenced this issue Feb 11, 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
Co-authored-by: Jelmer van der Linde <jelmer@ikhoefgeen.nl>
@abhi-agg
Copy link
Contributor Author

@jerinphilip I am re-opening this one just to make sure that we don't forget to add the test cases as well.

@abhi-agg
Copy link
Contributor Author

Closing this one as filed #363 for test cases.

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.

2 participants