Skip to content

Commit 45f492a

Browse files
authored
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](#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)*
1 parent b65fb53 commit 45f492a

7 files changed

Lines changed: 76 additions & 64 deletions

File tree

‎python/semantic_kernel/core_skills/file_io_skill.py‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ async def read_async(self, path: str) -> str:
5555
)
5656
@sk_function_context_parameter(name="path", description="Destination path")
5757
@sk_function_context_parameter(name="content", description="File content")
58-
async def write_async(self, context: "SKContext"):
58+
async def write_async(self, context: "SKContext") -> None:
5959
"""
6060
Write a file
6161
@@ -64,9 +64,6 @@ async def write_async(self, context: "SKContext"):
6464
Args:
6565
Contains the 'path' for the Destination file and
6666
the 'content' of the file to write.
67-
68-
Returns:
69-
The contents of the file
7067
"""
7168
has_path, path = context.variables.get("path")
7269
has_content, content = context.variables.get("content")

‎python/semantic_kernel/core_skills/text_memory_skill.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,7 @@ async def recall_async(self, ask: str, context: "SKContext") -> str:
111111
name=KEY_PARAM,
112112
description="The unique key to associate with the information",
113113
)
114-
async def save_async(self, text: str, context: "SKContext"):
114+
async def save_async(self, text: str, context: "SKContext") -> None:
115115
"""
116116
Save a fact to the long term memory.
117117

‎python/semantic_kernel/core_skills/wait_skill.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ class WaitSkill(PydanticField):
1818
"""
1919

2020
@sk_function(description="Wait for a certain number of seconds.")
21-
async def wait(self, seconds_text: str):
21+
async def wait(self, seconds_text: str) -> None:
2222
try:
2323
seconds = max(float(seconds_text), 0)
2424
except ValueError:

‎python/semantic_kernel/kernel.py‎

Lines changed: 15 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -734,26 +734,22 @@ def import_native_skill_from_directory(
734734
)
735735

736736
skill_name = os.path.basename(skill_directory)
737-
try:
738-
spec = importlib.util.spec_from_file_location(
739-
MODULE_NAME, native_py_file_path
740-
)
741-
module = importlib.util.module_from_spec(spec)
742-
spec.loader.exec_module(module)
743737

744-
class_name = next(
745-
(
746-
name
747-
for name, cls in inspect.getmembers(module, inspect.isclass)
748-
if cls.__module__ == MODULE_NAME
749-
),
750-
None,
751-
)
752-
if class_name:
753-
skill_obj = getattr(module, class_name)()
754-
return self.import_skill(skill_obj, skill_name)
755-
except Exception:
756-
pass
738+
spec = importlib.util.spec_from_file_location(MODULE_NAME, native_py_file_path)
739+
module = importlib.util.module_from_spec(spec)
740+
spec.loader.exec_module(module)
741+
742+
class_name = next(
743+
(
744+
name
745+
for name, cls in inspect.getmembers(module, inspect.isclass)
746+
if cls.__module__ == MODULE_NAME
747+
),
748+
None,
749+
)
750+
if class_name:
751+
skill_obj = getattr(module, class_name)()
752+
return self.import_skill(skill_obj, skill_name)
757753

758754
return {}
759755

‎python/semantic_kernel/orchestration/delegate_inference.py‎

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,10 @@ def _return_is_context(signature: Signature) -> bool:
3939
return _is_annotation_of_type(signature.return_annotation, SKContext)
4040

4141

42+
def _return_is_none(signature: Signature) -> bool:
43+
return signature.return_annotation is None
44+
45+
4246
def _no_return(signature: Signature) -> bool:
4347
return signature.return_annotation is Signature.empty
4448

@@ -79,7 +83,7 @@ class DelegateInference(PydanticField):
7983
@_infers(DelegateTypes.Void)
8084
def infer_void(signature: Signature, awaitable: bool) -> bool:
8185
matches = _has_no_params(signature)
82-
matches = matches and _no_return(signature)
86+
matches = matches and _return_is_none(signature)
8387
matches = matches and not awaitable
8488
return matches
8589

