-
Notifications
You must be signed in to change notification settings - Fork 12
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
ENG-504: Make the agent architecture configurable #243
Conversation
@@ -44,7 +44,7 @@ class AgentFactory: | |||
def create( | |||
cls, | |||
name: Text, | |||
llm_id: Text, | |||
llm_id: Text = "669a63646eb56306647e1091", |
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.
Please add Optional
in the method definition for all optional parameters e.g. Optional[llm_id]
.
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 think the use of Optional
in type annotations in Python is intended to indicate that a value can either be of the specified type or None
, no?. Since it has a not null value there is not need to use Optional. Please confirm.
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.
yes, you're correct. Didn't know that actually: https://docs.python.org/3/library/typing.html#typing.Optional
@@ -67,6 +67,8 @@ def create( | |||
Returns: | |||
Agent: created Agent | |||
""" | |||
# assert that at least one tool is provided | |||
assert len(tools) > 0, "At least one tool must be provided." |
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.
Do we need to force user to have at least one ModelTool
or PipelineTool
? I think if the user wants to use only serp api search we should let it do that.
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.
That's what the product asked, but you are correct. Implicitly we have serpAPI. I am going to change.
No description provided.