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 Elasticsearch log retriever #100
Conversation
src/backend/config/tools.py
Outdated
@@ -12,6 +12,7 @@ | |||
LangChainVectorDBRetriever, | |||
LangChainWikiRetriever, | |||
TavilyInternetSearch, |
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.
We recently added a new backend/community/tools
folder, can you move all logic there instead?
src/backend/config/tools.py
Outdated
@@ -32,6 +33,7 @@ class ToolName(StrEnum): | |||
Python_Interpreter = "Python_Interpreter" | |||
Calculator = "Calculator" | |||
Tavily_Internet_Search = "Internet Search" | |||
Logs_Retriever = "App Logs" |
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.
Rename this to clarify that the source is Elasticsearch
src/backend/config/tools.py
Outdated
}, | ||
is_visible=True, | ||
is_available=LogsRetriever.is_available(), | ||
error_message="LogsRetriever not available.", |
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.
Add here that the host and index env variables are required
self.index = index | ||
self.deployment_name = deployment_name | ||
self.client = Elasticsearch(host_url) | ||
self.cohere_api_key = os.environ.get("COHERE_API_KEY") |
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.
It looks cohere_api_key
is not required?
logger = get_logger() | ||
|
||
|
||
class NYCServiceRequestRetriever(BaseRetrieval): |
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.
It looks like this retriever tool isn't present in the dict mapping, should we add it? Or was this for testing?
index: str, | ||
deployment_name: str, | ||
) -> None: | ||
self.index = index |
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.
Suggestion: Rather than instantiating LogsRetriever()
with the index and host, can you add these as params on the class, eg :
class LogsRetriever(BaseRetrieval):
index = os.environ.get(.., None)
host = os.environ.get(.., None)
Then your __init__()
method would use self.index
and self.host
,
And your classmethod
is_available()
should check return all[self.index is not None, self.host is not None]
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.
Requested some changes
Thank you for contributing to the Cohere Toolkit!
PR title: "Add search to ELK cluster"
PR message: Delete this entire checklist and replace with
elasticsearch