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

allow serialize_to_str to work with non ascii when dumping via json.d… #2714

Merged
merged 3 commits into from
May 24, 2024

Conversation

jtoy
Copy link
Collaborator

@jtoy jtoy commented May 17, 2024

allow serialize_to_str to work with non ascii when dumping via json.dumps

Why are these changes needed?

developers will use serialize_to_str with non ascii characters, this allows them to do that.

Related issue number

Closes #2511

Checks

@Hk669 Hk669 requested review from ekzhu and Hk669 May 17, 2024 18:36
@marklysze
Copy link
Collaborator

marklysze commented May 19, 2024

Hi @jtoy, I have used json.dumps in the GroupChatManager to serialise messages for use in resuming chats later on:

Code here

Do you think it would be worth adding the ensure_ascii=False parameter to this line of code too?

UPDATE: see next comment...

@marklysze
Copy link
Collaborator

An update on my question...

I've tested my GroupChat code with and without the ensure_ascii=False and although the serialisation has the coded characters (e.g. \\u4e2d instead of ) when it is False, it will still restore the messages correctly with irrespective of whether ensure_ascii is True or False. I have also checked saving it to disk and loading from desk and it works fine.

So, for the purposes of serialising messages for saving GroupChat messages and resuming from it, it's not necessary to change the code I suggested.

@jtoy
Copy link
Collaborator Author

jtoy commented May 21, 2024

@marklysze Are you sure it went through similar code paths? In my PR, I added a unit test that shows if ensure_ascii is not used, it will change the formatting.

@jtoy
Copy link
Collaborator Author

jtoy commented May 21, 2024

The PR I wrote is still valid, can someone review the PRs please.

@marklysze
Copy link
Collaborator

@marklysze Are you sure it went through similar code paths? In my PR, I added a unit test that shows if ensure_ascii is not used, it will change the formatting.

Hi @jtoy, I used it to save to a file, then load from the file, and back into messages and without ensure_ascii=False the resulting messages were the same.

Your scenario may be different to mine so I think, as you are doing, you should continue with the PR, I just didn't see it affect the code section I referenced.

@ekzhu ekzhu added this pull request to the merge queue May 21, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 21, 2024
test/test_function_utils.py Show resolved Hide resolved
@sonichi sonichi added this pull request to the merge queue May 21, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 21, 2024
@sonichi sonichi added this pull request to the merge queue May 24, 2024
@codecov-commenter
Copy link

codecov-commenter commented May 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 40.54%. Comparing base (11d9336) to head (d7f0811).
Report is 24 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2714      +/-   ##
==========================================
+ Coverage   33.60%   40.54%   +6.94%     
==========================================
  Files          87       87              
  Lines        9336     9386      +50     
  Branches     1987     2154     +167     
==========================================
+ Hits         3137     3806     +669     
+ Misses       5933     5229     -704     
- Partials      266      351      +85     
Flag Coverage Δ
unittests 40.49% <100.00%> (+6.89%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Merged via the queue into microsoft:main with commit 1298875 May 24, 2024
76 of 90 checks passed
jayralencar pushed a commit to jayralencar/autogen that referenced this pull request May 28, 2024
…umps (microsoft#2714)

Co-authored-by: Jason <jtoy@grids.local>
Co-authored-by: Chi Wang <wang.chi@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: A problem found in the autogen.function_utils method
6 participants