Skip to content
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

.Net: FunctionResult serializer should use JsonSerializerOptions.Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping #10389

Open
RamType0 opened this issue Feb 4, 2025 · 4 comments
Assignees
Labels
.NET Issue or Pull requests regarding .NET code

Comments

@RamType0
Copy link
Contributor

RamType0 commented Feb 4, 2025

When I am using KernelFunction which returns user defined object, then it is called by Auto function invocation,
the result of the kernel function will be converted to string with JsonSerializer.Serialize via FunctionCallsProcessor.ProcessFunctionResult.

public static string ProcessFunctionResult(object functionResult)
{
if (functionResult is string stringResult)
{
return stringResult;
}
// This is an optimization to use ChatMessageContent content directly
// without unnecessary serialization of the whole message content class.
if (functionResult is ChatMessageContent chatMessageContent)
{
return chatMessageContent.ToString();
}
return JsonSerializer.Serialize(functionResult);
}

Because JsonSerializer.Serialize very likely to escape characters by default.
if I have this kind of record,

record A(string Text);

A a = new("オダ ノブナガ");

string x = JsonSerializer.Serialize(a);

the content of the x is ...

{"Text":"\uFF75\uFF80\uFF9E \uFF89\uFF8C\uFF9E\uFF85\uFF76\uFF9E"}

But at least, Azure OpenAI gpt-4o (2024-08-06) could not handle this kind of escaped characters.

But if I use

string y = JsonSerializer.Serialize(a, new JsonSerializerOptions(JsonSerializerOptions.Default)
{
    Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping,
});

The content of the y is ...

{"Text":"オダ ノブナガ"}

So it will be easily handled by LLM.

Thus, IMHO, FunctionCallsProcessor.ProcessFunctionResult should use new JsonSerializerOptions(JsonSerializerOptions.Default) { Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping, } for JsonSerializer.Serialize by default.

Also, even if we pass JsonSerializerOptions to IKernelBuilderPlugins AddFromType, it is not used for result serialization.

@markwallace-microsoft markwallace-microsoft added .NET Issue or Pull requests regarding .NET code java Issue or PR regarding Java code triage labels Feb 4, 2025
@github-actions github-actions bot changed the title FunctionResult serializer should use JsonSerializerOptions.Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping .Net: FunctionResult serializer should use JsonSerializerOptions.Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping Feb 4, 2025
@github-actions github-actions bot changed the title .Net: FunctionResult serializer should use JsonSerializerOptions.Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping Java: FunctionResult serializer should use JsonSerializerOptions.Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping Feb 4, 2025
@moonbox3
Copy link
Contributor

moonbox3 commented Feb 4, 2025

Hi @RamType0, thanks for filing the issue. The SK Java repo is housed here. Could you please re-file it in that repo? Thank you!

@moonbox3 moonbox3 closed this as completed Feb 4, 2025
@RamType0
Copy link
Contributor Author

RamType0 commented Feb 4, 2025

No, it is .NET issue.
But the bot automatically added Java label.

@moonbox3
Copy link
Contributor

moonbox3 commented Feb 4, 2025

Thanks for your response!

@moonbox3 moonbox3 reopened this Feb 4, 2025
@moonbox3 moonbox3 changed the title Java: FunctionResult serializer should use JsonSerializerOptions.Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping .Net: FunctionResult serializer should use JsonSerializerOptions.Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping Feb 4, 2025
@moonbox3 moonbox3 removed the java Issue or PR regarding Java code label Feb 4, 2025
@sphenry sphenry removed the triage label Feb 4, 2025
@sphenry
Copy link
Member

sphenry commented Feb 4, 2025

@dmytrostruk can you take a look?

RamType0 added a commit to RamType0/semantic-kernel that referenced this issue Feb 5, 2025
Use `JsonSerializerOptions.Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping`
@RamType0 RamType0 mentioned this issue Feb 5, 2025
4 tasks
@markwallace-microsoft markwallace-microsoft moved this to Sprint: Planned in Semantic Kernel Feb 6, 2025
github-merge-queue bot pushed a commit that referenced this issue Feb 10, 2025
### Motivation and Context

Fixes issue #10389.

### Description

Use `JsonSerializerOptions.Encoder =
JavaScriptEncoder.UnsafeRelaxedJsonEscaping` to generate more LLM
friendly serialized FunctionResult.

### 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 😄

---------

Co-authored-by: SergeyMenshykh <[email protected]>
@SergeyMenshykh SergeyMenshykh moved this from Sprint: Planned to Sprint: Done in Semantic Kernel Feb 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
.NET Issue or Pull requests regarding .NET code
Projects
Status: Sprint: Done
Development

No branches or pull requests

6 participants