-
Notifications
You must be signed in to change notification settings - Fork 75
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
Suggestion: Asynchronous Support #46
Comments
Hi @nfnfgo, that is right, currently only synchronous calls are possible because the library is based on We would like to add asynchronous functionality to this library in future. |
I think the fastest solution for now could be to generate async client based on available OpenAPI specification. But yes, it's nice to have officially supported "async" version |
@daniel-jones-deepl if this library would use httpx, it could support both sync and async easily. It has API most similar to |
Are there any further updates on if this will be happening/the current status? |
+1 for async support. aiohttp or httpx |
+1 |
Just wanna add, it seems like a single file, the http client file, that needs to be updated. Shouldn't be that hard for anyone who wants this and has some time. (this is directed towards non-maintainers looking at this) |
Hi everyone, thanks for adding your interest on this feature. (Note: this is @daniel-jones-deepl, I've migrated to my personal account). I had a quick look into this, Regarding only changing the file http_client.py: it would be an option. Another user implemented a DeepL Python library using something similar. However it doesn't allow proper typing support. I think the best way to support async would be add a new class On Friday I have some time to work on that more, I'll post another update then. |
@daniel-jones-dev Heck yeah! This will make lots of Discord bot developers (who like accessibility through readily available translations) happy |
I've pushed some WIP changes to the async branch, and I'd be interested in feedback. Lots of things are broken (see below), but the following should work with
Currently only Anyway, feedback is welcome! |
Hi! The WIP async version works for me, but I see there is not a lot of progress here. Maybe I could contribute some code to help with it. The proxy and SSL verification should be easy enough to implement, and then I could do the user-facing APIs. I wonder how the test will be adapted to test async implementation too. |
It seems that the WIP async branch has broken sync implementation! I don't know (but assume) that it is fine in the main branch. So I will need to find the issue with sync first - they use the same code to process the results from the network. Here is the code to reproduce: import asyncio
import traceback
import deepl
async def main():
trans = deepl.Translator(
"KEY",
# proxy={"https": "https://localhost:8080"},
# verify_ssl=False,
)
trans_async = deepl.TranslatorAsync("KEY")
try:
print(trans.translate_text("test", source_lang="EN", target_lang="RU"))
except Exception as e:
traceback.print_exception(e)
print("translation results: " + await trans_async.translate_text("test", source_lang="EN", target_lang="RU"))
await trans_async.close()
asyncio.run(main()) and the output on my side:
So the async version works, while sync doesn't. |
Hi @Nereg, I did make some progress on it a few weeks back, but I forgot to push those changes. I've pushed them to the async branch now. The changes fix all the existing tests for the sync implementation, and the async translate_text function works too. The async tests are incomplete and failing in the CI because asyncio is not installed. Some remaining work to be done:
|
Sorry for the delay, anyway. In my testing (although, I can't replicate that if I install directly from async branch, for some reason) I got this error:
It is caused by this code, not recognizing that it is a JSON response deepl-python/deepl/translator_base.py Lines 101 to 109 in c70f06a
There could be many variations of both Content-Type and application/json . Which is why I changed it to try to decode JSON regardless of MIME type: Nereg@9b85972Do you think there is a better way? I will now work on proxies and SSL verification, it could be difficult, if we want full interoperability. |
Nereg@24fd132 Implemented both full disablement of SSL verification and using your own SSL CA certificates. Not sure, if it will behave precisely like request's implementation, but seems fine, |
For now, I added proxy support: Nereg@efd9455 |
Hi @Nereg, thanks for testing and fixing these things. I struggle at the moment to find time to work on this. Your JSON-decoding, SSL and proxy changes all look good. You should add the SSL and proxy parameters to TranslatorAsync -- it does implement the requests, see the with_base_pre_and_post decorator: the request-functions are all implemented in three steps: pre-function, request, post-function. The pre- and post-functions come from TranslatorBase and are shared with the synchronous Translator. I've just pushed some more changes -- all async request functions work now, including document translation. I incorporated some of your changes above (will fix those commits later to give you credit). Also added some more async tests, but ideally we'd add some more for SSL and proxy before merging. |
While browsing the code, I found a weird thing, that I think should not be this way. deepl-python/deepl/aiohttp_http_client.py Lines 15 to 19 in adcb682
deepl-python/deepl/aiohttp_http_client.py Lines 86 to 89 in adcb682
So, in those lines, we first import the aiohttp library, and if it fails - we store the exception to throw it later. Wouldn't it be better to fail-fast ? As far as I can see - it is only imported in translator_async, which is a resonable place to fail, if there is no aiohttp. Furthermore, there is no such code in the requests_http_client.py which could also fail, if there is no requests installed. I can fix that, if there is no reason behind it. Currently working on applying my SSL and proxy changes to the code you pushed. It seems like you've done the JSON detection part already. Then will work on covering those with tests. |
So, I've applied the SSL bypass and proxy support, added tests for them, and fixed the |
I admit I haven't fully thought out the design of the conditional importing. My idea was to simplify the user experience: I didn't include conditionals on Thanks for the contributions; I have to review them later. |
It really a nice thing to hear DeepL provide a Python client module.
Through the doc I found that this module seems not support async functions, and I think it would be better to use a async request module such as aiohttp to make the translation can be used asynchronously
The text was updated successfully, but these errors were encountered: