-
Notifications
You must be signed in to change notification settings - Fork 0
Refactoring #5
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
Refactoring #5
Conversation
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.
Pull Request Overview
This PR refactors the AI Agent framework by improving documentation, modularizing tool integration, and enhancing debug output and chat workflows with both local and MCP tools.
- Added comprehensive architecture documentation (
docs/architecture.md) and updatedREADME.md - Introduced a
pretty_printutility and expanded debug logging across Azure OpenAI client, chat, and MCP sessions - Refactored tool registration/processing: replaced
tool_mapwith dynamically built tool lists/sets and delegated tool call handling toChat
Reviewed Changes
Copilot reviewed 54 out of 54 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| docs/architecture.md | New file detailing component architecture and sequence diagrams |
| src/utils/pretty.py | Added colorize_text, colorize_json, and prettify utilities |
| src/utils/azureopenai/chat.py | Enhanced Chat with debug logging and centralized tool processing |
| src/agent.py | Switched to dynamic tools set, updated chat instantiation |
|
|
||
| return f"{color}{text}{Style.RESET_ALL}" | ||
|
|
||
| def colorize_json(json_str: Dict[Any, Any], indent: int = 2) -> str: |
Copilot
AI
May 12, 2025
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.
The parameter json_str is annotated as a Dict but the function treats it as a string (calling split). Update the type hint to str or rename the parameter to reflect its actual usage.
| def colorize_json(json_str: Dict[Any, Any], indent: int = 2) -> str: | |
| def colorize_json(json_str: str, indent: int = 2) -> str: |
| print(colorize_text(f"\n<Active MCP Server: {colorize_text(server_name, "magenta")}>", "cyan")) | ||
| for tool in session.tools: | ||
| print(colorize_text(f"<Tool Initialized: {colorize_text(tool.name, "yellow")}>", "cyan")) |
Copilot
AI
May 12, 2025
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.
Nested double quotes inside this f-string will cause a syntax error. Use single quotes for the inner literal or escape the quotes appropriately.
| print(colorize_text(f"\n<Active MCP Server: {colorize_text(server_name, "magenta")}>", "cyan")) | |
| for tool in session.tools: | |
| print(colorize_text(f"<Tool Initialized: {colorize_text(tool.name, "yellow")}>", "cyan")) | |
| print(colorize_text(f"\n<Active MCP Server: {colorize_text(server_name, 'magenta')}>", "cyan")) | |
| for tool in session.tools: | |
| print(colorize_text(f"<Tool Initialized: {colorize_text(tool.name, 'yellow')}>", "cyan")) |
|
|
||
| if Chat.debug: | ||
| for tool in tool_list: | ||
| print(colorize_text(f"<Tool Initialized: {colorize_text(tool.name, "yellow")}>", "cyan")) |
Copilot
AI
May 12, 2025
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.
This f-string uses nested double quotes and will not parse. Swap inner quotes to single or escape them.
| print(colorize_text(f"<Tool Initialized: {colorize_text(tool.name, "yellow")}>", "cyan")) | |
| print(colorize_text(f"<Tool Initialized: {colorize_text(tool.name, 'yellow')}>", "cyan")) |
| arguments = function_data.get("arguments", "{}") | ||
|
|
||
| if Chat.debug: | ||
| print(colorize_text(f"<Tool Call: {colorize_text(tool_name, "green")}> ", "yellow"), arguments) |
Copilot
AI
May 12, 2025
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.
Nested double quotes in this f-string will lead to a syntax error. Use consistent quoting (e.g., single quotes inside).
| print(colorize_text(f"<Tool Call: {colorize_text(tool_name, "green")}> ", "yellow"), arguments) | |
| print(colorize_text(f"<Tool Call: {colorize_text(tool_name, 'green')}> ", "yellow"), arguments) |
| print(colorize_text(f"<Tool Result: {colorize_text(tool_name, "green")}> ", "yellow"), prettify(tool_result)) | ||
| except Exception as e: | ||
| tool_result = { | ||
| "error": f"Error running tool '{tool_name}': {str(e)}" | ||
| } | ||
| if Chat.debug: | ||
| print(colorize_text(f"<Tool Exception: {colorize_text(tool_name, "red")}> ", "yellow"), str(e)) |
Copilot
AI
May 12, 2025
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.
This f-string also nests double quotes and will not compile; switch inner quotes to single quotes or escape them.
| print(colorize_text(f"<Tool Result: {colorize_text(tool_name, "green")}> ", "yellow"), prettify(tool_result)) | |
| except Exception as e: | |
| tool_result = { | |
| "error": f"Error running tool '{tool_name}': {str(e)}" | |
| } | |
| if Chat.debug: | |
| print(colorize_text(f"<Tool Exception: {colorize_text(tool_name, "red")}> ", "yellow"), str(e)) | |
| print(colorize_text(f"<Tool Result: {colorize_text(tool_name, 'green')}> ", "yellow"), prettify(tool_result)) | |
| except Exception as e: | |
| tool_result = { | |
| "error": f"Error running tool '{tool_name}': {str(e)}" | |
| } | |
| if Chat.debug: | |
| print(colorize_text(f"<Tool Exception: {colorize_text(tool_name, 'red')}> ", "yellow"), str(e)) |
|
|
||
| hr = "#" * 50 | ||
| header = f"\n{hr} <{name}> {hr}\n" | ||
| footer = f"\n####{hr * 2}{"#" * len(name)}" |
Copilot
AI
May 12, 2025
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.
The f-string mixes double quotes for both the string and an inner expression, causing a syntax error. Use alternating quote styles or escape.
| footer = f"\n####{hr * 2}{"#" * len(name)}" | |
| footer = f"\n####{hr * 2}{'#' * len(name)}" |
| tools = { | ||
| GoogleSearch("google_search"), | ||
| ReadFile("read_file"), | ||
| WriteFile("write_file"), | ||
| ListFiles("list_files"), | ||
| WebFetch("web_fetch") | ||
| } |
Copilot
AI
May 12, 2025
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.
A set literal is used for tools, which is unordered and requires Tool to be hashable. Consider using a list for predictable iteration order.
| tools = { | |
| GoogleSearch("google_search"), | |
| ReadFile("read_file"), | |
| WriteFile("write_file"), | |
| ListFiles("list_files"), | |
| WebFetch("web_fetch") | |
| } | |
| tools = [ | |
| GoogleSearch("google_search"), | |
| ReadFile("read_file"), | |
| WriteFile("write_file"), | |
| ListFiles("list_files"), | |
| WebFetch("web_fetch") | |
| ] |
| def __init__(self, client, tool_map: Dict[str, Any] = {}): | ||
| debug = DEBUG | ||
|
|
||
| def __init__(self, client, tool_list: List[Tool] = []): |
Copilot
AI
May 12, 2025
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.
Using a mutable default argument ([]) can lead to shared state across instances. Use None and assign an empty list inside the method.
| def __init__(self, client, tool_list: List[Tool] = []): | |
| def __init__(self, client, tool_list: List[Tool] = None): | |
| if tool_list is None: | |
| tool_list = [] |
|
|
||
| @classmethod | ||
| def create(cls, tool_map = {}) -> 'Chat': | ||
| def create(cls, tool_list = []) -> 'Chat': |
Copilot
AI
May 12, 2025
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.
Mutable default argument ([]) in a factory method may retain tools between calls. Change default to None and initialize internally.
| def create(cls, tool_list = []) -> 'Chat': | |
| def create(cls, tool_list=None) -> 'Chat': | |
| if tool_list is None: | |
| tool_list = [] | |
This pull request introduces significant enhancements to the AI Agent framework, including architecture documentation, modularization of tools, and improved functionality for chat interactions with tools and MCP servers. The changes focus on improving code clarity, extensibility, and user experience.
Documentation Enhancements:
docs/architecture.mdfile detailing the AI Agent's architecture, components, and workflow, including sequence diagrams and descriptions of tool interactions.README.mdto include a high-level description of the component-based architecture and a link to the detailed architecture documentation.New Features:
demo/2-tool.py: Demonstrates querying a local tool (google_search) and printing results.demo/3-chat-with-tools.py: Showcases chat functionality with integrated tools likeGoogleSearchandWriteFile.demo/4-chat-with-tools-and-mcp.py: Extends functionality to include MCP server tools, demonstrating dynamic tool discovery and usage.Code Refactoring:
src/agent.pyto replace thetool_mapdictionary with atoolsset for better modularity and dynamic tool addition. Simplified tool call processing by delegating it to theChatclass. [1] [2] [3] [4]src/toolsmodules (google_search.py,list_files.py,read_file.py) to dynamically use the tool's name in their definitions, improving flexibility. [1] [2] [3]Debugging and Utility Enhancements:
set_debug(True)in multiple scripts (demo/1-chat.py,src/main.py) to enable debugging during execution. [1] [2]pretty_printutility for consistent and formatted output in demo scripts. [1] [2]Minor Improvements:
src/chat.pytodemo/1-chat.pyfor better organization and to align with the new demo structure.list_files.pyto useresultinstead ofmessagefor clarity.