Python: Enforcing return typehints on native functions (#2948)

### Motivation and Context

<!-- Thank you for your contribution to the semantic-kernel repo!
Please help reviewers and future users, providing the following
information:
  1. Why is this change required?
  2. What problem does it solve?
  3. What scenario does it contribute to?
  4. If it fixes an open issue, please link to the issue here.
-->
This solves [this
issue](https://github.com/microsoft/semantic-kernel/issues/1069) where
native functions are not returning the expected result. In python,
return typehints are optional. However, the semantic kernel requires a
return typehint to determine how to handle the function. For that
reason, we're now enforcing return typehints and raising an exception
when one is not provided. This may break existing void functions but
none in our repo are affected.

Fixes #1069 
### Description

<!-- Describe your changes, the overall approach, the underlying design.
These notes will help understanding how your code works. Thanks! -->

### Contribution Checklist

<!-- Before submitting this PR, please make sure: -->

- [x] The code builds clean without any errors or warnings
- [x] The PR follows the [SK Contribution
Guidelines](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md)
and the [pre-submission formatting
script](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md#development-scripts)
raises no violations
- [x] All unit tests pass, and I have added new tests where possible
- [] I didn't break anyone 😄

## Summary
This pull request includes two sets of changes. First, it fixes the
return type annotations in three core skills: file_io_skill,
text_memory_skill, and wait_skill. Specifically, the write_async method
in file_io_skill and the save_async method in text_memory_skill now have
a return type of None, and the wait method in wait_skill now has a
return type of None. Additionally, the
import_native_skill_from_directory method in kernel.py has been
refactored to simplify the code. Second, this pull request adds error
handling for functions with no return type in the delegate inference
module. Previously, if a function did not specify a return type, the
DelegateType could not be inferred, leading to errors. This pull request
adds a check for functions with no return type and raises a
KernelException with an appropriate error message. Additionally, this
pull request includes some minor changes to unit tests for native
functions.

## Changes
- Fixed return type annotation for write_async method in file_io_skill
- Fixed return type annotation for save_async method in
text_memory_skill
- Fixed return type annotation for wait method in wait_skill
- Refactored import_native_skill_from_directory method in kernel.py
- Added error handling for functions with no return type in the delegate
inference module
- Added a check for functions with no return type and raised a
KernelException with an appropriate error message
- Made minor changes to unit tests for native functions

---
*Powered by [Microsoft Semantic
Kernel](https://github.com/microsoft/semantic-kernel)*

python/semantic_kernel/core_skills/file_io_skill.py

@@ -55,7 +55,7 @@ class FileIOSkill(PydanticField):
     )
     @sk_function_context_parameter(name="path", description="Destination path")
     @sk_function_context_parameter(name="content", description="File content")
-    async def write_async(self, context: "SKContext"):
+    async def write_async(self, context: "SKContext") -> None:
         """
         Write a file
 
@@ -64,9 +64,6 @@ class FileIOSkill(PydanticField):
         Args:
             Contains the 'path' for the Destination file and
             the 'content' of the file to write.
-
-        Returns:
-            The contents of the file
         """
         has_path, path = context.variables.get("path")
         has_content, content = context.variables.get("content")

python/semantic_kernel/core_skills/text_memory_skill.py

@@ -111,7 +111,7 @@ class TextMemorySkill(PydanticField):
         name=KEY_PARAM,
         description="The unique key to associate with the information",
     )
-    async def save_async(self, text: str, context: "SKContext"):
+    async def save_async(self, text: str, context: "SKContext") -> None:
         """
         Save a fact to the long term memory.
 

python/semantic_kernel/core_skills/wait_skill.py

@@ -18,7 +18,7 @@ class WaitSkill(PydanticField):
     """
 
     @sk_function(description="Wait for a certain number of seconds.")
-    async def wait(self, seconds_text: str):
+    async def wait(self, seconds_text: str) -> None:
         try:
             seconds = max(float(seconds_text), 0)
         except ValueError:

python/semantic_kernel/kernel.py

@@ -734,26 +734,22 @@ class Kernel:
             )
 
         skill_name = os.path.basename(skill_directory)
-        try:
-            spec = importlib.util.spec_from_file_location(
-                MODULE_NAME, native_py_file_path
-            )
-            module = importlib.util.module_from_spec(spec)
-            spec.loader.exec_module(module)
 
-            class_name = next(
-                (
-                    name
-                    for name, cls in inspect.getmembers(module, inspect.isclass)
-                    if cls.__module__ == MODULE_NAME
-                ),
-                None,
-            )
-            if class_name:
-                skill_obj = getattr(module, class_name)()
-                return self.import_skill(skill_obj, skill_name)
-        except Exception:
-            pass
+        spec = importlib.util.spec_from_file_location(MODULE_NAME, native_py_file_path)
+        module = importlib.util.module_from_spec(spec)
+        spec.loader.exec_module(module)
+
+        class_name = next(
+            (
+                name
+                for name, cls in inspect.getmembers(module, inspect.isclass)
+                if cls.__module__ == MODULE_NAME
+            ),
+            None,
+        )
+        if class_name:
+            skill_obj = getattr(module, class_name)()
+            return self.import_skill(skill_obj, skill_name)
 
         return {}
 

python/semantic_kernel/orchestration/delegate_inference.py

@@ -39,6 +39,10 @@ def _return_is_context(signature: Signature) -> bool:
     return _is_annotation_of_type(signature.return_annotation, SKContext)
 
 
+def _return_is_none(signature: Signature) -> bool:
+    return signature.return_annotation is None
+
+
 def _no_return(signature: Signature) -> bool:
     return signature.return_annotation is Signature.empty
 
@@ -79,7 +83,7 @@ class DelegateInference(PydanticField):
     @_infers(DelegateTypes.Void)
     def infer_void(signature: Signature, awaitable: bool) -> bool:
         matches = _has_no_params(signature)
-        matches = matches and _no_return(signature)
+        matches = matches and _return_is_none(signature)
         matches = matches and not awaitable
         return matches
 
@@ -103,7 +107,7 @@ class DelegateInference(PydanticField):
     @_infers(DelegateTypes.InSKContext)
     def infer_in_sk_context(signature: Signature, awaitable: bool) -> bool:
         matches = _first_param_is_context(signature)
-        matches = matches and _no_return(signature)
+        matches = matches and _return_is_none(signature)
         matches = matches and not awaitable
         return matches
 
@@ -139,7 +143,7 @@ class DelegateInference(PydanticField):
     @_infers(DelegateTypes.InString)
     def infer_in_string(signature: Signature, awaitable: bool) -> bool:
         matches = _first_param_is_str(signature)
-        matches = matches and _no_return(signature)
+        matches = matches and _return_is_none(signature)
         matches = matches and not awaitable
         return matches
 
@@ -164,7 +168,7 @@ class DelegateInference(PydanticField):
     def infer_in_string_and_context(signature: Signature, awaitable: bool) -> bool:
         matches = _first_param_is_str(signature, only=False)
         matches = matches and _has_two_params_second_is_context(signature)
-        matches = matches and _no_return(signature)
+        matches = matches and _return_is_none(signature)
         matches = matches and not awaitable
         return matches
 
@@ -205,7 +209,7 @@ class DelegateInference(PydanticField):
     @_infers(DelegateTypes.InStringOutTask)
     def infer_in_string_out_task(signature: Signature, awaitable: bool) -> bool:
         matches = _first_param_is_str(signature)
-        matches = matches and _no_return(signature)
+        matches = matches and _return_is_none(signature)
         matches = matches and awaitable
         return matches
 
@@ -213,7 +217,7 @@ class DelegateInference(PydanticField):
     @_infers(DelegateTypes.InContextOutTask)
     def infer_in_context_out_task(signature: Signature, awaitable: bool) -> bool:
         matches = _first_param_is_context(signature)
-        matches = matches and _no_return(signature)
+        matches = matches and _return_is_none(signature)
         matches = matches and awaitable
         return matches
 
@@ -224,7 +228,7 @@ class DelegateInference(PydanticField):
     ) -> bool:
         matches = _first_param_is_str(signature, only=False)
         matches = matches and _has_two_params_second_is_context(signature)
