Skip to content
This repository has been archived by the owner on Sep 4, 2023. It is now read-only.

Integrate Basic QE feature #144

Merged
merged 11 commits into from
Mar 11, 2022
Merged

Integrate Basic QE feature #144

merged 11 commits into from
Mar 11, 2022

Conversation

abhi-agg
Copy link
Collaborator

@abhi-agg abhi-agg commented Mar 7, 2022

Fixes:
#133
#132
#96

@abhi-agg abhi-agg marked this pull request as draft March 7, 2022 12:35
 - Modified worker to construct TranslationModel with
   the appropriate model config for Quality Estimate feature
   when it is enabled by user

 - Supported QE feature only for in-page translation and not
   for Outbound translation
 - For in-page translation, the flag is passed at it is
 - For outbound translation, we don't need QE. So passing it as false
   and restoring it after translate call is done.
 - An api change has made it necessary to specify the size
@abhi-agg abhi-agg marked this pull request as ready for review March 11, 2022 17:30
@abhi-agg
Copy link
Collaborator Author

abhi-agg commented Mar 11, 2022

Fixes first 3 parts of #26. The last part is left (i.e. using CSS to show colors for bad scores) which will be taken up the next week.

@@ -246,7 +288,7 @@ class TranslationHelper {
// instantiate the Translation Service
constructTranslationService() {
if (!this.translationService) {
let translationServiceConfig = {};
let translationServiceConfig = { cacheSize: 10 };
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a note: a cache size of 10 won't do you much good. The cache is non-probing, it is basically just cache[hash(sentence) % cacheSize]. The description of the parameter is a bit indirect about that. If you set it too low, you'll end up having too many different sentences hitting the same cache entry, constantly overwriting each other, and no cache benefit at all.

In my experience you'd get about 20% occupancy, so if you set it to 50 you'd be caching about 10 sentences. But from testing in TranslateLocally and my extension fork, I'd suggest starting with something around 1000 or higher, and see whether you can notice it in the memory usage.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the tip Jelmer, please keep them coming. I think caching will be particularly helpful after I move the engine to the background script last week. I heard from the security folks that's fine, so I think that will be helpful.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants