Feature/ translate tool #7
Conversation
…tool # Conflicts: # docs/docs/integrations/vectorstores/redis.ipynb
| @@ -14,24 +14,82 @@ | |||
| }, | |||
There was a problem hiding this comment.
It looks like some unintended files and changes got added to this PR.
| @@ -0,0 +1,150 @@ | |||
| from __future__ import annotations | |||
There was a problem hiding this comment.
It's a stylistic convention in Python to name files using snake_case instead of camelCase.
azureTranslate.py --> azure_translate.py
edit: I'm also not sure we need to put this under its own azure_translator directory - it seems like the other Azure AI tools all live under /azure_ai_services/.
| "azure-ai-translation-text package." | ||
| ) | ||
|
|
||
| def __init__( |
There was a problem hiding this comment.
same comment as the content safety PR:
should we follow the pattern of the other tools and define a validate_environment method instead of init?
| logger.error("Input text for translation is empty.") | ||
| return None |
There was a problem hiding this comment.
If there is no input text, we should raise a ValueError so the user can catch the exception and act accordingly. It's much harder to programmatically act on a logging error.
| endpoint=translate_endpoint, credential=AzureKeyCredential(translate_key) | ||
| ) | ||
|
|
||
| def _translate_text(self, text: str, to_language: str) -> str: |
There was a problem hiding this comment.
Since this gets called internally, I don't believe the user has the ability to pass to_language. I think we can follow the way speech-to-text handles this by setting a default on the class: https://github.com/langchain-ai/langchain/blob/master/libs/community/langchain_community/tools/azure_ai_services/speech_to_text.py#L29
| logger.warning( | ||
| f"Translation successful: {response[0].translations[0].text}" | ||
| ) # Use WARNING level for successful operations |
There was a problem hiding this comment.
I think it's sufficient to just return the translated text, no need to log
| else: | ||
| logger.error("Translation failed with an empty response") | ||
| return None |
There was a problem hiding this comment.
curious, in what cases does the call to translate return an empty response? this seems unexpected for the API
| # Example test usage for the AzureTranslateTool | ||
| if __name__ == "__main__": | ||
| tool = AzureTranslateTool.from_env() | ||
| try: | ||
| translated_text = tool._run("good morning, How are you?", "es") | ||
| logger.info(f"Translated text: {translated_text}") | ||
| except RuntimeError as e: | ||
| logger.error(f"Error occurred: {e}") |
There was a problem hiding this comment.
It's fine to have this for testing for now, but we'll need to remove it in the final PR
| "AZURE_TRANSLATE_ENDPOINT is missing in environment variables" | ||
| ) | ||
|
|
||
| logger.info(f"API Key: {translate_key[:4]}**** (masked)") |
There was a problem hiding this comment.
we should not log sensitive information, even if we redact some of it
| raise RuntimeError(f"Error while running AzureTranslateTool: {e}") | ||
|
|
||
| @classmethod | ||
| def from_env(cls): |
There was a problem hiding this comment.
was this just added for temp testing? It seems we should read the env var values in the validate_environment.
initial text translate tool