-        matches = matches and _no_return(signature)
+        matches = matches and _return_is_none(signature)
         matches = matches and awaitable
         return matches
 
@@ -232,7 +236,6 @@ class DelegateInference(PydanticField):
     @_infers(DelegateTypes.OutTask)
     def infer_out_task(signature: Signature, awaitable: bool) -> bool:
         matches = _has_no_params(signature)
-        matches = matches and _no_return(signature)
         matches = matches and awaitable
         return matches
 
@@ -249,6 +252,13 @@ class DelegateInference(PydanticField):
     def infer_delegate_type(function) -> DelegateTypes:
         # Get the function signature
         function_signature = signature(function)
+
+        if _no_return(function_signature):
+            raise KernelException(
+                KernelException.ErrorCodes.FunctionTypeNotSupported,
+                "No return type specified, unable to infer DelegateType.",
+            )
+
         awaitable = iscoroutinefunction(function)
 
         for name, value in DelegateInference.__dict__.items():

python/semantic_kernel/planning/stepwise_planner/stepwise_planner.py

@@ -132,7 +132,7 @@ class StepwisePlanner:
     @sk_function_context_parameter(
         name="function_descriptions", description="List of tool descriptions"
     )
-    async def execute_plan_async(self, context: SKContext):
+    async def execute_plan_async(self, context: SKContext) -> SKContext:
         question = context["question"]
 
         steps_taken: List[SystemStep] = []

