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: Fix #10389 #10406

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

RamType0
Copy link

@RamType0 RamType0 commented Feb 5, 2025

Motivation and Context

Fixes issue #10389.

Description

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

Contribution Checklist

Use `JsonSerializerOptions.Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping`
@RamType0 RamType0 requested a review from a team as a code owner February 5, 2025 02:40
@markwallace-microsoft markwallace-microsoft added the .NET Issue or Pull requests regarding .NET code label Feb 5, 2025
@github-actions github-actions bot changed the title Fix #10389 .Net: Fix #10389 Feb 5, 2025
@SergeyMenshykh
Copy link
Member

Hi @RamType0, thanks for submitting the PR. We are evaluating the fix along with an alternative solution that would allow for customizing the function result serialization process. Please bear with us; it shouldn't take long.

dmytrostruk

This comment was marked as outdated.

@RamType0
Copy link
Author

RamType0 commented Feb 5, 2025

@RamType0 Thanks for your contribution!

I'm not sure if this solution will work since this encoder may impact the serialization for other users in unpredictable way. Here is more information: image

Why you think that we will use the function result for HTML rendering directly?

I could not imagine that scenario.

In ideal scenario, users should be able to provide their custom JsonSerializerOptions which will be used for {de}serialization through the entire Semantic Kernel flow. We know that it's not possible today and we may work on it to add this feature in the future.

Customization feature is good, but I think this should be the default behavior.

Many alphabet speaker programmers previously made many bugs with multi byte characters.
And today, they will made that kind of bugs with this library again.

@dmytrostruk
Copy link
Member

@RamType0 Thanks for your response! My comment is outdated now, @SergeyMenshykh will provide more details soon.

@SergeyMenshykh
Copy link
Member

Hi @RamType0, we plan to proceed with merging this PR, but before that, please cover the change with unit test(s).

@SergeyMenshykh
Copy link
Member

Linking this PR to the M.E.AI one: Use unsafe relaxed escaping in AIJsonUtilities.DefaultOptions as both address the same issue

@markwallace-microsoft markwallace-microsoft added the kernel Issues or pull requests impacting the core kernel label Feb 7, 2025
@RamType0
Copy link
Author

RamType0 commented Feb 7, 2025

@microsoft-github-policy-service agree

@SergeyMenshykh SergeyMenshykh added this pull request to the merge queue Feb 7, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kernel Issues or pull requests impacting the core kernel .NET Issue or Pull requests regarding .NET code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants