From d468909555a858dc8ef1b41c804b8e2f72579c09 Mon Sep 17 00:00:00 2001 From: Yj Date: Tue, 17 Dec 2024 00:03:50 +0800 Subject: [PATCH 1/5] created better throw exception logic for tools remove any sys.exit on business logic level for tools, and only raise exceptions. let cli/tools.py handle any errors and do sys.exit --- agentstack/cli/tools.py | 99 ++++++++++++++---------- agentstack/exceptions.py | 7 ++ agentstack/generation/tool_generation.py | 10 +-- agentstack/tools.py | 15 ++-- 4 files changed, 76 insertions(+), 55 deletions(-) diff --git a/agentstack/cli/tools.py b/agentstack/cli/tools.py index bee8a537..80e4b733 100644 --- a/agentstack/cli/tools.py +++ b/agentstack/cli/tools.py @@ -1,31 +1,37 @@ from typing import Optional import itertools +import sys import inquirer from agentstack.utils import term_color from agentstack import generation from agentstack.tools import get_all_tools from agentstack.agents import get_all_agents +from agentstack.exceptions import ToolError def list_tools(): """ List all available tools by category. """ - tools = get_all_tools() - curr_category = None + try: + tools = get_all_tools() + curr_category = None - print("\n\nAvailable AgentStack Tools:") - for category, tools in itertools.groupby(tools, lambda x: x.category): - if curr_category != category: - print(f"\n{category}:") - curr_category = category - for tool in tools: - print(" - ", end='') - print(term_color(f"{tool.name}", 'blue'), end='') - print(f": {tool.url if tool.url else 'AgentStack default tool'}") + print("\n\nAvailable AgentStack Tools:") + for category, tools in itertools.groupby(tools, lambda x: x.category): + if curr_category != category: + print(f"\n{category}:") + curr_category = category + for tool in tools: + print(" - ", end='') + print(term_color(f"{tool.name}", 'blue'), end='') + print(f": {tool.url if tool.url else 'AgentStack default tool'}") - print("\n\n✨ Add a tool with: agentstack tools add ") - print(" https://docs.agentstack.sh/tools/core") + print("\n\n✨ Add a tool with: agentstack tools add ") + print(" https://docs.agentstack.sh/tools/core") + except ToolError as e: + print(term_color(f"Error listing tools: {str(e)}", 'red')) + sys.exit(1) def add_tool(tool_name: Optional[str], agents=Optional[list[str]]): @@ -38,32 +44,45 @@ def add_tool(tool_name: Optional[str], agents=Optional[list[str]]): - add the tool to the user's project - add the tool to the specified agents or all agents if none are specified """ - if not tool_name: - # ask the user for the tool name - tools_list = [ - inquirer.List( - "tool_name", - message="Select a tool to add to your project", - choices=[tool.name for tool in get_all_tools()], - ) - ] - try: - tool_name = inquirer.prompt(tools_list)['tool_name'] - except TypeError: - return # user cancelled the prompt + try: + if not tool_name: + # ask the user for the tool name + tools_list = [ + inquirer.List( + "tool_name", + message="Select a tool to add to your project", + choices=[tool.name for tool in get_all_tools()], + ) + ] + try: + tool_name = inquirer.prompt(tools_list)['tool_name'] + except TypeError: + return # user cancelled the prompt - # ask the user for the agents to add the tool to - agents_list = [ - inquirer.Checkbox( - "agents", - message="Select which agents to make the tool available to", - choices=[agent.name for agent in get_all_agents()], - ) - ] - try: - agents = inquirer.prompt(agents_list)['agents'] - except TypeError: - return # user cancelled the prompt + # ask the user for the agents to add the tool to + agents_list = [ + inquirer.Checkbox( + "agents", + message="Select which agents to make the tool available to", + choices=[agent.name for agent in get_all_agents()], + ) + ] + try: + agents = inquirer.prompt(agents_list)['agents'] + except TypeError: + return # user cancelled the prompt - assert tool_name # appease type checker - generation.add_tool(tool_name, agents=agents) + assert tool_name # appease type checker + generation.add_tool(tool_name, agents=agents) + except ToolError as e: + print(term_color(f"Error adding tool: {str(e)}", 'red')) + sys.exit(1) + + +def remove_tool(tool_name: str, agents: Optional[list[str]] = []): + """Remove a tool from the project""" + try: + generation.remove_tool(tool_name, agents=agents) + except ToolError as e: + print(term_color(f"Error removing tool: {str(e)}", 'red')) + sys.exit(1) diff --git a/agentstack/exceptions.py b/agentstack/exceptions.py index c0e95569..6fc2cff5 100644 --- a/agentstack/exceptions.py +++ b/agentstack/exceptions.py @@ -1,3 +1,4 @@ +from pathlib import Path class ValidationError(Exception): """ Raised when a validation error occurs ie. a file does not meet the required @@ -5,3 +6,9 @@ class ValidationError(Exception): """ pass + + +class ToolError(Exception): + """Base exception for tool-related errors""" + + pass diff --git a/agentstack/generation/tool_generation.py b/agentstack/generation/tool_generation.py index 314ff481..5ee28ad7 100644 --- a/agentstack/generation/tool_generation.py +++ b/agentstack/generation/tool_generation.py @@ -1,5 +1,4 @@ import os -import sys from typing import Optional from pathlib import Path import shutil @@ -7,7 +6,7 @@ from agentstack import conf from agentstack.conf import ConfigFile -from agentstack.exceptions import ValidationError +from agentstack.exceptions import ValidationError, ToolError from agentstack import frameworks from agentstack import packaging from agentstack.utils import term_color @@ -97,7 +96,7 @@ def add_tool(tool_name: str, agents: Optional[list[str]] = []): with ToolsInitFile(conf.PATH / TOOLS_INIT_FILENAME) as tools_init: tools_init.add_import_for_tool(tool, agentstack_config.framework) except ValidationError as e: - print(term_color(f"Error adding tool:\n{e}", 'red')) + raise ToolError(f"Error adding tool:\n{e}") if tool.env: # add environment variables which don't exist with EnvFile() as env: @@ -129,8 +128,7 @@ def remove_tool(tool_name: str, agents: Optional[list[str]] = []): agentstack_config = ConfigFile() if tool_name not in agentstack_config.tools: - print(term_color(f'Tool {tool_name} is not installed', 'red')) - sys.exit(1) + raise ToolError(f'Tool {tool_name} is not installed') tool = ToolConfig.from_tool_name(tool_name) if tool.packages: @@ -146,7 +144,7 @@ def remove_tool(tool_name: str, agents: Optional[list[str]] = []): with ToolsInitFile(conf.PATH / TOOLS_INIT_FILENAME) as tools_init: tools_init.remove_import_for_tool(tool, agentstack_config.framework) except ValidationError as e: - print(term_color(f"Error removing tool:\n{e}", 'red')) + raise ToolError(f"Error removing tool:\n{e}") # Edit the framework entrypoint file to exclude the tool in the agent definition if not agents: # If no agents are specified, remove the tool from all agents diff --git a/agentstack/tools.py b/agentstack/tools.py index 1acb8d97..2d35ce1f 100644 --- a/agentstack/tools.py +++ b/agentstack/tools.py @@ -1,9 +1,9 @@ from typing import Optional import os -import sys from pathlib import Path import pydantic from agentstack.utils import get_package_path, open_json_file, term_color +from agentstack.exceptions import ValidationError, ToolError class ToolConfig(pydantic.BaseModel): @@ -26,9 +26,8 @@ class ToolConfig(pydantic.BaseModel): @classmethod def from_tool_name(cls, name: str) -> 'ToolConfig': path = get_package_path() / f'tools/{name}.json' - if not os.path.exists(path): # TODO raise exceptions and handle message/exit in cli - print(term_color(f'No known agentstack tool: {name}', 'red')) - sys.exit(1) + if not os.path.exists(path): + raise ToolError(f'No known agentstack tool: {name}') return cls.from_json(path) @classmethod @@ -37,11 +36,9 @@ def from_json(cls, path: Path) -> 'ToolConfig': try: return cls(**data) except pydantic.ValidationError as e: - # TODO raise exceptions and handle message/exit in cli - print(term_color(f"Error validating tool config JSON: \n{path}", 'red')) - for error in e.errors(): - print(f"{' '.join([str(loc) for loc in error['loc']])}: {error['msg']}") - sys.exit(1) + error_msg = f"Error validating tool config JSON: \n{path}\n" + error_msg += "\n".join(f"{' '.join([str(loc) for loc in error['loc']])}: {error['msg']}" for error in e.errors()) + raise ValidationError(error_msg) @property def module_name(self) -> str: From 63a5c75d5dfe43b27ef6a7a8b167ac886b4f143a Mon Sep 17 00:00:00 2001 From: Yj Date: Thu, 19 Dec 2024 14:42:56 +0800 Subject: [PATCH 2/5] added more friendly message for tools cli --- agentstack/cli/tools.py | 34 ++++++++++++++++++++++++++++------ 1 file changed, 28 insertions(+), 6 deletions(-) diff --git a/agentstack/cli/tools.py b/agentstack/cli/tools.py index 80e4b733..1dfa4c7d 100644 --- a/agentstack/cli/tools.py +++ b/agentstack/cli/tools.py @@ -6,7 +6,7 @@ from agentstack import generation from agentstack.tools import get_all_tools from agentstack.agents import get_all_agents -from agentstack.exceptions import ToolError +from agentstack.exceptions import ToolError, ValidationError def list_tools(): @@ -29,8 +29,15 @@ def list_tools(): print("\n\n✨ Add a tool with: agentstack tools add ") print(" https://docs.agentstack.sh/tools/core") - except ToolError as e: - print(term_color(f"Error listing tools: {str(e)}", 'red')) + except ToolError: + print(term_color("Could not retrieve list of tools. The tools directory may be corrupted or missing.", 'red')) + sys.exit(1) + except ValidationError: + print(term_color("Some tool configurations are invalid and could not be loaded.", 'red')) + print(term_color("Please check your tool JSON files for formatting errors.", 'red')) + sys.exit(1) + except Exception: + print(term_color("An unexpected error occurred while listing tools.", 'red')) sys.exit(1) @@ -74,8 +81,14 @@ def add_tool(tool_name: Optional[str], agents=Optional[list[str]]): assert tool_name # appease type checker generation.add_tool(tool_name, agents=agents) - except ToolError as e: - print(term_color(f"Error adding tool: {str(e)}", 'red')) + except ToolError: + print(term_color(f"Could not add tool '{tool_name}'. Run 'agentstack tools list' to see available tools.", 'red')) + sys.exit(1) + except ValidationError: + print(term_color(f"Tool configuration is invalid. Please check the tool's JSON configuration file.", 'red')) + sys.exit(1) + except Exception: + print(term_color("An unexpected error occurred while adding the tool.", 'red')) sys.exit(1) @@ -84,5 +97,14 @@ def remove_tool(tool_name: str, agents: Optional[list[str]] = []): try: generation.remove_tool(tool_name, agents=agents) except ToolError as e: - print(term_color(f"Error removing tool: {str(e)}", 'red')) + if "not installed" in str(e): + print(term_color(f"Tool '{tool_name}' is not installed in this project.", 'red')) + else: + print(term_color(f"Could not remove tool '{tool_name}'. The tool may be in use or corrupted.", 'red')) + sys.exit(1) + except ValidationError: + print(term_color(f"Invalid tool configuration detected while removing '{tool_name}'.", 'red')) + sys.exit(1) + except Exception: + print(term_color("An unexpected error occurred while removing the tool.", 'red')) sys.exit(1) From 431b58f782401ea70c5a27312f577e2a3dde7e6a Mon Sep 17 00:00:00 2001 From: Yj Date: Mon, 23 Dec 2024 11:40:14 +0800 Subject: [PATCH 3/5] log exceptions addresses in only the case of a validation error, it'd be smart to log the exception because it will help them track down what's failing in the validation of their tool config. --- agentstack/cli/tools.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/agentstack/cli/tools.py b/agentstack/cli/tools.py index 1dfa4c7d..86c65b20 100644 --- a/agentstack/cli/tools.py +++ b/agentstack/cli/tools.py @@ -32,9 +32,9 @@ def list_tools(): except ToolError: print(term_color("Could not retrieve list of tools. The tools directory may be corrupted or missing.", 'red')) sys.exit(1) - except ValidationError: + except ValidationError as e: print(term_color("Some tool configurations are invalid and could not be loaded.", 'red')) - print(term_color("Please check your tool JSON files for formatting errors.", 'red')) + print(term_color(f"Validation error details: {str(e)}", 'red')) sys.exit(1) except Exception: print(term_color("An unexpected error occurred while listing tools.", 'red')) @@ -84,8 +84,9 @@ def add_tool(tool_name: Optional[str], agents=Optional[list[str]]): except ToolError: print(term_color(f"Could not add tool '{tool_name}'. Run 'agentstack tools list' to see available tools.", 'red')) sys.exit(1) - except ValidationError: + except ValidationError as e: print(term_color(f"Tool configuration is invalid. Please check the tool's JSON configuration file.", 'red')) + print(term_color(f"Validation error details: {str(e)}", 'red')) sys.exit(1) except Exception: print(term_color("An unexpected error occurred while adding the tool.", 'red')) @@ -102,8 +103,9 @@ def remove_tool(tool_name: str, agents: Optional[list[str]] = []): else: print(term_color(f"Could not remove tool '{tool_name}'. The tool may be in use or corrupted.", 'red')) sys.exit(1) - except ValidationError: + except ValidationError as e: print(term_color(f"Invalid tool configuration detected while removing '{tool_name}'.", 'red')) + print(term_color(f"Validation error details: {str(e)}", 'red')) sys.exit(1) except Exception: print(term_color("An unexpected error occurred while removing the tool.", 'red')) From 865e1d343c906203f71f3c35476003f5fecf95b2 Mon Sep 17 00:00:00 2001 From: Yj Date: Mon, 23 Dec 2024 11:48:09 +0800 Subject: [PATCH 4/5] fix validation reraising bug addresses " if i'm not missing something, ValidationError will never be surfaced to tools.py for the proper logging because we're catching it here and re-raising it as a ToolError" --- agentstack/generation/tool_generation.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/agentstack/generation/tool_generation.py b/agentstack/generation/tool_generation.py index 5ee28ad7..485ec5ba 100644 --- a/agentstack/generation/tool_generation.py +++ b/agentstack/generation/tool_generation.py @@ -96,7 +96,7 @@ def add_tool(tool_name: str, agents: Optional[list[str]] = []): with ToolsInitFile(conf.PATH / TOOLS_INIT_FILENAME) as tools_init: tools_init.add_import_for_tool(tool, agentstack_config.framework) except ValidationError as e: - raise ToolError(f"Error adding tool:\n{e}") + raise # Let ValidationError propagate up to CLI layer for proper error handling if tool.env: # add environment variables which don't exist with EnvFile() as env: @@ -144,7 +144,7 @@ def remove_tool(tool_name: str, agents: Optional[list[str]] = []): with ToolsInitFile(conf.PATH / TOOLS_INIT_FILENAME) as tools_init: tools_init.remove_import_for_tool(tool, agentstack_config.framework) except ValidationError as e: - raise ToolError(f"Error removing tool:\n{e}") + raise # Let ValidationError propagate up to CLI layer for proper error handling # Edit the framework entrypoint file to exclude the tool in the agent definition if not agents: # If no agents are specified, remove the tool from all agents From 3a8e1d3aac07a11953bc624a7ae9c3f6cea56b9b Mon Sep 17 00:00:00 2001 From: Yj Date: Mon, 23 Dec 2024 14:06:50 +0800 Subject: [PATCH 5/5] reduce redundant error message --- agentstack/cli/tools.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/agentstack/cli/tools.py b/agentstack/cli/tools.py index 86c65b20..7515769b 100644 --- a/agentstack/cli/tools.py +++ b/agentstack/cli/tools.py @@ -33,8 +33,7 @@ def list_tools(): print(term_color("Could not retrieve list of tools. The tools directory may be corrupted or missing.", 'red')) sys.exit(1) except ValidationError as e: - print(term_color("Some tool configurations are invalid and could not be loaded.", 'red')) - print(term_color(f"Validation error details: {str(e)}", 'red')) + print(term_color(f"Validation error: {str(e)}", 'red')) sys.exit(1) except Exception: print(term_color("An unexpected error occurred while listing tools.", 'red')) @@ -85,8 +84,7 @@ def add_tool(tool_name: Optional[str], agents=Optional[list[str]]): print(term_color(f"Could not add tool '{tool_name}'. Run 'agentstack tools list' to see available tools.", 'red')) sys.exit(1) except ValidationError as e: - print(term_color(f"Tool configuration is invalid. Please check the tool's JSON configuration file.", 'red')) - print(term_color(f"Validation error details: {str(e)}", 'red')) + print(term_color(f"Validation error: {str(e)}", 'red')) sys.exit(1) except Exception: print(term_color("An unexpected error occurred while adding the tool.", 'red')) @@ -104,8 +102,7 @@ def remove_tool(tool_name: str, agents: Optional[list[str]] = []): print(term_color(f"Could not remove tool '{tool_name}'. The tool may be in use or corrupted.", 'red')) sys.exit(1) except ValidationError as e: - print(term_color(f"Invalid tool configuration detected while removing '{tool_name}'.", 'red')) - print(term_color(f"Validation error details: {str(e)}", 'red')) + print(term_color(f"Validation error: {str(e)}", 'red')) sys.exit(1) except Exception: print(term_color("An unexpected error occurred while removing the tool.", 'red'))