feat: Address pr feedback

Author: Unshure

src/strands/tools/loader.py

@@ -55,7 +55,7 @@ def load_tools_from_file_path(tool_path: str) -> List[AgentTool]:
     # ./path/to/my_cool_tool.py -> my_cool_tool
     module_name = os.path.basename(tool_path).split(".")[0]
 
-    # This function import a module based on its path, and gives it the provided name
+    # This function imports a module based on its path, and gives it the provided name
 
     spec: ModuleSpec = cast(ModuleSpec, importlib.util.spec_from_file_location(module_name, abs_path))
     if not spec:
@@ -72,31 +72,31 @@ def load_tools_from_file_path(tool_path: str) -> List[AgentTool]:
     return load_tools_from_module(module, module_name)
 
 
-def load_tools_from_module_path(module_path: str) -> list[AgentTool]:
+def load_tools_from_module_path(module_tool_path: str) -> list[AgentTool]:
     """Load strands tool from a module path.
 
     Example module paths:
     my.module.path
     my.module.path:tool_name
     """
-    if ":" in module_path:
-        module_name, tool_func_name = module_path.split(":")
+    if ":" in module_tool_path:
+        module_path, tool_func_name = module_tool_path.split(":")
     else:
-        module_name, tool_func_name = (module_path, None)
+        module_path, tool_func_name = (module_tool_path, None)
 
     try:
-        module = importlib.import_module(module_name)
+        module = importlib.import_module(module_path)
     except ModuleNotFoundError as e:
-        raise AttributeError(f'Tool string: "{module_path}" is not a valid tool string.') from e
+        raise AttributeError(f'Tool string: "{module_tool_path}" is not a valid tool string.') from e
 
     # If a ':' is present in the string, then its a targeted function in a module
     if tool_func_name:
-        if tool_func_name in dir(module):
+        if hasattr(module, tool_func_name):
             target_tool = getattr(module, tool_func_name)
             if isinstance(target_tool, DecoratedFunctionTool):
                 return [target_tool]
 
-            raise AttributeError(f"Tool {tool_func_name} not found in module {module_name}")
+        raise AttributeError(f"Tool {tool_func_name} not found in module {module_path}")
 
     # Else, try to import all of the @tool decorated tools, or the module based tool
     module_name = module_path.split(".")[-1]

src/strands/tools/registry.py

@@ -75,15 +75,6 @@ def add_tool(tool: Any) -> None:
                         self.register_tool(a_tool)
                         tool_names.append(a_tool.tool_name)
 
-                # Dictionary with path only
-                elif isinstance(tool, dict) and "path" in tool:
-                    tools = load_tool_from_string(tool["path"])
-
-                    for a_tool in tools:
-                        a_tool.mark_dynamic()
-                        self.register_tool(a_tool)
-                        tool_names.append(a_tool.tool_name)
-
                 # Dictionary with name and path
                 elif isinstance(tool, dict) and "name" in tool and "path" in tool:
                     tools = load_tool_from_string(tool["path"])
@@ -97,7 +88,16 @@ def add_tool(tool: Any) -> None:
                             tool_found = True
 
                     if not tool_found:
-                        raise ValueError(f"Failed to load tool {tool}")
+                        raise ValueError(f'Tool "{tool["name"]}" not found in "{tool["path"]}"')
+
+                # Dictionary with path only
+                elif isinstance(tool, dict) and "path" in tool:
+                    tools = load_tool_from_string(tool["path"])
+
+                    for a_tool in tools:
+                        a_tool.mark_dynamic()
+                        self.register_tool(a_tool)
+                        tool_names.append(a_tool.tool_name)
 
                 # Imported Python module
                 elif hasattr(tool, "__file__") and inspect.ismodule(tool):
@@ -131,7 +131,7 @@ def add_tool(tool: Any) -> None:
         return tool_names
 
     def load_tool_from_filepath(self, tool_name: str, tool_path: str) -> None:
-        """Load a tool from a file path.
+        """DEPRECATED: Load a tool from a file path.
 
         Args:
             tool_name: Name of the tool.

tests/fixtures/say_tool.py

@@ -11,3 +11,7 @@ def say(input: str) -> str:
 def dont_say(input: str) -> str:
     """Dont say something."""
     return "Didnt say anything!"
+
+
+def not_a_tool() -> str:
+    return "Not a tool!"

tests/fixtures/tool_with_spec_but_no_function.py

@@ -0,0 +1 @@
+TOOL_SPEC = {"hello": "world!"}

