.Net: Marshaling of function arguments #4044
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stephentoub
reviewed
Dec 7, 2023
dotnet/src/SemanticKernel.Core/Functions/KernelFunctionFromMethod.cs
Outdated
Show resolved
Hide resolved
stephentoub
reviewed
Dec 7, 2023
dotnet/src/SemanticKernel.Core/Functions/KernelFunctionFromMethod.cs
Outdated
Show resolved
Hide resolved
stephentoub
reviewed
Dec 7, 2023
dotnet/src/SemanticKernel.Core/Functions/KernelFunctionFromMethod.cs
Outdated
Show resolved
Hide resolved
…ypes to support implicit conversions such as: Guid to string, Enum to int, etc.
…om/microsoft/semantic-kernel into function-arguments-type-marshaling
SergeyMenshykh
changed the title
SergeyMenshykh
requested review from
dmytrostruk,
RogerBarreto and
markwallace-microsoft
4 tasks
stephentoub
reviewed
Dec 8, 2023
dotnet/src/SemanticKernel.Core/Functions/KernelFunctionFromMethod.cs
Outdated
Show resolved
Hide resolved
stephentoub
reviewed
Dec 8, 2023
dotnet/src/SemanticKernel.Core/Functions/KernelFunctionFromMethod.cs
Outdated
Show resolved
Hide resolved
stephentoub
approved these changes
Dec 8, 2023
…om/microsoft/semantic-kernel into function-arguments-type-marshaling
dmytrostruk
approved these changes
Dec 11, 2023
dotnet/src/SemanticKernel.UnitTests/Functions/KernelFunctionFromMethodTests1.cs
Outdated
Show resolved
Hide resolved
dotnet/src/SemanticKernel.UnitTests/Functions/KernelFunctionFromMethodTests1.cs
Outdated
Show resolved
Hide resolved
markwallace-microsoft
added
the
v1.0.1
dmytrostruk
approved these changes
Dec 11, 2023
Kevdome3000
pushed a commit
to Kevdome3000/semantic-kernel
that referenced
this pull request
Dec 11, 2023
This PR changes the way function arguments are marshalled to function parameters. Currently, the marshaling process passes arguments of non-string types as they are to the `MethodInfo.Invoke`. String arguments are marshaled through .NET type converters, such as `BooleanConverter`, `Int32Converter`, etc., which convert the string arguments to the target types of method parameters. Additionally, if no suitable converter can be found to convert a string to the target type, the current marshaling mechanism checks if the target type has a type converter registered through the `TypeConverterAttribute` and use it. Even though the mechanism works today, it has a few limitations: - Implicit casting/conversion is available for primitives only. Conversions such as int to long and float to double work out of the box because they are supported by the runtime. However, slightly complex conversions like guid to string or enum to int are not supported and require additional handling. - Data converters for custom types, registered through `TypeConverterAttribute`, are only supported for string arguments and not for non-string ones. This is because string arguments undergo custom marshaling that respects the attribute, while non-string arguments are handled by the runtime, which does not take it into account. To overcome the limitations, this PR applies custom marshaling to all arguments of string type and non-string types. The custom marshaling relies on the data type converter mentioned above and respects the `TypeConverterAttribute`. Moreover, it leverages type converters to support conversions such as guid to string or enum to int. Update: An interesting side effect was discovered during the implicit conversion from a double argument to an int parameter using the `System.ComponentModel.DoubleConverter.ConvertTo` method. Instead of casting the 99.9 double value to a 99 int value, it rounded it up to 100. The fix is simple - ensure that the argument types match the parameter types. (cherry picked from commit 48c7c8d)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR changes the way function arguments are marshalled to function parameters.
Currently, the marshaling process passes arguments of non-string types as they are to the
MethodInfo.Invoke. String arguments are marshaled through .NET type converters, such asBooleanConverter,Int32Converter, etc., which convert the string arguments to the target types of method parameters. Additionally, if no suitable converter can be found to convert a string to the target type, the current marshaling mechanism checks if the target type has a type converter registered through theTypeConverterAttributeand use it.Even though the mechanism works today, it has a few limitations:
TypeConverterAttribute, are only supported for string arguments and not for non-string ones. This is because string arguments undergo custom marshaling that respects the attribute, while non-string arguments are handled by the runtime, which does not take it into account.To overcome the limitations, this PR applies custom marshaling to all arguments of string type and non-string types. The custom marshaling relies on the data type converter mentioned above and respects the
TypeConverterAttribute. Moreover, it leverages type converters to support conversions such as guid to string or enum to int.Update: An interesting side effect was discovered during the implicit conversion from a double argument to an int parameter using the
System.ComponentModel.DoubleConverter.ConvertTomethod. Instead of casting the 99.9 double value to a 99 int value, it rounded it up to 100. The fix is simple - ensure that the argument types match the parameter types.