@@ -103,7 +107,7 @@ def infer_out_task_string(signature: Signature, awaitable: bool) -> bool:
103107
@_infers(DelegateTypes.InSKContext)
104108
def infer_in_sk_context(signature: Signature, awaitable: bool) -> bool:
105109
matches = _first_param_is_context(signature)
106-
matches = matches and _no_return(signature)
110+
matches = matches and _return_is_none(signature)
107111
matches = matches and not awaitable
108112
return matches
109113

@@ -139,7 +143,7 @@ def infer_context_switch_in_sk_context_out_task_sk_context(
139143
@_infers(DelegateTypes.InString)
140144
def infer_in_string(signature: Signature, awaitable: bool) -> bool:
141145
matches = _first_param_is_str(signature)
142-
matches = matches and _no_return(signature)
146+
matches = matches and _return_is_none(signature)
143147
matches = matches and not awaitable
144148
return matches
145149

@@ -164,7 +168,7 @@ def infer_in_string_out_task_string(signature: Signature, awaitable: bool) -> bo
164168
def infer_in_string_and_context(signature: Signature, awaitable: bool) -> bool:
165169
matches = _first_param_is_str(signature, only=False)
166170
matches = matches and _has_two_params_second_is_context(signature)
167-
matches = matches and _no_return(signature)
171+
matches = matches and _return_is_none(signature)
168172
matches = matches and not awaitable
169173
return matches
170174

@@ -205,15 +209,15 @@ def infer_context_switch_in_string_and_context_out_task_context(
205209
@_infers(DelegateTypes.InStringOutTask)
206210
def infer_in_string_out_task(signature: Signature, awaitable: bool) -> bool:
207211
matches = _first_param_is_str(signature)
208-
matches = matches and _no_return(signature)
212+
matches = matches and _return_is_none(signature)
209213
matches = matches and awaitable
210214
return matches
211215

212216
@staticmethod
213217
@_infers(DelegateTypes.InContextOutTask)
214218
def infer_in_context_out_task(signature: Signature, awaitable: bool) -> bool:
215219
matches = _first_param_is_context(signature)
216-
matches = matches and _no_return(signature)
220+
matches = matches and _return_is_none(signature)
217221
matches = matches and awaitable
218222
return matches
219223

@@ -224,15 +228,14 @@ def infer_in_string_and_context_out_task(
224228
) -> bool:
225229
matches = _first_param_is_str(signature, only=False)
226230
matches = matches and _has_two_params_second_is_context(signature)
227-
matches = matches and _no_return(signature)
231+
matches = matches and _return_is_none(signature)
228232
matches = matches and awaitable
229233
return matches
230234

231235
@staticmethod
232236
@_infers(DelegateTypes.OutTask)
233237
def infer_out_task(signature: Signature, awaitable: bool) -> bool:
234238
matches = _has_no_params(signature)
235-
matches = matches and _no_return(signature)
236239
matches = matches and awaitable
237240
return matches
238241

@@ -249,6 +252,13 @@ def infer_unknown(signature: Signature, awaitable: bool) -> NoReturn:
249252
def infer_delegate_type(function) -> DelegateTypes:
250253
# Get the function signature
251254
function_signature = signature(function)
255+
256+
if _no_return(function_signature):
257+
raise KernelException(
258+
KernelException.ErrorCodes.FunctionTypeNotSupported,
259+
"No return type specified, unable to infer DelegateType.",
260+
)
261+
252262
awaitable = iscoroutinefunction(function)
253263

254264
for name, value in DelegateInference.__dict__.items():

‎python/semantic_kernel/planning/stepwise_planner/stepwise_planner.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,7 @@ def create_plan(self, goal: str) -> Plan:
132132
@sk_function_context_parameter(
133133
name="function_descriptions", description="List of tool descriptions"
134134
)
135-
async def execute_plan_async(self, context: SKContext):
135+
async def execute_plan_async(self, context: SKContext) -> SKContext:
136136
question = context["question"]
137137

138138
steps_taken: List[SystemStep] = []

‎python/tests/unit/orchestration/test_native_function.py‎

Lines changed: 39 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -1,25 +1,32 @@
11
# Copyright (c) Microsoft. All rights reserved.
22

3+
from typing import TYPE_CHECKING
4+
35
from semantic_kernel.orchestration.sk_function import SKFunction
46
from semantic_kernel.skill_definition.sk_function_decorator import sk_function
57

8+
if TYPE_CHECKING:
9+
from semantic_kernel.orchestration.sk_context import SKContext
10+
611

712
def test_init_native_function_with_input_description():
8-
class MockMethodWithInputDescription:
9-
__sk_function__ = True
10-
__sk_function_name__ = "mock_function"
11-
__sk_function_description__ = "Mock description"
12-
__sk_function_input_description__ = "Mock input description"
13-
__sk_function_input_default_value__ = "default_input_value"
14-
__sk_function_context_parameters__ = [
15-
{
16-
"name": "param1",
17-
"description": "Param 1 description",
18-
"default_value": "default_param1_value",
19-
}
20-
]
21-
22-
mock_method = MockMethodWithInputDescription
13+
def mock_function(input: str, context: "SKContext") -> None:
14+
pass
15+
16+
mock_function.__sk_function__ = True
17+
mock_function.__sk_function_name__ = "mock_function"
18+
mock_function.__sk_function_description__ = "Mock description"
19+
mock_function.__sk_function_input_description__ = "Mock input description"
20+
mock_function.__sk_function_input_default_value__ = "default_input_value"
21+
mock_function.__sk_function_context_parameters__ = [
22+
{
23+
"name": "param1",
24+
"description": "Param 1 description",
25+
"default_value": "default_param1_value",
26+
}
27+
]
28+
29+
mock_method = mock_function
2330

2431
native_function = SKFunction.from_native_method(mock_method, "MockSkill")
2532

@@ -33,19 +40,21 @@ class MockMethodWithInputDescription:
3340

3441

3542
def test_init_native_function_without_input_description():
36-
class MockMethodWithoutInputDescription:
37-
__sk_function__ = True
38-
__sk_function_name__ = "mock_function_no_input_desc"
39-
__sk_function_description__ = "Mock description no input desc"
40-
__sk_function_context_parameters__ = [
41-
{
42-
"name": "param1",
43-
"description": "Param 1 description",
44-
"default_value": "default_param1_value",
45-
}
46-
]
47-
48-
mock_method = MockMethodWithoutInputDescription
43+
def mock_function(context: "SKContext") -> None:
44+
pass
45+
46+
mock_function.__sk_function__ = True
47+
mock_function.__sk_function_name__ = "mock_function_no_input_desc"
48+
mock_function.__sk_function_description__ = "Mock description no input desc"
49+
mock_function.__sk_function_context_parameters__ = [
50+
{
51+
"name": "param1",
52+
"description": "Param 1 description",
53+
"default_value": "default_param1_value",
54+
}
55+
]
56+
57+
mock_method = mock_function
4958

5059
native_function = SKFunction.from_native_method(mock_method, "MockSkill")
5160

@@ -62,7 +71,7 @@ def test_init_native_function_from_sk_function_decorator():
6271
input_description="Test input description",
6372
input_default_value="test_default_value",
6473
)
65-
def decorated_function():
74+
def decorated_function() -> None:
6675
pass
6776

6877
assert decorated_function.__sk_function__ is True
@@ -85,7 +94,7 @@ def decorated_function():
8594

8695
def test_init_native_function_from_sk_function_decorator_defaults():
8796
@sk_function()
88-
def decorated_function():
97+
def decorated_function() -> None:
8998
pass
9099

91100
assert decorated_function.__sk_function__ is True

0 commit comments

Comments
 (0)