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

[Core] Fix update_usage_summary when response doesn't have usage attribute #1008

Merged
merged 15 commits into from
Jan 4, 2024

Conversation

yiranwu0
Copy link
Collaborator

Why are these changes needed?

Related issue number

Close #1002 #984

Checks

@codecov-commenter
Copy link

codecov-commenter commented Dec 18, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (1508f53) 30.82% compared to head (27a04b2) 64.43%.

Files Patch % Lines
autogen/oai/client.py 60.00% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1008       +/-   ##
===========================================
+ Coverage   30.82%   64.43%   +33.61%     
===========================================
  Files          30       30               
  Lines        4033     4043       +10     
  Branches      915      964       +49     
===========================================
+ Hits         1243     2605     +1362     
+ Misses       2711     1167     -1544     
- Partials       79      271      +192     
Flag Coverage Δ
unittests 64.33% <60.00%> (+33.56%) ⬆️

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.

@qingyun-wu
Copy link
Collaborator

@kevin666aa The PR LGTM overeall. Have you tested if this sufficiently address the issue raised in #1002 and #984. Can they be closed with this PR? Is it possible to add a test here?

@yiranwu0 yiranwu0 changed the title Fix update_usage_summary when response doesn't have usage attrixbut Fix update_usage_summary when response doesn't have usage attribute Dec 18, 2023
@yiranwu0
Copy link
Collaborator Author

yiranwu0 commented Dec 18, 2023

@kevin666aa The PR LGTM overeall. Have you tested if this sufficiently address the issue raised in #1002 and #984. Can they be closed with this PR? Is it possible to add a test here?

I cannot replicate the error, so I can't put down a test for this. The bug is happening because response.usage may be a nonetype due to the use of a local model. The code I added in the try catch block would make sure that response.usage can be referenced, and it is not none type. Further checking of response.prompt_tokens is also added. So I think it would be sure to resolve the two issues.

I can ask people to test it out to see if it works.

@ekzhu ekzhu changed the title Fix update_usage_summary when response doesn't have usage attribute [Core] Fix update_usage_summary when response doesn't have usage attribute Dec 26, 2023
@sonichi
Copy link
Collaborator

sonichi commented Dec 27, 2023

openai 1.5 support has been added by #1044 + #1071
There is no need to limit the openai version in this PR.

@ekzhu
Copy link
Collaborator

ekzhu commented Dec 28, 2023

We can merge this once the conflict has been resolved. @kevin666aa

@ekzhu
Copy link
Collaborator

ekzhu commented Dec 29, 2023

@kevin666aa failing tests.

@yiranwu0
Copy link
Collaborator Author

@kevin666aa failing tests.

@kevin666aa failing tests.

Hello @ekzhu, can you help me run the openai test again?

@sonichi
Copy link
Collaborator

sonichi commented Dec 31, 2023

#1110 needs to be merged first to prevent test failures.

@sonichi sonichi added this pull request to the merge queue Jan 4, 2024
Merged via the queue into main with commit 6bf33df Jan 4, 2024
84 checks passed
@qingyun-wu qingyun-wu deleted the fixcost branch February 13, 2024 16:53
whiskyboy pushed a commit to whiskyboy/autogen that referenced this pull request Apr 17, 2024
…osoft#1008)

* update max_spark_parallelism to fit in auto-scale spark cluster

* update test
whiskyboy pushed a commit to whiskyboy/autogen that referenced this pull request Apr 17, 2024
…ibute (microsoft#1008)

* update cost for cache

* fix test

* fix bug when reponse doesn't have usage info

* update setupversion

* update

* update

* Update setup.py

Co-authored-by: Chi Wang <wang.chi@microsoft.com>

---------

Co-authored-by: Qingyun Wu <qingyun.wu@psu.edu>
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
5 participants