From 64944b2998937b0cf535fc863ecdc03b9c29c409 Mon Sep 17 00:00:00 2001 From: Bin Wang Date: Tue, 19 Sep 2023 11:39:14 +0800 Subject: [PATCH] [Feature] Improve error handling for loading tool error. --- src/promptflow/promptflow/_core/_errors.py | 9 +- .../promptflow/_core/tools_manager.py | 8 +- .../promptflow/_utils/exception_utils.py | 106 +++++++++++------- src/promptflow/promptflow/executor/_errors.py | 48 +++++++- .../promptflow/executor/_tool_resolver.py | 56 ++++----- .../e2etests/test_executor_validation.py | 51 ++++++--- .../unittests/_utils/test_exception_utils.py | 23 ++++ .../unittests/executor/test_exceptions.py | 63 +++++++++++ 8 files changed, 275 insertions(+), 89 deletions(-) create mode 100644 src/promptflow/tests/executor/unittests/executor/test_exceptions.py diff --git a/src/promptflow/promptflow/_core/_errors.py b/src/promptflow/promptflow/_core/_errors.py index d191b5c2473..edcf5406244 100644 --- a/src/promptflow/promptflow/_core/_errors.py +++ b/src/promptflow/promptflow/_core/_errors.py @@ -24,12 +24,15 @@ class PackageToolNotFoundError(ValidationException): pass -class LoadToolError(ValidationException): +class MissingRequiredInputs(ValidationException): pass -class MissingRequiredInputs(LoadToolError): - pass +class ToolLoadError(UserErrorException): + """Exception raised when tool load failed.""" + + def __init__(self, *, module: str = None): + super().__init__(target=ErrorTarget.TOOL, module=module) class ToolExecutionError(UserErrorException): diff --git a/src/promptflow/promptflow/_core/tools_manager.py b/src/promptflow/promptflow/_core/tools_manager.py index 72f65bcacd7..d02e15c8c46 100644 --- a/src/promptflow/promptflow/_core/tools_manager.py +++ b/src/promptflow/promptflow/_core/tools_manager.py @@ -13,7 +13,7 @@ import pkg_resources import yaml -from promptflow._core._errors import MissingRequiredInputs, NotSupported, PackageToolNotFoundError +from promptflow._core._errors import MissingRequiredInputs, NotSupported, PackageToolNotFoundError, ToolLoadError from promptflow._core.tool_meta_generator import ( _parse_tool_from_function, collect_tool_function_in_module, @@ -161,7 +161,11 @@ def _load_package_tool(tool_name, module_name, class_name, method_name, node_inp target=ErrorTarget.EXECUTOR, ) # Return the init_inputs to update node inputs in the afterward steps - return getattr(provider_class(**init_inputs_values), method_name), init_inputs + try: + api = getattr(provider_class(**init_inputs_values), method_name) + except Exception as ex: + raise ToolLoadError(target=ErrorTarget.TOOL, module=module_name) from ex + return api, init_inputs @staticmethod def load_tool_by_api_name(api_name: str) -> Tool: diff --git a/src/promptflow/promptflow/_utils/exception_utils.py b/src/promptflow/promptflow/_utils/exception_utils.py index c2e8434d79c..0162ade20d6 100644 --- a/src/promptflow/promptflow/_utils/exception_utils.py +++ b/src/promptflow/promptflow/_utils/exception_utils.py @@ -198,55 +198,44 @@ def build_debug_info(self, ex: Exception): "innerException": inner_exception, } - def to_dict(self, *, include_debug_info=False): - """Return a dict representation of the exception. - - This dict specification corresponds to the specification of the Microsoft API Guidelines: - https://github.com/microsoft/api-guidelines/blob/vNext/Guidelines.md#7102-error-condition-responses + @property + def error_codes(self): + """The hierarchy of the error codes. - Note that this dict representation the "error" field in the response body of the API. - The whole error response is then populated in another place outside of this class. + For general exceptions, the hierarchy should be: + ["SystemError", {exception type name}] """ - if isinstance(self._ex, JsonSerializedPromptflowException): - return self._ex.to_dict(include_debug_info=include_debug_info) - - # Otherwise, return general dict representation of the exception. - result = { - "code": infer_error_code_from_class(SystemErrorException), - "message": str(self._ex), - "messageFormat": "", - "messageParameters": {}, - "innerError": { - "code": self._ex.__class__.__name__, - "innerError": None, - }, - } - if include_debug_info: - result["debugInfo"] = self.debug_info - - return result - + error_codes: list[str] = [infer_error_code_from_class(SystemErrorException), self._ex.__class__.__name__] + return error_codes -class PromptflowExceptionPresenter(ExceptionPresenter): @property def error_code_recursed(self): """Returns a dict of the error codes for this exception. It is populated in a recursive manner, using the source from `error_codes` property. - i.e. For ToolExcutionError which inherits from UserErrorException, + i.e. For PromptflowException, such as ToolExcutionError which inherits from UserErrorException, The result would be: { - "code": "UserErrorException", + "code": "UserError", "innerError": { "code": "ToolExecutionError", "innerError": None, }, } + For other exception types, such as ValueError, the result would be: + + { + "code": "SystemError", + "innerError": { + "code": "ValueError", + "innerError": None, + }, + } """ current_error = None - reversed_error_codes = reversed(self._ex.error_codes) if self._ex.error_codes else [] + reversed_error_codes = reversed(self.error_codes) if self.error_codes else [] for code in reversed_error_codes: current_error = { "code": code, @@ -255,6 +244,52 @@ def error_code_recursed(self): return current_error + def to_dict(self, *, include_debug_info=False): + """Return a dict representation of the exception. + + This dict specification corresponds to the specification of the Microsoft API Guidelines: + https://github.com/microsoft/api-guidelines/blob/vNext/Guidelines.md#7102-error-condition-responses + + Note that this dict representation the "error" field in the response body of the API. + The whole error response is then populated in another place outside of this class. + """ + if isinstance(self._ex, JsonSerializedPromptflowException): + return self._ex.to_dict(include_debug_info=include_debug_info) + + # Otherwise, return general dict representation of the exception. + result = {"message": str(self._ex), "messageFormat": "", "messageParameters": {}} + if self.error_code_recursed: + result.update(self.error_code_recursed) + else: + result["code"] = infer_error_code_from_class(SystemErrorException) + result["innerError"] = None + if include_debug_info: + result["debugInfo"] = self.debug_info + + return result + + +class PromptflowExceptionPresenter(ExceptionPresenter): + @property + def error_codes(self): + """The hierarchy of the error codes. + + For subclass of PromptflowException, use the ex.error_codes directly. + + For PromptflowException (not a subclass), the ex.error_code is None. + The result should be: + ["SystemError", {inner_exception type name if exist}] + """ + if self._ex.error_codes: + return self._ex.error_codes + + # For PromptflowException (not a subclass), the ex.error_code is None. + # Handle this case specifically. + error_codes: list[str] = [infer_error_code_from_class(SystemErrorException)] + if self._ex.inner_exception: + error_codes.append(infer_error_code_from_class(self._ex.inner_exception.__class__)) + return error_codes + def to_dict(self, *, include_debug_info=False): result = { "message": self._ex.message, @@ -266,17 +301,8 @@ def to_dict(self, *, include_debug_info=False): if self.error_code_recursed: result.update(self.error_code_recursed) else: - # For PromptflowException (not a subclass), the error_code_recursed is None. - # Handle this case specifically. result["code"] = infer_error_code_from_class(SystemErrorException) - if self._ex.inner_exception: - # Set the type of inner_exception as the inner error - result["innerError"] = { - "code": infer_error_code_from_class(self._ex.inner_exception.__class__), - "innerError": None, - } - else: - result["innerError"] = None + result["innerError"] = None if self._ex.additional_info: result["additionalInfo"] = [{"type": k, "info": v} for k, v in self._ex.additional_info.items()] if include_debug_info: diff --git a/src/promptflow/promptflow/executor/_errors.py b/src/promptflow/promptflow/executor/_errors.py index 8fef87f2354..df149d54f16 100644 --- a/src/promptflow/promptflow/executor/_errors.py +++ b/src/promptflow/promptflow/executor/_errors.py @@ -2,7 +2,14 @@ # Copyright (c) Microsoft Corporation. All rights reserved. # --------------------------------------------------------- -from promptflow.exceptions import ErrorTarget, SystemErrorException, UserErrorException, ValidationException +from promptflow._utils.exception_utils import ExceptionPresenter, RootErrorCode +from promptflow.exceptions import ( + ErrorTarget, + PromptflowException, + SystemErrorException, + UserErrorException, + ValidationException, +) class InvalidCustomLLMTool(ValidationException): @@ -172,3 +179,42 @@ def __init__(self, line_number, timeout): super().__init__( message=f"Line {line_number} execution timeout for exceeding {timeout} seconds", target=ErrorTarget.EXECUTOR ) + + +class ResolveToolError(PromptflowException): + """Exception raised when tool load failed.""" + + def __init__(self, *, node_name: str, target: ErrorTarget = ErrorTarget.EXECUTOR, module: str = None): + self._node_name = node_name + super().__init__(target=target, module=module) + + @property + def message_format(self): + if self.inner_exception: + return "Tool load failed in '{node_name}': {error_type_and_message}" + else: + return "Tool load failed in '{node_name}'." + + @property + def message_parameters(self): + error_type_and_message = None + if self.inner_exception: + error_type_and_message = f"({self.inner_exception.__class__.__name__}) {self.inner_exception}" + + return { + "node_name": self._node_name, + "error_type_and_message": error_type_and_message, + } + + @property + def additional_info(self): + """Get additional info from innererror when the innererror is PromptflowException""" + if isinstance(self.inner_exception, PromptflowException): + return self.inner_exception.additional_info + return None + + @property + def error_codes(self): + if self.inner_exception: + return ExceptionPresenter.create(self.inner_exception).error_codes + return [RootErrorCode.SYSTEM_ERROR] diff --git a/src/promptflow/promptflow/executor/_tool_resolver.py b/src/promptflow/promptflow/executor/_tool_resolver.py index 8490f9161a6..e31298b547d 100644 --- a/src/promptflow/promptflow/executor/_tool_resolver.py +++ b/src/promptflow/promptflow/executor/_tool_resolver.py @@ -10,22 +10,19 @@ from typing import Callable, List, Optional from promptflow._core.connection_manager import ConnectionManager -from promptflow._core.tools_manager import ( - BuiltinsManager, - ToolLoader, - connection_type_to_api_mapping, -) +from promptflow._core.tools_manager import BuiltinsManager, ToolLoader, connection_type_to_api_mapping from promptflow._utils.tool_utils import get_inputs_for_prompt_template, get_prompt_param_name_from_func from promptflow.contracts.flow import InputAssignment, InputValueType, Node, ToolSourceType from promptflow.contracts.tool import ConnectionType, Tool, ToolType, ValueType from promptflow.contracts.types import PromptTemplate -from promptflow.exceptions import ErrorTarget, UserErrorException +from promptflow.exceptions import ErrorTarget, PromptflowException, UserErrorException from promptflow.executor._errors import ( ConnectionNotFound, InvalidConnectionType, InvalidCustomLLMTool, InvalidSource, NodeInputValidationError, + ResolveToolError, ValueTypeUnresolved, ) @@ -97,26 +94,33 @@ def _convert_node_literal_input_types(self, node: Node, tool: Tool): return updated_node def resolve_tool_by_node(self, node: Node, convert_input_types=True) -> ResolvedTool: - if node.source is None: - raise UserErrorException(f"Node {node.name} does not have source defined.") - - if node.type is ToolType.PYTHON: - if node.source.type == ToolSourceType.Package: - return self._resolve_package_node(node, convert_input_types=convert_input_types) - elif node.source.type == ToolSourceType.Code: - return self._resolve_script_node(node, convert_input_types=convert_input_types) - raise NotImplementedError(f"Tool source type {node.source.type} for python tool is not supported yet.") - elif node.type is ToolType.PROMPT: - return self._resolve_prompt_node(node) - elif node.type is ToolType.LLM: - return self._resolve_llm_node(node, convert_input_types=convert_input_types) - elif node.type is ToolType.CUSTOM_LLM: - if node.source.type == ToolSourceType.PackageWithPrompt: - resolved_tool = self._resolve_package_node(node, convert_input_types=convert_input_types) - return self._integrate_prompt_in_package_node(node, resolved_tool) - raise NotImplementedError(f"Tool source type {node.source.type} for custom_llm tool is not supported yet.") - else: - raise NotImplementedError(f"Tool type {node.type} is not supported yet.") + try: + if node.source is None: + raise UserErrorException(f"Node {node.name} does not have source defined.") + + if node.type is ToolType.PYTHON: + if node.source.type == ToolSourceType.Package: + return self._resolve_package_node(node, convert_input_types=convert_input_types) + elif node.source.type == ToolSourceType.Code: + return self._resolve_script_node(node, convert_input_types=convert_input_types) + raise NotImplementedError(f"Tool source type {node.source.type} for python tool is not supported yet.") + elif node.type is ToolType.PROMPT: + return self._resolve_prompt_node(node) + elif node.type is ToolType.LLM: + return self._resolve_llm_node(node, convert_input_types=convert_input_types) + elif node.type is ToolType.CUSTOM_LLM: + if node.source.type == ToolSourceType.PackageWithPrompt: + resolved_tool = self._resolve_package_node(node, convert_input_types=convert_input_types) + return self._integrate_prompt_in_package_node(node, resolved_tool) + raise NotImplementedError( + f"Tool source type {node.source.type} for custom_llm tool is not supported yet." + ) + else: + raise NotImplementedError(f"Tool type {node.type} is not supported yet.") + except Exception as e: + if isinstance(e, PromptflowException) and e.target != ErrorTarget.UNKNOWN: + raise ResolveToolError(node_name=node.name, target=e.target, module=e.module) from e + raise ResolveToolError(node_name=node.name) from e def _load_source_content(self, node: Node) -> str: source = node.source diff --git a/src/promptflow/tests/executor/e2etests/test_executor_validation.py b/src/promptflow/tests/executor/e2etests/test_executor_validation.py index f0077f72f97..f90aa193e16 100644 --- a/src/promptflow/tests/executor/e2etests/test_executor_validation.py +++ b/src/promptflow/tests/executor/e2etests/test_executor_validation.py @@ -22,6 +22,7 @@ NodeInputValidationError, NodeReferenceNotFound, OutputReferenceNotFound, + ResolveToolError, ) from promptflow.executor.flow_executor import BulkResult @@ -32,12 +33,13 @@ @pytest.mark.e2etest class TestValidation: @pytest.mark.parametrize( - "flow_folder, yml_file, error_class, error_msg", + "flow_folder, yml_file, error_class, inner_class, error_msg", [ ( "nodes_names_duplicated", "flow.dag.yaml", DuplicateNodeName, + None, ( "Invalid node definitions found in the flow graph. Node with name 'stringify_num' appears more " "than once in the node definitions in your flow, which is not allowed. To " @@ -48,16 +50,19 @@ class TestValidation: ( "source_file_missing", "flow.dag.jinja.yaml", + ResolveToolError, InvalidSource, ( - "Node source path 'summarize_text_content__variant_1.jinja2' is invalid on " - "node 'summarize_text_content'." + "Tool load failed in 'summarize_text_content': (InvalidSource) " + "Node source path 'summarize_text_content__variant_1.jinja2' is invalid on node " + "'summarize_text_content'." ), ), ( "node_reference_not_found", "flow.dag.yaml", NodeReferenceNotFound, + None, ( "Invalid node definitions found in the flow graph. Node 'divide_num_2' references a non-existent " "node 'divide_num_3' in your flow. Please review your flow to ensure that the " @@ -68,6 +73,7 @@ class TestValidation: "node_circular_dependency", "flow.dag.yaml", NodeCircularDependency, + None, ( "Invalid node definitions found in the flow graph. Node circular dependency has been detected " "among the nodes in your flow. Kindly review the reference relationships for " @@ -79,6 +85,7 @@ class TestValidation: "flow_input_reference_invalid", "flow.dag.yaml", InputReferenceNotFound, + None, ( "Invalid node definitions found in the flow graph. Node 'divide_num' references flow input 'num_1' " "which is not defined in your flow. To resolve this issue, please review your " @@ -90,6 +97,7 @@ class TestValidation: "flow_output_reference_invalid", "flow.dag.yaml", EmptyOutputReference, + None, ( "The output 'content' for flow is incorrect. The reference is not specified for the output " "'content' in the flow. To rectify this, ensure that you accurately specify " @@ -100,6 +108,7 @@ class TestValidation: "outputs_reference_not_valid", "flow.dag.yaml", OutputReferenceNotFound, + None, ( "The output 'content' for flow is incorrect. The output 'content' references non-existent " "node 'another_stringify_num' in your flow. To resolve this issue, please " @@ -111,6 +120,7 @@ class TestValidation: "outputs_with_invalid_flow_inputs_ref", "flow.dag.yaml", OutputReferenceNotFound, + None, ( "The output 'num' for flow is incorrect. The output 'num' references non-existent flow " "input 'num11' in your flow. Please carefully review your flow and correct " @@ -120,21 +130,25 @@ class TestValidation: ], ) def test_executor_create_failure_type_and_message( - self, flow_folder, yml_file, error_class, error_msg, dev_connections + self, flow_folder, yml_file, error_class, error_msg, inner_class, dev_connections ): with pytest.raises(error_class) as exc_info: FlowExecutor.create(get_yaml_file(flow_folder, WRONG_FLOW_ROOT, yml_file), dev_connections) + if isinstance(exc_info.value, ResolveToolError): + assert isinstance(exc_info.value.inner_exception, inner_class) assert error_msg == exc_info.value.message @pytest.mark.parametrize( - "flow_folder, yml_file, error_class", + "flow_folder, yml_file, error_class, inner_class", [ - ("source_file_missing", "flow.dag.python.yaml", PythonParsingError), + ("source_file_missing", "flow.dag.python.yaml", ResolveToolError, PythonParsingError), ], ) - def test_executor_create_failure_type(self, flow_folder, yml_file, error_class, dev_connections): - with pytest.raises(error_class): + def test_executor_create_failure_type(self, flow_folder, yml_file, error_class, inner_class, dev_connections): + with pytest.raises(error_class) as e: FlowExecutor.create(get_yaml_file(flow_folder, WRONG_FLOW_ROOT, yml_file), dev_connections) + if isinstance(e.value, ResolveToolError): + assert isinstance(e.value.inner_exception, inner_class) @pytest.mark.parametrize( "ordered_flow_folder, unordered_flow_folder", @@ -150,18 +164,20 @@ def test_node_topology_in_order(self, ordered_flow_folder, unordered_flow_folder assert node1.name == node2.name @pytest.mark.parametrize( - "flow_folder, error_class", + "flow_folder, error_class, inner_class", [ - ("invalid_connection", ConnectionNotFound), - ("tool_type_missing", NotImplementedError), - ("wrong_module", FailedToImportModule), - ("wrong_api", APINotFound), - ("wrong_provider", APINotFound), + ("invalid_connection", ResolveToolError, ConnectionNotFound), + ("tool_type_missing", ResolveToolError, NotImplementedError), + ("wrong_module", FailedToImportModule, None), + ("wrong_api", ResolveToolError, APINotFound), + ("wrong_provider", ResolveToolError, APINotFound), ], ) - def test_invalid_flow_dag(self, flow_folder, error_class, dev_connections): - with pytest.raises(error_class): + def test_invalid_flow_dag(self, flow_folder, error_class, inner_class, dev_connections): + with pytest.raises(error_class) as e: FlowExecutor.create(get_yaml_file(flow_folder, WRONG_FLOW_ROOT), dev_connections) + if isinstance(e.value, ResolveToolError): + assert isinstance(e.value.inner_exception, inner_class) @pytest.mark.parametrize( "flow_folder, line_input, error_class", @@ -315,8 +331,9 @@ def test_single_node_input_type_invalid( ], ) def test_flow_run_with_duplicated_inputs(self, flow_folder, msg, dev_connections): - with pytest.raises(NodeInputValidationError, match=msg): + with pytest.raises(ResolveToolError, match=msg) as e: FlowExecutor.create(get_yaml_file(flow_folder, FLOW_ROOT), dev_connections) + assert isinstance(e.value.inner_exception, NodeInputValidationError) @pytest.mark.parametrize( "flow_folder, batch_input, raise_on_line_failure, error_class", diff --git a/src/promptflow/tests/executor/unittests/_utils/test_exception_utils.py b/src/promptflow/tests/executor/unittests/_utils/test_exception_utils.py index 593dd87525d..09b038cf7d5 100644 --- a/src/promptflow/tests/executor/unittests/_utils/test_exception_utils.py +++ b/src/promptflow/tests/executor/unittests/_utils/test_exception_utils.py @@ -229,6 +229,29 @@ def test_to_dict_for_tool_execution_error(self): }, } + def test_error_codes_for_general_exception(self): + with pytest.raises(CustomizedException) as e: + raise_general_exception() + + presenter = ExceptionPresenter.create(e.value) + assert presenter.error_codes == ["SystemError", "CustomizedException"] + + def test_error_codes_romptflow_exception(self): + with pytest.raises(ToolExecutionError) as e: + raise_tool_execution_error() + presenter = ExceptionPresenter.create(e.value) + assert presenter.error_codes == ["UserError", "ToolExecutionError"] + + with pytest.raises(PromptflowException) as e: + raise_promptflow_exception() + presenter = ExceptionPresenter.create(e.value) + assert presenter.error_codes == ["SystemError", "ZeroDivisionError"] + + with pytest.raises(PromptflowException) as e: + raise_promptflow_exception_without_inner_exception() + presenter = ExceptionPresenter.create(e.value) + assert presenter.error_codes == ["SystemError"] + @pytest.mark.unittest class TestErrorResponse: diff --git a/src/promptflow/tests/executor/unittests/executor/test_exceptions.py b/src/promptflow/tests/executor/unittests/executor/test_exceptions.py new file mode 100644 index 00000000000..148b0b7faa7 --- /dev/null +++ b/src/promptflow/tests/executor/unittests/executor/test_exceptions.py @@ -0,0 +1,63 @@ +import pytest + +from promptflow._core.tool_meta_generator import PythonLoadError +from promptflow.exceptions import ErrorTarget +from promptflow.executor._errors import ResolveToolError + + +def code_with_bug(): + 1 / 0 + + +def raise_resolve_tool_error(func, target=None, module=None): + try: + func() + except Exception as e: + if target: + raise ResolveToolError(node_name="MyTool", target=target, module=module) from e + raise ResolveToolError(node_name="MyTool") from e + + +def raise_python_load_error(): + try: + code_with_bug() + except Exception as e: + raise PythonLoadError(message="Test PythonLoadError.") from e + + +def test_resolve_tool_error(): + with pytest.raises(ResolveToolError) as e: + raise_resolve_tool_error(raise_python_load_error, ErrorTarget.TOOL, "__pf_main__") + + exception = e.value + inner_exception = exception.inner_exception + + assert isinstance(inner_exception, PythonLoadError) + assert exception.message == "Tool load failed in 'MyTool': (PythonLoadError) Test PythonLoadError." + assert exception.additional_info == inner_exception.additional_info + assert exception.error_codes == ["UserError", "ToolValidationError", "PythonParsingError", "PythonLoadError"] + assert exception.reference_code == "Tool/__pf_main__" + + +def test_resolve_tool_error_with_none_inner(): + with pytest.raises(ResolveToolError) as e: + raise ResolveToolError(node_name="MyTool") + + exception = e.value + assert exception.inner_exception is None + assert exception.message == "Tool load failed in 'MyTool'." + assert exception.additional_info is None + assert exception.error_codes == ["SystemError"] + assert exception.reference_code == "Executor" + + +def test_resolve_tool_error_with_no_PromptflowException_inner(): + with pytest.raises(ResolveToolError) as e: + raise_resolve_tool_error(code_with_bug) + + exception = e.value + assert isinstance(exception.inner_exception, ZeroDivisionError) + assert exception.message == "Tool load failed in 'MyTool': (ZeroDivisionError) division by zero" + assert exception.additional_info is None + assert exception.error_codes == ["SystemError", "ZeroDivisionError"] + assert exception.reference_code == "Executor"