From 68a4caff985cb9324462c1f72436eeb05b787971 Mon Sep 17 00:00:00 2001 From: Sam Partee Date: Mon, 7 Oct 2024 17:49:34 -0700 Subject: [PATCH] CLI Engine Env passing and Tool Executor cleanup (#95) This PR introduces the following changes: - **Engine Environment Configuration**: Adds support for specifying an environment variables file for the engine via the `arcade dev` CLI command. - **Configuration File Handling**: Refactors configuration file handling in the CLI launcher to generalize logic for locating configuration files. - **Tool Execution Logging**: Enhances logging in `BaseActor` to include execution duration and adjusts logging levels for better visibility. - **Enhanced Tool Exception Handling**: Improves exception handling in `ToolExecutor` and updates the `@tool` decorator to ensure proper propagation and handling of exceptions raised during tool execution. --- arcade/arcade/actor/core/base.py | 17 +++- arcade/arcade/cli/launcher.py | 89 ++++++++++------- arcade/arcade/cli/main.py | 6 +- arcade/arcade/core/errors.py | 25 +++-- arcade/arcade/core/executor.py | 24 +++-- arcade/arcade/core/output.py | 14 ++- arcade/arcade/core/schema.py | 2 + arcade/arcade/sdk/error.py | 5 + arcade/arcade/sdk/tool.py | 46 +++++++-- arcade/tests/core/test_executor.py | 154 +++++++++++++++++++++++++++++ 10 files changed, 314 insertions(+), 68 deletions(-) create mode 100644 arcade/tests/core/test_executor.py diff --git a/arcade/arcade/actor/core/base.py b/arcade/arcade/actor/core/base.py index 4e210029..523aa6f0 100644 --- a/arcade/arcade/actor/core/base.py +++ b/arcade/arcade/actor/core/base.py @@ -137,22 +137,29 @@ class BaseActor(Actor): **tool_request.inputs or {}, ) + end_time = time.time() # End time in seconds + duration_ms = (end_time - start_time) * 1000 # Convert to milliseconds + if output.error: logger.warning( f"{invocation_id} | Tool {tool_fqname} version {tool_request.tool.version} failed" ) logger.warning(f"{invocation_id} | Tool error: {output.error.message}") - logger.debug( + logger.warning( f"{invocation_id} | Tool developer message: {output.error.developer_message}" ) + logger.debug( + f"{invocation_id} | duration: {duration_ms}ms | Tool output: {output.value}" + ) + if output.error.traceback_info: + logger.debug(f"{invocation_id} | Tool traceback: {output.error.traceback_info}") else: logger.info( f"{invocation_id} | Tool {tool_fqname} version {tool_request.tool.version} success" ) - logger.debug(f"{invocation_id} | Tool output: {output}") - - end_time = time.time() # End time in seconds - duration_ms = (end_time - start_time) * 1000 # Convert to milliseconds + logger.debug( + f"{invocation_id} | duration: {duration_ms}ms | Tool output: {output.value}" + ) return ToolCallResponse( invocation_id=invocation_id, diff --git a/arcade/arcade/cli/launcher.py b/arcade/arcade/cli/launcher.py index 4c2593f2..521e628e 100644 --- a/arcade/arcade/cli/launcher.py +++ b/arcade/arcade/cli/launcher.py @@ -21,7 +21,7 @@ def start_servers( host: str, port: int, engine_config: str | None, - engine_env: dict[str, str] | None = None, + engine_env: str | None = None, debug: bool = False, ) -> None: """ @@ -31,20 +31,27 @@ def start_servers( host: Host for the actor server. port: Port for the actor server. engine_config: Path to the engine configuration file. + engine_env: Path to the engine environment file. + debug: Whether to run in debug mode. """ # Validate host and port host = _validate_host(host) port = _validate_port(port) # Ensure engine_config is provided and validated - engine_config = _get_engine_config(engine_config) + engine_config = _get_config_file(engine_config, default_filename="engine.yaml") + + # Ensure engine_env is provided or found and either way, validated + env_file = _get_config_file(engine_env, default_filename="arcade.env") # Prepare command-line arguments for the actor server and engine actor_cmd = _build_actor_command(host, port, debug) - engine_cmd = _build_engine_command(engine_config) + + # even if the user didn't pass an env file we may have found it in the default locations + engine_cmd = _build_engine_command(engine_config, engine_env=env_file if env_file else None) # Start and manage the processes - _manage_processes(actor_cmd, engine_cmd, engine_env, debug) + _manage_processes(actor_cmd, engine_cmd, debug=debug) def _validate_host(host: str) -> str: @@ -90,42 +97,44 @@ def _validate_port(port: int) -> int: return port -def _get_engine_config(engine_config: str | None) -> str: +def _get_config_file(file_path: str | None, default_filename: str = "engine.yaml") -> str: """ - Determines and validates the engine config file path. + Determines and validates the config file path. Args: - engine_config: Optional path provided by the user. + file_path: Optional path provided by the user. + default_filename: The default filename to look for. Returns: - The resolved engine config file path. + The resolved config file path. Raises: - RuntimeError: If the config file is not found or invalid. + RuntimeError: If the config file is not found. """ - if engine_config: - engine_config_path = Path(os.path.expanduser(engine_config)).resolve() - if not engine_config_path.is_file(): - console.print( - f"❌ Engine config file not found at {engine_config_path}", style="bold red" - ) - raise RuntimeError("Engine config file not found.") + if file_path: + config_path = Path(os.path.expanduser(file_path)).resolve() + if not config_path.is_file(): + console.print(f"❌ Config file not found at {config_path}", style="bold red") + raise RuntimeError(f"Config file not found at {config_path}") + return str(config_path) - elif Path(os.path.expanduser("~/.arcade/engine.yaml")).is_file(): - engine_config_path = Path(os.path.expanduser("~/.arcade/engine.yaml")) - console.print( - f"Using default engine config file at {engine_config_path}", style="bold green" - ) - else: - # Look for engine.yaml in the current directory - engine_config_path = Path(os.getcwd()) / "engine.yaml" - if not engine_config_path.is_file(): - console.print( - "❌ Engine config file not specified and not found in current directory.", - style="bold red", - ) - raise RuntimeError("Engine config file not specified.") - return str(engine_config_path) + # Look for the file in the current working directory + config_path = Path(os.getcwd()) / default_filename + if config_path.is_file(): + console.print(f"Using config file at {config_path}", style="bold green") + return str(config_path) + + # Look for the file in the user's home directory under .arcade/ + home_config_path = Path.home() / ".arcade" / default_filename + if home_config_path.is_file(): + console.print(f"Using config file at {home_config_path}", style="bold green") + return str(home_config_path) + + console.print( + f"❌ Config file '{default_filename}' not found in any of the default locations.", + style="bold red", + ) + raise RuntimeError(f"Config file '{default_filename}' not found.") def _build_actor_command(host: str, port: int, debug: bool) -> list[str]: @@ -161,12 +170,13 @@ def _build_actor_command(host: str, port: int, debug: bool) -> list[str]: return cmd -def _build_engine_command(engine_config: str) -> list[str]: +def _build_engine_command(engine_config: str, engine_env: str | None = None) -> list[str]: """ Builds the command to start the engine. Args: engine_config: Path to the engine configuration file. + engine_env: Path to the engine environment file. Returns: The command as a list. @@ -184,6 +194,10 @@ def _build_engine_command(engine_config: str) -> list[str]: "-c", engine_config, ] + if engine_env: + cmd.append("-e") + cmd.append(engine_env) + return cmd @@ -215,7 +229,7 @@ def _manage_processes( _setup_signal_handlers(terminate_processes) retry_count = 0 - max_retries = 3 # Define the maximum number of retries + max_retries = 1 # Define the maximum number of retries while retry_count <= max_retries: try: @@ -322,10 +336,15 @@ def _stream_output(process: subprocess.Popen, name: str) -> None: with pipe: for line in iter(pipe.readline, ""): line = line.rstrip() + + if "DEBUG" in line: + line = line.replace("DEBUG", "[#87CEFA]DEBUG[/#87CEFA]", 1) + if "INFO" in line: + line = line.replace("INFO", "[#109a10]INFO[/#109a10]", 1) if "WARNING" in line: - line = line.replace("WARNING", "[orange]WARNING[/orange]") + line = line.replace("WARNING", "[#FFA500]WARNING[/#FFA500]", 1) if "ERROR" in line: - line = line.replace("ERROR", "[red]ERROR[/red]") + line = line.replace("ERROR", "[#FF0000]ERROR[/#FF0000]", 1) console.print(f"[{style}]{name}>[/{style}] {line}") threading.Thread(target=stream, args=(process.stdout, stdout_style), daemon=True).start() diff --git a/arcade/arcade/cli/main.py b/arcade/arcade/cli/main.py index 6656cae9..d6b609d4 100644 --- a/arcade/arcade/cli/main.py +++ b/arcade/arcade/cli/main.py @@ -496,14 +496,16 @@ def dev( engine_config: str = typer.Option( None, "-c", "--config", help="Path to the engine configuration file." ), + env_file: str = typer.Option( + None, "-e", "--env-file", help="Path to the environment variables file." + ), debug: bool = typer.Option(False, "-d", "--debug", help="Show debug information"), ) -> None: """ Start both the actor and engine servers. """ try: - # TODO: pass Engine env vars from here - start_servers(host, port, engine_config, engine_env=None, debug=debug) + start_servers(host, port, engine_config, engine_env=env_file, debug=debug) except Exception as e: error_message = f"❌ Failed to start servers: {escape(str(e))}" console.print(error_message, style="bold red") diff --git a/arcade/arcade/core/errors.py b/arcade/arcade/core/errors.py index 5fbebea4..35ff6afc 100644 --- a/arcade/arcade/core/errors.py +++ b/arcade/arcade/core/errors.py @@ -1,3 +1,4 @@ +import traceback from typing import Optional @@ -37,19 +38,28 @@ class ToolDefinitionError(ToolError): class ToolRuntimeError(RuntimeError): - def __init__(self, message: str, developer_message: Optional[str] = None): + def __init__( + self, + message: str, + developer_message: Optional[str] = None, + ): super().__init__(message) self.message = message self.developer_message = developer_message + def traceback_info(self) -> str | None: + # return the traceback information of the parent exception + if self.__cause__: + return "\n".join(traceback.format_exception(self.__cause__)) + return None + class ToolExecutionError(ToolRuntimeError): """ Raised when there is an error executing a tool. """ - def __init__(self, message: str, developer_message: Optional[str] = None): - super().__init__(message, developer_message) + pass class RetryableToolError(ToolExecutionError): @@ -74,8 +84,7 @@ class ToolSerializationError(ToolRuntimeError): Raised when there is an error executing a tool. """ - def __init__(self, message: str, developer_message: Optional[str] = None): - super().__init__(message, developer_message) + pass class ToolInputError(ToolSerializationError): @@ -83,8 +92,7 @@ class ToolInputError(ToolSerializationError): Raised when there is an error in the input to a tool. """ - def __init__(self, message: str, developer_message: Optional[str] = None): - super().__init__(message, developer_message) + pass class ToolOutputError(ToolSerializationError): @@ -92,5 +100,4 @@ class ToolOutputError(ToolSerializationError): Raised when there is an error in the output of a tool. """ - def __init__(self, message: str, developer_message: Optional[str] = None): - super().__init__(message, developer_message) + pass diff --git a/arcade/arcade/core/executor.py b/arcade/arcade/core/executor.py index 6de7e1db..052e3944 100644 --- a/arcade/arcade/core/executor.py +++ b/arcade/arcade/core/executor.py @@ -1,4 +1,5 @@ import asyncio +import traceback from typing import Any, Callable from pydantic import BaseModel, ValidationError @@ -8,6 +9,7 @@ from arcade.core.errors import ( ToolInputError, ToolOutputError, ToolRuntimeError, + ToolSerializationError, ) from arcade.core.output import output_factory from arcade.core.schema import ToolCallOutput, ToolContext, ToolDefinition @@ -58,20 +60,24 @@ class ToolExecutor: retry_after_ms=e.retry_after_ms, ) - except ToolInputError as e: + except ToolSerializationError as e: return output_factory.fail(message=e.message, developer_message=e.developer_message) - except ToolOutputError as e: - return output_factory.fail(message=e.message, developer_message=e.developer_message) - - except ToolRuntimeError as e: # Catch any remaining tool-related errors + # should catch all tool exceptions due to the try/except in the tool decorator + except ToolRuntimeError as e: return output_factory.fail( - message=f"Error in execution: {e.message}", developer_message=e.developer_message + message=e.message, + developer_message=e.developer_message, + traceback_info=e.traceback_info(), ) # if we get here we're in trouble except Exception as e: - return output_factory.fail(message="Error in execution", developer_message=str(e)) + return output_factory.fail( + message="Error in execution", + developer_message=str(e), + traceback_info=traceback.format_exc(), + ) @staticmethod async def _serialize_input(input_model: type[BaseModel], **kwargs: Any) -> BaseModel: @@ -85,7 +91,9 @@ class ToolExecutor: inputs = input_model(**kwargs) except ValidationError as e: - raise ToolInputError(message="Error in input", developer_message=str(e)) from e + raise ToolInputError( + message="Error in tool input deserialization", developer_message=str(e) + ) from e return inputs diff --git a/arcade/arcade/core/output.py b/arcade/arcade/core/output.py index 42da8e69..5cea3f8d 100644 --- a/arcade/arcade/core/output.py +++ b/arcade/arcade/core/output.py @@ -18,10 +18,19 @@ class ToolOutputFactory: value = getattr(data, "result", "") if data else "" return ToolCallOutput(value=value) - def fail(self, *, message: str, developer_message: str | None = None) -> ToolCallOutput: + def fail( + self, + *, + message: str, + developer_message: str | None = None, + traceback_info: str | None = None, + ) -> ToolCallOutput: return ToolCallOutput( error=ToolCallError( - message=message, developer_message=developer_message, can_retry=False + message=message, + developer_message=developer_message, + can_retry=False, + traceback_info=traceback_info, ) ) @@ -32,6 +41,7 @@ class ToolOutputFactory: developer_message: str | None = None, additional_prompt_content: str | None = None, retry_after_ms: int | None = None, + traceback_info: str | None = None, ) -> ToolCallOutput: return ToolCallOutput( error=ToolCallError( diff --git a/arcade/arcade/core/schema.py b/arcade/arcade/core/schema.py index 5a873752..7fa3772c 100644 --- a/arcade/arcade/core/schema.py +++ b/arcade/arcade/core/schema.py @@ -265,6 +265,8 @@ class ToolCallError(BaseModel): """Additional content to be included in the retry prompt.""" retry_after_ms: int | None = None """The number of milliseconds (if any) to wait before retrying the tool call.""" + traceback_info: str | None = None + """The traceback information for the tool call.""" class ToolCallRequiresAuthorization(BaseModel): diff --git a/arcade/arcade/sdk/error.py b/arcade/arcade/sdk/error.py index 977075d6..dc123b18 100644 --- a/arcade/arcade/sdk/error.py +++ b/arcade/arcade/sdk/error.py @@ -1,3 +1,8 @@ +from arcade.core.errors import RetryableToolError, ToolExecutionError + +__all__ = ["SDKError", "WeightError", "ToolExecutionError", "RetryableToolError"] + + class SDKError(Exception): """Base class for all SDK errors.""" diff --git a/arcade/arcade/sdk/tool.py b/arcade/arcade/sdk/tool.py index 574a7b67..f3e6fe48 100644 --- a/arcade/arcade/sdk/tool.py +++ b/arcade/arcade/sdk/tool.py @@ -1,13 +1,14 @@ +import functools import inspect -from typing import Callable, TypeVar, Union +from typing import Any, Callable, TypeVar, Union from arcade.core.utils import snake_to_pascal_case from arcade.sdk.auth import ToolAuthorization +from arcade.sdk.error import ToolExecutionError T = TypeVar("T") -# TODO change desc to description def tool( func: Callable | None = None, desc: str | None = None, @@ -18,12 +19,43 @@ def tool( func_name = str(getattr(func, "__name__", None)) tool_name = name or snake_to_pascal_case(func_name) - setattr(func, "__tool_name__", tool_name) # noqa: B010 (Do not call `setattr` with a constant attribute value) - setattr(func, "__tool_description__", desc or inspect.cleandoc(func.__doc__ or "")) # noqa: B010 - setattr(func, "__tool_requires_auth__", requires_auth) # noqa: B010 + func.__tool_name__ = tool_name # type: ignore[attr-defined] + func.__tool_description__ = desc or inspect.cleandoc(func.__doc__ or "") # type: ignore[attr-defined] + func.__tool_requires_auth__ = requires_auth # type: ignore[attr-defined] - return func + if inspect.iscoroutinefunction(func): - if func: # This means the decorator is used without parameters + @functools.wraps(func) + async def func_with_error_handling(*args: Any, **kwargs: Any) -> Any: + try: + return await func(*args, **kwargs) + + # make sure developer raised ToolExecutionError is not + # reraised incorrectly. + except ToolExecutionError: + raise + except Exception as e: + raise ToolExecutionError( + message=f"Error in execution of {tool_name}", + developer_message=f"Error in {func_name}: {e!s}", + ) from e + + else: + + @functools.wraps(func) + def func_with_error_handling(*args: Any, **kwargs: Any) -> Any: + try: + return func(*args, **kwargs) + except ToolExecutionError: + raise + except Exception as e: + raise ToolExecutionError( + message=f"Error in execution of {tool_name}", + developer_message=f"Error in {func_name}: {e!s}", + ) from e + + return func_with_error_handling + + if func: return decorator(func) return decorator diff --git a/arcade/tests/core/test_executor.py b/arcade/tests/core/test_executor.py new file mode 100644 index 00000000..c58894d4 --- /dev/null +++ b/arcade/tests/core/test_executor.py @@ -0,0 +1,154 @@ +from typing import Annotated + +import pytest + +from arcade.core.catalog import ToolCatalog +from arcade.core.executor import ToolExecutor +from arcade.core.schema import ToolCallError, ToolCallOutput, ToolContext +from arcade.sdk import tool +from arcade.sdk.error import RetryableToolError, ToolExecutionError + + +@tool +def simple_tool(inp: Annotated[str, "input"]) -> Annotated[str, "output"]: + """Simple tool""" + return inp + + +@tool +def retryable_error_tool() -> Annotated[str, "output"]: + """Tool that raises a retryable error""" + raise RetryableToolError("test", "test", "test", 1000) + + +@tool +def exec_error_tool() -> Annotated[str, "output"]: + """Tool that raises an error""" + raise ToolExecutionError("test", "test") + + +@tool +def unexpected_error_tool() -> Annotated[str, "output"]: + """Tool that raises an unexpected error""" + raise RuntimeError("test") + + +@tool +def bad_output_error_tool() -> Annotated[str, "output"]: + """tool that returns a bad output type""" + return {"output": "test"} + + +# ---- Test Driver ---- + +catalog = ToolCatalog() +catalog.add_tool(simple_tool, "simple_toolkit") +catalog.add_tool(retryable_error_tool, "simple_toolkit") +catalog.add_tool(exec_error_tool, "simple_toolkit") +catalog.add_tool(unexpected_error_tool, "simple_toolkit") +catalog.add_tool(bad_output_error_tool, "simple_toolkit") + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + "tool_func, inputs, expected_output", + [ + (simple_tool, {"inp": "test"}, ToolCallOutput(value="test")), + ( + retryable_error_tool, + {}, + ToolCallOutput( + error=ToolCallError( + message="test", + developer_message="test", + additional_prompt_content="test", + retry_after_ms=1000, + can_retry=True, + ) + ), + ), + ( + exec_error_tool, + {}, + ToolCallOutput( + error=ToolCallError( + message="test", + developer_message="test", + ) + ), + ), + ( + unexpected_error_tool, + {}, + ToolCallOutput( + error=ToolCallError( + message="Error in execution of UnexpectedErrorTool", + developer_message="Error in unexpected_error_tool: test", + ) + ), + ), + ( + simple_tool, + {"inp": {"test": "test"}}, # takes in a string not a dict + ToolCallOutput( + error=ToolCallError( + message="Error in tool input deserialization", + developer_message=None, # can't gaurantee this will be the same + ) + ), + ), + ( + bad_output_error_tool, + {}, + ToolCallOutput( + error=ToolCallError( + message="Failed to serialize tool output", + developer_message=None, # can't gaurantee this will be the same + ) + ), + ), + ], + ids=[ + "simple_tool", + "retryable_error_tool", + "exec_error_tool", + "unexpected_error_tool", + "invalid_input_type", + "bad_output_type", + ], +) +async def test_tool_executor(tool_func, inputs, expected_output): + tool_definition = catalog.find_tool_by_func(tool_func) + + dummy_context = ToolContext() + full_tool = catalog.get_tool(tool_definition.get_fully_qualified_name()) + output = await ToolExecutor.run( + func=tool_func, + definition=tool_definition, + input_model=full_tool.input_model, + output_model=full_tool.output_model, + context=dummy_context, + **inputs, + ) + + check_output(output, expected_output) + + +def check_output(output: ToolCallOutput, expected_output: ToolCallOutput): + # execution error in tool + if output.error: + assert output.error.message == expected_output.error.message + if expected_output.error.developer_message: + assert output.error.developer_message == expected_output.error.developer_message + if expected_output.error.traceback_info: + assert output.error.traceback_info == expected_output.error.traceback_info + assert output.error.can_retry == expected_output.error.can_retry + assert ( + output.error.additional_prompt_content + == expected_output.error.additional_prompt_content + ) + assert output.error.retry_after_ms == expected_output.error.retry_after_ms + + # normal tool execution + else: + assert output.value == expected_output.value