Skip to content

Commit

Permalink
Python: Fix KernelArgument bug in invoke_prompt functions (#8414)
Browse files Browse the repository at this point in the history
### 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.
-->
In `invoke_prompt(...)` and `invoke_prompt_async(...)`, the `arguments`
parameter is tested to see if it's empty before proceeding. The type of
the `arguments` parameter is `KernelArgument`, which derives from a
`dict` type, with an additional field for execution settings. When
`arguments` contains only the execution settings, it will be considered
as an empty dictionary. This will result in the `arguments` parameter
being overridden completely, invalidating all execution settings passed
to the APIs.

To reproduce (assuming we have a plugin/function that will return the
weather):
```
result = await kernel.invoke_prompt(
        "What is the weather like in my location?",
        arguments=KernelArguments(
            settings={
                service_id: PromptExecutionSettings(
                    service_id=service_id,
                    function_choice_behavior=FunctionChoiceBehavior.Auto(),
                ),
            }
        ),
    )
```
In the example above, the settings will be overridden to None.

### Description

<!-- Describe your changes, the overall approach, the underlying design.
These notes will help understanding how your code works. Thanks! -->
1. Fix the bug.
2. Fix mypy in the pre-commit hook in order to commit.

### 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
- [x] I didn't break anyone 😄
  • Loading branch information
TaoChenOSU authored Aug 29, 2024
1 parent 53dcf39 commit 2205df8
Show file tree
Hide file tree
Showing 4 changed files with 21 additions and 6 deletions.
2 changes: 1 addition & 1 deletion python/.pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ repos:
- id: mypy
files: ^python/semantic_kernel/
name: mypy
entry: cd python && uv run mypy -p semantic_kernel --config-file mypy.ini
entry: uv run mypy -p semantic_kernel --config-file python/mypy.ini
language: system
types: [python]
pass_filenames: false
Expand Down
6 changes: 6 additions & 0 deletions python/semantic_kernel/functions/kernel_arguments.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,3 +43,9 @@ def __init__(
else:
settings_dict = {settings.service_id or DEFAULT_SERVICE_NAME: settings}
self.execution_settings: dict[str, "PromptExecutionSettings"] | None = settings_dict

def __bool__(self) -> bool:
"""Returns True if the arguments have any values."""
has_arguments = self.__len__() > 0
has_execution_settings = self.execution_settings is not None and len(self.execution_settings) > 0
return has_arguments or has_execution_settings
8 changes: 3 additions & 5 deletions python/semantic_kernel/kernel.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,7 @@
from semantic_kernel.utils.naming import generate_random_ascii_name

if TYPE_CHECKING:
from semantic_kernel.connectors.ai.function_choice_behavior import (
FunctionChoiceBehavior,
)
from semantic_kernel.connectors.ai.function_choice_behavior import FunctionChoiceBehavior
from semantic_kernel.connectors.ai.prompt_execution_settings import PromptExecutionSettings
from semantic_kernel.functions.kernel_function import KernelFunction

Expand Down Expand Up @@ -239,7 +237,7 @@ async def invoke_prompt(
Returns:
FunctionResult | list[FunctionResult] | None: The result of the function(s)
"""
if not arguments:
if arguments is None:
arguments = KernelArguments(**kwargs)
if not prompt:
raise TemplateSyntaxError("The prompt is either null or empty.")
Expand Down Expand Up @@ -280,7 +278,7 @@ async def invoke_prompt_stream(
Returns:
AsyncIterable[StreamingContentMixin]: The content of the stream of the last function provided.
"""
if not arguments:
if arguments is None:
arguments = KernelArguments(**kwargs)
if not prompt:
raise TemplateSyntaxError("The prompt is either null or empty.")
Expand Down
11 changes: 11 additions & 0 deletions python/tests/unit/functions/test_kernel_arguments.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,3 +35,14 @@ def test_kernel_arguments_with_execution_settings():
kargs = KernelArguments(settings=[test_pes])
assert kargs is not None
assert kargs.execution_settings == {"test": test_pes}


def test_kernel_arguments_bool():
# An empty KernelArguments object should return False
assert not KernelArguments()
# An KernelArguments object with keyword arguments should return True
assert KernelArguments(input=10)
# An KernelArguments object with execution_settings should return True
assert KernelArguments(settings=PromptExecutionSettings(service_id="test"))
# An KernelArguments object with both keyword arguments and execution_settings should return True
assert KernelArguments(input=10, settings=PromptExecutionSettings(service_id="test"))

0 comments on commit 2205df8

Please sign in to comment.