-
Notifications
You must be signed in to change notification settings - Fork 571
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 language support to translation client, solves #1763 #1869
Conversation
The documentation is not available anymore as the PR was closed or merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ceferisbarov thanks for opening the PR! It is already in good shape with the logic + docs + tests 🚀 I've added a few comments below. Other than that there are 2 things to do:
- There is a
InferenceClient
and anAsyncInferenceClient
. When the logic is updated in one, the other should be updated as well. Luckily we have a script to automatically port the logic to the async client. You should first installpip install -e ".[dev]"
locally (see here). Then runmake style
which will apply the code linter + code formatter + the script to generate async client. Once done, commit the changes and that's it. - The tests in
test_inference_client.py
are mocked. This means that the requests are not really made to the Inference server each time you run the test. What happens is that the first time you run the test, it will create a file locally under./tests/cassettes
. The next times you run the test, this file will be reused. What you need to do is to run the test to create the mock file automatically, then check the content "looks good" and finally commit it to the repo.
If you need any help for one of those things, just let me know 🤗
Co-authored-by: Lucain <lucainp@gmail.com>
Co-authored-by: Lucain <lucainp@gmail.com>
Co-authored-by: Lucain <lucainp@gmail.com>
@Wauplin I didn't know abot test cassettes. Cool stuff. Can you have another look at the PR? Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thanks for making the changes and uploading the VCR cassette 🔥
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made 2 small changes (typo + add comment about model card).
Let's wait for the CI to complete and it'll be good to merge :) Some tests are failing for it's not related to the PR.
No description provided.