-
Notifications
You must be signed in to change notification settings - Fork 4.5k
Python: fix: optimize KernelArguments merge to avoid unnecessary dict copy #13598
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 7 commits
2041c8a
da0c622
574ab8e
9f9fa02
43bc49d
b1374d6
e2ed726
76c1c3b
810cb92
b37a8cc
44250c7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -67,10 +67,15 @@ def __or__(self, value: dict) -> "KernelArguments": | |||||||||||||||
| f"TypeError: unsupported operand type(s) for |: '{type(self).__name__}' and '{type(value).__name__}'" | ||||||||||||||||
| ) | ||||||||||||||||
|
|
||||||||||||||||
| # Merge execution settings | ||||||||||||||||
| new_execution_settings = (self.execution_settings or {}).copy() | ||||||||||||||||
| # Merge execution settings - only copy when needed | ||||||||||||||||
| if self.execution_settings: | ||||||||||||||||
| new_execution_settings = self.execution_settings # Reuse reference if immutable | ||||||||||||||||
|
||||||||||||||||
| # Merge execution settings - only copy when needed | |
| if self.execution_settings: | |
| new_execution_settings = self.execution_settings # Reuse reference if immutable | |
| # Merge execution settings | |
| new_execution_settings = (self.execution_settings or {}).copy() | |
| if isinstance(value, KernelArguments) and value.execution_settings: | |
| new_execution_settings |= value.execution_settings |
nimanikoo marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
nimanikoo marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
nimanikoo marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
nimanikoo marked this conversation as resolved.
Show resolved
Hide resolved
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,116 @@ | ||
| # Copyright (c) Microsoft. All rights reserved. | ||
|
|
||
| """Tests for KernelArguments merge optimization (Issue #2: Lazy dict copy).""" | ||
|
|
||
| from semantic_kernel.functions.kernel_arguments import KernelArguments | ||
| class TestKernelArgumentsMergeOptimization: | ||
| """Test suite for KernelArguments merge operator optimization.""" | ||
|
|
||
| def test_or_operator_no_execution_settings_copy(self): | ||
| """Test that | operator doesn't copy when execution_settings is None or empty. | ||
|
|
||
| This tests the optimization where dict copy is avoided when not needed. | ||
| """ | ||
| args = KernelArguments(a=1, b=2) | ||
| args.execution_settings = None | ||
|
|
||
| result = args | {"c": 3} | ||
|
|
||
| # Result should have merged values | ||
| assert result["a"] == 1 | ||
| assert result["b"] == 2 | ||
| assert result["c"] == 3 | ||
nimanikoo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| def test_or_operator_with_kernel_arguments_merge(self): | ||
| """Test | operator with another KernelArguments with execution_settings.""" | ||
| settings1 = {"model": "gpt-4", "temperature": 0.7} | ||
| settings2 = {"temperature": 0.9, "max_tokens": 100} | ||
|
|
||
nimanikoo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| args1 = KernelArguments(a=1, settings=settings1) | ||
| args2 = KernelArguments(b=2, settings=settings2) | ||
|
|
||
| result = args1 | args2 | ||
|
|
||
| # Check merged arguments | ||
| assert result["a"] == 1 | ||
| assert result["b"] == 2 | ||
|
|
||
| # Check merged execution settings | ||
| assert result.execution_settings["model"] == "gpt-4" | ||
| assert result.execution_settings["temperature"] == 0.9 # args2 overwrites | ||
| assert result.execution_settings["max_tokens"] == 100 | ||
|
Comment on lines
+26
to
+41
|
||
|
|
||
| def test_ror_operator_lazy_copy(self): | ||
| """Test reverse | operator avoids copy when execution_settings is empty.""" | ||
| args = KernelArguments(a=1, b=2) | ||
| args.execution_settings = {} | ||
|
|
||
| result = {"c": 3} | args | ||
|
|
||
| # Result should have merged values | ||
| assert result["a"] == 1 | ||
| assert result["b"] == 2 | ||
| assert result["c"] == 3 | ||
|
|
||
| def test_ior_operator_lazy_copy(self): | ||
| """Test in-place |= operator avoids copy when execution_settings exists.""" | ||
| settings1 = {"model": "gpt-4", "temperature": 0.7} | ||
| args1 = KernelArguments(a=1, settings=settings1) | ||
|
|
||
| settings2 = {"temperature": 0.9} | ||
| args2 = KernelArguments(b=2, settings=settings2) | ||
|
|
||
| args1 |= args2 | ||
|
|
||
| # Check merged in-place | ||
| assert args1["a"] == 1 | ||
| assert args1["b"] == 2 | ||
|
|
||
| # Check in-place merge of settings | ||
| assert args1.execution_settings["model"] == "gpt-4" | ||
| assert args1.execution_settings["temperature"] == 0.9 | ||
|
|
||
| def test_ior_operator_creates_copy_when_needed(self): | ||
| """Test that |= creates new dict only when target has no execution_settings.""" | ||
| args1 = KernelArguments(a=1) | ||
| args1.execution_settings = None | ||
|
|
||
| settings2 = {"model": "gpt-4"} | ||
| args2 = KernelArguments(b=2, settings=settings2) | ||
|
|
||
| args1 |= args2 | ||
|
|
||
| # Should have new execution_settings dict | ||
| assert args1.execution_settings is not None | ||
| assert args1.execution_settings["model"] == "gpt-4" | ||
| # Should not be the same reference (it's a copy) | ||
| assert args1.execution_settings is not args2.execution_settings | ||
|
|
||
| def test_or_operator_preserves_original_settings(self): | ||
| """Test that | operator doesn't mutate original execution_settings.""" | ||
| original_settings = {"model": "gpt-4", "temperature": 0.7} | ||
| args1 = KernelArguments(a=1, settings=original_settings.copy()) | ||
| args2 = KernelArguments(b=2, settings={"temperature": 0.9}) | ||
|
|
||
| result = args1 | args2 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test only covers the case where both sides have def test_or_operator_with_plain_dict_preserves_original_settings(self):
args1 = KernelArguments(a=1, settings=PromptExecutionSettings(service_id='s1'))
result = args1 | {'b': 2}
assert result.execution_settings is not args1.execution_settings |
||
|
|
||
| # Original args1 settings should be unchanged | ||
| assert args1.execution_settings["temperature"] == 0.7 | ||
| # Result should have merged settings | ||
| assert result.execution_settings["temperature"] == 0.9 | ||
nimanikoo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| def test_ior_operator_merges_into_existing_dict(self): | ||
| """Test that |= merges into existing settings dict when present.""" | ||
| settings1 = {"model": "gpt-4", "temperature": 0.7} | ||
| args1 = KernelArguments(a=1, settings=settings1) | ||
| original_dict = args1.execution_settings | ||
|
|
||
| settings2 = {"temperature": 0.9, "max_tokens": 100} | ||
| args2 = KernelArguments(b=2, settings=settings2) | ||
|
|
||
| args1 |= args2 | ||
|
|
||
| # Should have reused and updated the original dict | ||
| assert args1.execution_settings is original_dict | ||
| assert args1.execution_settings["temperature"] == 0.9 | ||
| assert args1.execution_settings["max_tokens"] == 100 | ||
Uh oh!
There was an error while loading. Please reload this page.