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

Add LibreTranslate machine translator, documentation and its test cases. #753

Closed
wants to merge 8 commits into from

Conversation

drivard
Copy link
Contributor

@drivard drivard commented Dec 18, 2023

This PR introduces a new machine translator to the wagtail-localize project, leveraging the LibreTranslate API. The LibreTranslator class extends the BaseMachineTranslator and implements the necessary methods for translation.

It supports configuration alike

WAGTAILLOCALIZE_MACHINE_TRANSLATOR = {
    "CLASS": "wagtail_localize.machine_translators.libretranslate.LibreTranslator",
    "OPTIONS": {
        "LIBRETRANSLATE_URL": "https://libretranslate.org",
        "API_KEY": "<Your LibreTranslate api key here>",
    },
}

The LibreTranslate API can also be hosted using many methods documented on their github page https://github.com/LibreTranslate/LibreTranslate.
I tested it against a self-hosted instance.

Here are some screenshots of an about us page that I translated using the LibreTranslator.

image

image

Please review and provide any feedback.

@codecov-commenter
Copy link

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (a3efa8f) 92.60% compared to head (8d0d62b) 92.53%.

Files Patch % Lines
...ail_localize/machine_translators/libretranslate.py 76.47% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #753      +/-   ##
==========================================
- Coverage   92.60%   92.53%   -0.07%     
==========================================
  Files          46       47       +1     
  Lines        4017     4034      +17     
  Branches      598      600       +2     
==========================================
+ Hits         3720     3733      +13     
- Misses        175      179       +4     
  Partials      122      122              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@zerolab zerolab left a comment

Choose a reason for hiding this comment

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

@drivard thank you for this.

Left a few suggestions for improvement

docs/how-to/integrations/machine-translation.md Outdated Show resolved Hide resolved
api_endpoint = self.translator.get_api_endpoint()
self.assertEqual(api_endpoint, "https://libretranslate.org")

# This probably requires a request to use the API but the test works against my local instance
Copy link
Collaborator

Choose a reason for hiding this comment

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

rather than making an actual API call, I'd test 2 things:

  1. that translate will raise for status (so, mock the post call to that effect)
  2. that we get a dict of string: StringValue(translation) from the response, if there are translated strings. Again, using a mocked response

Copy link
Contributor Author

Choose a reason for hiding this comment

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

could I have a bit of pointer on mocking a post request please?

docs/how-to/integrations/machine-translation.md Outdated Show resolved Hide resolved
"CLASS": "wagtail_localize.machine_translators.libretranslate.LibreTranslator",
"OPTIONS": {
"LIBRETRANSLATE_URL": "https://libretranslate.org",
"API_KEY": "<Your LibreTranslate api key here>", # Optional on self-hosted instance by providing a random string
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"API_KEY": "<Your LibreTranslate api key here>", # Optional on self-hosted instance by providing a random string
"API_KEY": "<Your LibreTranslate api key here>", # Use any random string with self-hosted instances.

Copy link
Contributor Author

@drivard drivard Dec 18, 2023

Choose a reason for hiding this comment

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

Maybe something like this would be more appropriate
# On self-hosted instances, if you did not setup the api keys support use any random string.

@drivard drivard requested a review from zerolab December 18, 2023 19:01
@drivard
Copy link
Contributor Author

drivard commented Dec 18, 2023

@zerolab I have completed the changes requests. Please review.

@drivard drivard requested a review from zerolab December 19, 2023 19:17
Copy link
Collaborator

@zerolab zerolab left a comment

Choose a reason for hiding this comment

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

Thank you @drivard, merged in a458ed8

@zerolab zerolab closed this Dec 20, 2023
@drivard
Copy link
Contributor Author

drivard commented Dec 20, 2023

@zerolab Do you have an idea of when it will be released ?

@zerolab
Copy link
Collaborator

zerolab commented Dec 20, 2023

Got a couple of more PRs and if all goes well, sometime next week (i.e before the end of the year)

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.

None yet

3 participants