-
Notifications
You must be signed in to change notification settings - Fork 5.6k
feat: Foundry agent adapter #2852
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
|
Disclaimer: This review was made by a crew of AI Agents. Code Review Comment: Foundry Agent Adapter ImplementationOverviewThis code implements a Foundry Agent adapter for crewAI that integrates with Azure AI services. It introduces important functionality for handling Azure-based AI agents across multiple files. Code Quality Findings1. Import Organization and DependenciesFindings:
Suggestions:
2. Error Handling and LoggingFindings:
Suggestions:
3. Configuration ManagementFindings:
Suggestions:
Example Code: class FoundryConfig(BaseModel):
connection_string: str4. Tool Adapter ImplementationFindings:
Suggestions:
5. Thread ManagementFindings:
Suggestions:
Example Addition: async def cleanup_threads(self):
"""Cleanup active threads."""6. Result ProcessingFindings:
Suggestions:
Example Enhancement: def _process_messages(self, messages: Any) -> str:
if not messages.data:
raise ValueError("No messages received.")Historical Context and Related PRsAlthough the specific pull request history isn’t accessible, referencing similar changes in related PRs may demonstrate how previous issues were resolved or improved upon. It’s essential to maintain best practices and learn from historical decisions regarding error management, configuration handling, and modular design. Implications for Related FilesThe modifications within this PR will likely influence how tools are adapted and how configuration settings are handled across the crewAI framework. Further integration tests should be implemented to ensure that these changes work seamlessly with existing code. General Recommendations
Security Considerations
In summary, while this implementation establishes a solid foundation for the integration of Foundry services, it must undergo several enhancements in error handling, configuration management, and resource cleaning to achieve production readiness. This structured review encapsulates specific code improvements, suggestions, and potential implications of changes. Please make sure to integrate community feedback as well for continuous enhancement. |
src/crewai/agents/agent_adapters/foundry_agents/foundry_adapter.py
Outdated
Show resolved
Hide resolved
lucasgomide
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.
@bassmang nice work here! Thank you for your collaboration
I dropped a few comments, let me know if you need any help with them.
Would you mind recording a Loom session showing this agent working alongside a Crew?
src/crewai/agents/agent_adapters/foundry_agents/foundry_adapter.py
Outdated
Show resolved
Hide resolved
src/crewai/agents/agent_adapters/foundry_agents/foundry_adapter.py
Outdated
Show resolved
Hide resolved
| role = kwargs.pop("role", None) | ||
| goal = kwargs.pop("goal", None) | ||
| backstory = kwargs.pop("backstory", 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.
I think those attributes would be explicitly defined on init since there are the required ones to build an Agent
| ) | ||
| agent = self._foundry_client.agents.create_agent( | ||
| model=self.llm, | ||
| name=self.role or "crewai-agent", |
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'd recommend use the same Agent's role. Take in mind that the role can not be none.
| if self._tool_adapter.converted_tools: | ||
| self._converted_tools = self._tool_adapter.converted_tools |
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.
Prefer using the .tools(), you can also drop this if condition
| if self._tool_adapter.converted_tools: | |
| self._converted_tools = self._tool_adapter.converted_tools | |
| self._converted_tools = self._tool_adapter.tools() |
| def __init__(self, tools: Optional[List[BaseTool]] = None): | ||
| self.original_tools = tools or [] |
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 this method could be removed. You can rely on BaseToolAdapter initialization behavior
| # test.py | ||
| from crewai import Task, Crew, Process | ||
| from crewai.agents.agent_adapters.foundry_agents.foundry_adapter import FoundryAgentAdapter | ||
| from crewai_tools import SerperDevTool | ||
| import os | ||
|
|
||
| # Create a single agent | ||
| agent = FoundryAgentAdapter( | ||
| role="Quick Assistant", | ||
| goal="Answer simple questions clearly", | ||
| backstory="You're a helpful assistant", | ||
| verbose=True, | ||
| tools=[SerperDevTool()], | ||
| ) | ||
|
|
||
| # Define a task | ||
| task = Task( | ||
| description="Answer the question: 'What is the capital of France?'", | ||
| expected_output="The capital of France is Paris.", | ||
| agent=agent, | ||
| ) | ||
|
|
||
| # Create a crew to run it | ||
| crew = Crew( | ||
| agents=[agent], | ||
| tasks=[task], | ||
| process=Process.sequential, | ||
| verbose=True, | ||
| ) | ||
|
|
||
| # Run the crew | ||
| if __name__ == "__main__": | ||
| result = crew.kickoff() | ||
| print("Result:", result) |
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 supposed this file should contains assert, right?
|
This PR is stale because it has been open for 45 days with no activity. |
|
This PR is stale because it has been open for 45 days with no activity. |
No description provided.