Cache calls to external API services for one week#4020
Conversation
flodolo
force-pushed
the
issue2664
branch
2 times, most recently
from
ff5afb5 to
5e9d648
Compare
mathjazz
left a comment
mathjazz
left a comment
There was a problem hiding this comment.
Looks good!
Left some suggestions to improve readability and reduce cache footprint.
|
Will flag again once ready for review, I think I'm caching unnecessarily in one place. |
|
OK, kept all changes from last review in a single commit All the code is now caching only the translation, not the structure of the response. |
mathjazz
left a comment
mathjazz
left a comment
There was a problem hiding this comment.
Looks good, nice work!
Just realized that we Google and Microsoft utils functions could return just the translation data (like the OpenAI service does), in which case the code handling caching would be more readable and we'd take care of the JSON response format in the view.
But I'll leave the decision to implement that or merge as is to you.
|
Sounds good. Let me take a look when I'm done with meetings. |
flodolo
force-pushed
the
issue2664
branch
3 times, most recently
from
22fb04d to
b83e1b5
Compare
Also move code handling MS query to utils
It's a bit larger than I expected, since I had to deal with the error statuses Do you want to review the changes? The code is now raising the exception, and using that in the view to log the error. Also move the MS code to |
| "status": False, | ||
| "message": f"{e}", | ||
| } | ||
| client = translate.TranslationServiceClient() |
There was a problem hiding this comment.
Why do we no longer cache the error here?
There was a problem hiding this comment.
Caching the error = wrapping in try/except?
Before we were catching DefaultCredentialsError. This would propagate to _machinery_error_response() and still be logged with a generic error.
I'm going to wrap it back and use a clearer error message.
Let me know if that makes sense.
There was a problem hiding this comment.
Ugh, I meant "catch". :) Thanks.
2 participants
Fixes #2664