-
Notifications
You must be signed in to change notification settings - Fork 265
add support for agents #564
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
Conversation
Co-authored-by: Salman Paracha <salmanparacha@MacBook-Pro-288.local>
#583) * adding support for Qwen models and fixed issue with passing PATH variable * don't need to have qwen in the model alias routing example * fixed base_url for qwen --------- Co-authored-by: Salman Paracha <salmanparacha@MacBook-Pro-288.local>
Co-authored-by: Salman Paracha <salmanparacha@MacBook-Pro-288.local>
salmanap
left a comment
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 haven't looked at the demos. Those weren't as important as the core logic for agents and agent_filter_chains. Overall the structure looks really good. But lets discuss the few comments below. I think we have an opportunity to harden this a bit more.
| required: | ||
| - version | ||
| - llm_providers | ||
| - listeners |
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.
don't we need model_providers || llm_providers as a required property?
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 is possible to have an agent without llm_providers. Same way we can have prompt listener without llm_providers.
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.
Technically that's true. Should we throw a warning?
adilhafeez
left a comment
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.
thanks @salmanap for reviewing - have responded to your PR. Will update the code by noon.
|
Thank you for making all the changes. This PR looks good - just one thing: we need to have a story on filter chain failures, recovery and that whole workflow looks like. We need some basic work on that. Second, I think given that we don't want to publicly release the feature until full support for MCP, should we pull the demos out of this PR to not confuse developers on our current v. new config. Because they will see both in the demos and will scratch their heads. The second PR is needed anyways to complete the feature on routing and orchestration. |
Update arch_config to define agents. You can now define agents and combine them in a listener using
agentsandlistenerkeys. Here is a sample config defining simple rag agent,