search

browsermt / bergamot-translator

Cross platform C++ library focusing on optimized machine translation on the consumer-grade device.
http://browser.mt
Mozilla Public License 2.0
340 stars 38 forks source link

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

Closed abhi-agg closed 2 years ago

abhi-agg commented 2 years ago

Based on the discussions in https://github.com/mozilla/firefox-translations/issues/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 https://github.com/mozilla/firefox-translations/issues/51#issuecomment-1024989397) 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 commented 2 years ago

@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 commented 2 years ago

@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.

abhi-agg commented 2 years ago

@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 commented 2 years ago

Closing this one as filed https://github.com/browsermt/bergamot-translator/issues/363 for test cases.