-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
[Bugfix][Frontend] Cleanup "fix chat logprobs" #5026
Merged
Merged
Changes from all commits
Commits
Show all changes
21 commits
Select commit
Hold shift + click to select a range
2e19b90
Fix logprobs for chat completion API
DarkLight1337 08e41d7
Update and fix tests
DarkLight1337 bbd4415
Fix and refine tests
DarkLight1337 504dd49
Fix incorrect parameters to `_create_chat_logprobs`
DarkLight1337 390e93d
Allow `logprobs=True` when `top_logprobs=0` or `top_logprobs=None` (#…
DarkLight1337 cbed5ec
Refine tests and fix them
DarkLight1337 518ff5f
Merge branch 'upstream' into openai-logprobs
DarkLight1337 a72b33c
Use stricter test for Chat Completions API
DarkLight1337 d18287a
Merge branch 'upstream' into openai-logprobs
DarkLight1337 4cb9068
Merge branch 'upstream' into openai-logprobs
DarkLight1337 5ed37cd
Update tests
DarkLight1337 edeb3f6
Apply formatter
DarkLight1337 6584a51
Remove unused typevar
DarkLight1337 2b6b3d8
Remove unnecessary disable
DarkLight1337 fcf4d6f
Update `test_single_chat_session`
DarkLight1337 fed335b
Use strict equality tests for length, and remove unnecessary non-null…
DarkLight1337 ecef584
Revert use strict equality tests
DarkLight1337 72d58e1
Fix bad types caused by reassignment of same variable
DarkLight1337 8226295
Merge branch 'upstream' into openai-logprobs
DarkLight1337 908cac4
Fix confusing assertion and variable name
DarkLight1337 7de6cf7
Merge branch 'upstream' into openai-logprobs
DarkLight1337 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit : since we are doing a more stricter check about completion.usage (L:174) wonder if we can have a more strict equality check in the len(choice.text) for consistency here and in other places?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that we can't check the length of the string using strict equality as each token may correspond to multiple characters. I'll keep the range check then.