-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Update map_finish_reason #7264
base: main
Are you sure you want to change the base?
Update map_finish_reason #7264
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@ishaan-jaff updated this function |
"SPII": "content_filter", | ||
} | ||
|
||
return finish_reason_mapping.get(finish_reason, finish_reason) |
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.
would this return None if unmapped?
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.
This returns the finish_reason
if unmapped, just like the original definition.
The second value of get
provides the default value if the key does not exist
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.
nice - can we add a unit test for this, maybe here -
from datetime import datetime |
if you can share a screenshot of it passing, this pr should be good to merge!
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.
Done @krrishdholakia.
@krrishdholakia Ready for merge :) |
Title
Update mapping helper function that converts from api specific finish reasons to a standard set
Type
🧹 Refactoring
📖 Documentation
Changes
Documented the source
Made it clearer what the mappings are