Change TranslationRequest into something like TranslationModelConfig

[jerinphilip]

Re-file in bergamot-translator and tag me there if you need feedback on if some particular API design/binding approach is possible/performant in JS.

@motin Would you mind changing the TranslationRequest object into something similar to a JSON object which has key value pairs and you pass it in as a string, as you now do with TranslationModelConfig? I can provide you more configurability (for starters, an alignment-threshold, how to combine text for getTranslatedText , maybe different type of quality scores to experiment on extension-side, and many more as development progresses). Key thing I put forward is this will be a dict of sorts and extensible with key values easily. The current TranslationRequest doesn't allow me this flexibility. I see it's rather unused below, at least for now.
https://github.com/mozilla-extensions/bergamot-browser-extension/blob/21f43e6739faff4b2a600dd9aea4ac48321bda58/src/core/static/wasm/bergamot-translator-worker.appendix.js#L101-L118

The above will map one to one with a YAML (and therefore JSON) capable structure which is marian::Options similar to what's done over here. You can pass in the std::string like for TranslationModelConfig and gain a boost with higher configurability on your side.

/cc @kpu

[motin]

Adding a yaml/json-formatted string as a parameter to TranslationRequest for specifying options specific to the request is definitely possible/performant in JS.

Note that while I am not married to the current design of the API/bindings and have no issues with updating the integration code to match whatever is decided in this repo, neither am I actively involved in the design decisions of the API at the moment, so I'll let y'all come up with the changes you need to achieve progress on that front, if that is ok with you?

[motin]

@abhi-agg ping for comments

[abhi-agg]

@jerinphilip If you are proposing to remove TranslationRequest class altogether and pass the translation request parameters as a YAML/JSON formatted std::string then I am fine with it.

I have just one comment. Since, model configuration is already being provided as a yaml-formatted string, it would be consistent if we treat the translation request also as a yaml-formatted string (instead of json formatted string). Is it fine with you?

We will have to add a documentation listing all the keys and the corresponding values that can be provided as translation request. Could you document them here? The PR review will be straightforward then 👍

[jerinphilip]

All I have asked is this structure have certain properties similar to a dict. YAML or JSON is just a conversion thing (also aren't they same except how they're printed?), and the redundancies can be removed/adjusted during implementation. A string representation of such a dict is parsable into the dict, which suffices for me.

Could you document them here?

Probably not, this issue is solved at typedef std::string TranslationRequest;, let's have it at another issue.

[abhi-agg]

YAML or JSON is just a conversion thing (also aren't they same except how they're printed?)

They are not same. YAML is a superset of JSON => A YAML parser can parse both JSON and a YAML formatted string.

Could you document them here?

Probably not, this issue is solved at typedef std::string TranslationRequest;,

Probably, I am not getting what you meant here. Let me ask you this way. What should we provide as std::string for translation request?

let's have it at another issue.

Wait. The title says "Change TranslationRequest into something like TranslationModelConfig". Is this issue not the right place for this discussion?

[jerinphilip]

@abhi-agg Scope is bigger, moving to #89. Resolution, as I understand here, is you're okay dropping the existing TranslationRequest class (effectively replacing it with YAML string). I'll leave the issue open until the implementation is complete.