Skip to content

Fix R1 reasoning parser for openai client - #5961

Merged
Eric Zhu (ekzhu) merged 7 commits into
microsoft:mainfrom
ZakWork:R1_reasoning_parser_for_openai_client
Mar 17, 2025
Merged

Eric Zhu (ekzhu) merged 7 commits into
microsoft:mainfrom
ZakWork:R1_reasoning_parser_for_openai_client

Conversation

@ZakWork

@ZakWork Zakrea (ZakWork) commented Mar 16, 2025 •

Copy link
Copy Markdown
Contributor

Hey Eric Zhu (@ekzhu), can you please have a look at this. This resolves the issues with parsing of the R1 reasoning tokens at least for openai client there are still areas where it still uses the old parser which need to be look at in the future. This doesn't fully resolve all the issues mentioned previously however the parsing of reasoning tokens should now be resolved.

Why are these changes needed?

R1 reasoning tokens were not parsed correctly for the openai client

Related issue number

#5941

Checks

@ZakWork Zakrea (ZakWork) mentioned this pull request Mar 16, 2025
3 tasks
Comment thread python/packages/autogen-ext/src/autogen_ext/models/openai/_openai_client.py Outdated
Comment thread python/packages/autogen-ext/src/autogen_ext/models/openai/_openai_client.py Outdated
@ekzhu

Eric Zhu (ekzhu) commented Mar 16, 2025 •

Copy link
Copy Markdown
Contributor

For the 400 server error about alternating user-assistant roles in messages, let's use this issue :#5965

@ZakWork
Zakrea (ZakWork) force-pushed the R1_reasoning_parser_for_openai_client branch from f450a77 to e5cb30f Compare March 16, 2025 17:32
@ZakWork

Copy link
Copy Markdown
Contributor Author

Hey Eric, I have applied the requested changes if there is anything else let me know.

@ekzhu Eric Zhu (ekzhu) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a unit test, to validate that create and create_stream use reasoning_content when available.

See example here: https://github.com/microsoft/autogen/blob/main/python/packages/autogen-ext/tests/models/test_openai_model_client.py#L890-L891

@ekzhu

Copy link
Copy Markdown
Contributor

We can only merge the PR if you agree to the CLA, see automated comment above.

@ekzhu

Copy link
Copy Markdown
Contributor

Please read the python developer guide for formatting and type checking: https://github.com/microsoft/autogen/blob/main/python/README.md

cd python/packages/autogen-ext
poe pyright
poe mypy
poe format
poe lint

@codecov

codecov Bot commented Mar 16, 2025 •

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 80.95238% with 4 lines in your changes missing coverage. Please review.

Project coverage is 76.67%. Comparing base (e5ab7d5) to head (f9f48a0).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...xt/src/autogen_ext/models/openai/_openai_client.py 80.95% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5961      +/-   ##
==========================================
+ Coverage   76.65%   76.67%   +0.02%     
==========================================
  Files         191      191              
  Lines       13129    13141      +12     
==========================================
+ Hits        10064    10076      +12     
  Misses       3065     3065              
Flag Coverage Δ
unittests 76.67% <80.95%> (+0.02%) ⬆️

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.

🚀 New features to boost your workflow:
  • ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ZakWork

Copy link
Copy Markdown
Contributor Author

@microsoft-github-policy-service agree

@ZakWork

Copy link
Copy Markdown
Contributor Author

Hey Eric Zhu (@ekzhu) can you please run the format check, just want to see if that fixes it, I am not using uv atm, so I got the Ruff extension and applied that but it changed quite a bit on the file formatting.

Added tests for the handling of reasoning_content for R1 model
@ZakWork

Copy link
Copy Markdown
Contributor Author

Tests added and formatting should be fixed but let me know if there is anything else

@ekzhu

Copy link
Copy Markdown
Contributor

Thanks. There is a few formatting and type checking issues remaining.

Have you tried to use uv to set up the development environment?

@ZakWork

Zakrea (ZakWork) commented Mar 17, 2025 •

Copy link
Copy Markdown
Contributor Author

The type checking issues are interesting because they relate to reasoning_content not being defined in ChatCompletionMessage (For the test file only as we are basically trying to emulate deepseek's api) which it probably thinks is a problem but this is not the case when using Pydantic as the reasoning_content data is added to model_extra and that is what we are using. So my thought and I don't like doing this but is either a comment on the code # type: ignore[call-arg] to say ignore type check for these two lines or leave as is and skip the test. Let me know what you think about this and if you have any ideas to fix it.

@ZakWork

Copy link
Copy Markdown
Contributor Author

Okay format issues should (third time lucky) be fixed. I got uv and poe running locally and fixed the formatting issues.

@ekzhu Eric Zhu (ekzhu) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

y26s4824k264, please continue your PR from this one once merged.

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.

ERROR when using deepseek r1

2 participants