tests/fixtures/tool_with_spec_but_non_callable_function.py

@@ -0,0 +1,3 @@
+TOOL_SPEC = {"hello": "world"}
+
+tool_with_spec_but_non_callable_function = "not a function!"

tests/strands/tools/test_loader.py

@@ -1,11 +1,12 @@
 import os
 import re
+import tempfile
 import textwrap
 
 import pytest
 
 from strands.tools.decorator import DecoratedFunctionTool
-from strands.tools.loader import ToolLoader
+from strands.tools.loader import ToolLoader, load_tools_from_file_path
 from strands.tools.tools import PythonAgentTool
 
 
@@ -310,3 +311,9 @@ def test_load_tool_path_returns_single_tool(tool_path):
 
     assert loaded_python_tool.tool_name == "alpha"
     assert loaded_tool.tool_name == "alpha"
+
+
+def test_load_tools_from_file_path_module_spec_missing():
+    with tempfile.NamedTemporaryFile() as f:
+        with pytest.raises(ImportError, match=f"Could not create spec for {os.path.basename(f.name)}"):
+            load_tools_from_file_path(f.name)

tests/strands/tools/test_registry.py

@@ -185,3 +185,78 @@ def test_register_strands_tools_specific_tool_from_module():
     assert len(tool_registry.registry) == 1
     assert "say" in tool_registry.registry
     assert "dont_say" not in tool_registry.registry
+
+
+def test_register_strands_tools_specific_tool_from_module_tool_missing():
+    tool_registry = ToolRegistry()
+
+    with pytest.raises(ValueError, match="Failed to load tool tests.fixtures.say_tool:nay: "):
+        tool_registry.process_tools(["tests.fixtures.say_tool:nay"])
+
+
+def test_register_strands_tools_specific_tool_from_module_not_a_tool():
+    tool_registry = ToolRegistry()
+
+    with pytest.raises(ValueError, match="Failed to load tool tests.fixtures.say_tool:not_a_tool: "):
+        tool_registry.process_tools(["tests.fixtures.say_tool:not_a_tool"])
+
+
+def test_register_strands_tools_with_dict():
+    tool_registry = ToolRegistry()
+    tool_registry.process_tools([{"path": "tests.fixtures.say_tool"}])
+
+    assert len(tool_registry.registry) == 2
+    assert "say" in tool_registry.registry
+    assert "dont_say" in tool_registry.registry
+
+
+def test_register_strands_tools_specific_tool_with_dict():
+    tool_registry = ToolRegistry()
+    tool_registry.process_tools([{"path": "tests.fixtures.say_tool", "name": "say"}])
+
+    assert len(tool_registry.registry) == 1
+    assert "say" in tool_registry.registry
+
+
+def test_register_strands_tools_specific_tool_with_dict_not_found():
+    tool_registry = ToolRegistry()
+
+    with pytest.raises(
+        ValueError,
+        match="Failed to load tool {'path': 'tests.fixtures.say_tool'"
+        ", 'name': 'nay'}: Tool \"nay\" not found in \"tests.fixtures.say_tool\"",
+    ):
+        tool_registry.process_tools([{"path": "tests.fixtures.say_tool", "name": "nay"}])
+
+
+def test_register_strands_tools_module_no_spec():
+    tool_registry = ToolRegistry()
+
+    with pytest.raises(
+        ValueError,
+        match="Failed to load tool tests.fixtures.mocked_model_provider: "
+        "The module mocked_model_provider is not a valid module",
+    ):
+        tool_registry.process_tools(["tests.fixtures.mocked_model_provider"])
+
+
+def test_register_strands_tools_module_no_function():
+    tool_registry = ToolRegistry()
+
+    with pytest.raises(
+        ValueError,
+        match="Failed to load tool tests.fixtures.tool_with_spec_but_no_function: "
+        "Module-based tool tool_with_spec_but_no_function missing function tool_with_spec_but_no_function",
+    ):
+        tool_registry.process_tools(["tests.fixtures.tool_with_spec_but_no_function"])
+
+
+def test_register_strands_tools_module_non_callable_function():
+    tool_registry = ToolRegistry()
+
+    with pytest.raises(
+        ValueError,
+        match="Failed to load tool tests.fixtures.tool_with_spec_but_non_callable_function:"
+        " Tool tool_with_spec_but_non_callable_function function is not callable",
+    ):
+        tool_registry.process_tools(["tests.fixtures.tool_with_spec_but_non_callable_function"])