python/tests/unit/orchestration/test_native_function.py

@@ -1,25 +1,32 @@
 # Copyright (c) Microsoft. All rights reserved.
 
+from typing import TYPE_CHECKING
+
 from semantic_kernel.orchestration.sk_function import SKFunction
 from semantic_kernel.skill_definition.sk_function_decorator import sk_function
 
+if TYPE_CHECKING:
+    from semantic_kernel.orchestration.sk_context import SKContext
+
 
 def test_init_native_function_with_input_description():
-    class MockMethodWithInputDescription:
-        __sk_function__ = True
-        __sk_function_name__ = "mock_function"
-        __sk_function_description__ = "Mock description"
-        __sk_function_input_description__ = "Mock input description"
-        __sk_function_input_default_value__ = "default_input_value"
-        __sk_function_context_parameters__ = [
-            {
-                "name": "param1",
-                "description": "Param 1 description",
-                "default_value": "default_param1_value",
-            }
-        ]
+    def mock_function(input: str, context: "SKContext") -> None:
+        pass
 
-    mock_method = MockMethodWithInputDescription
+    mock_function.__sk_function__ = True
+    mock_function.__sk_function_name__ = "mock_function"
+    mock_function.__sk_function_description__ = "Mock description"
+    mock_function.__sk_function_input_description__ = "Mock input description"
+    mock_function.__sk_function_input_default_value__ = "default_input_value"
+    mock_function.__sk_function_context_parameters__ = [
+        {
+            "name": "param1",
+            "description": "Param 1 description",
+            "default_value": "default_param1_value",
+        }
+    ]
+
+    mock_method = mock_function
 
     native_function = SKFunction.from_native_method(mock_method, "MockSkill")
 
@@ -33,19 +40,21 @@ def test_init_native_function_with_input_description():
 
 
 def test_init_native_function_without_input_description():
-    class MockMethodWithoutInputDescription:
-        __sk_function__ = True
-        __sk_function_name__ = "mock_function_no_input_desc"
-        __sk_function_description__ = "Mock description no input desc"
-        __sk_function_context_parameters__ = [
-            {
-                "name": "param1",
-                "description": "Param 1 description",
-                "default_value": "default_param1_value",
-            }
-        ]
+    def mock_function(context: "SKContext") -> None:
+        pass
 
-    mock_method = MockMethodWithoutInputDescription
+    mock_function.__sk_function__ = True
+    mock_function.__sk_function_name__ = "mock_function_no_input_desc"
+    mock_function.__sk_function_description__ = "Mock description no input desc"
+    mock_function.__sk_function_context_parameters__ = [
+        {
+            "name": "param1",
+            "description": "Param 1 description",
+            "default_value": "default_param1_value",
+        }
+    ]
+
+    mock_method = mock_function
 
     native_function = SKFunction.from_native_method(mock_method, "MockSkill")
 
@@ -62,7 +71,7 @@ def test_init_native_function_from_sk_function_decorator():
         input_description="Test input description",
         input_default_value="test_default_value",
     )
-    def decorated_function():
+    def decorated_function() -> None:
         pass
 
     assert decorated_function.__sk_function__ is True
@@ -85,7 +94,7 @@ def test_init_native_function_from_sk_function_decorator():
 
 def test_init_native_function_from_sk_function_decorator_defaults():
     @sk_function()
-    def decorated_function():
+    def decorated_function() -> None:
         pass
 
     assert decorated_function.__sk_function__ is True