-
Notifications
You must be signed in to change notification settings - Fork 61
feat(async): Migrate Together.ai provider to async #247
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
Conversation
Codecov Report❌ Patch coverage is
... and 26 files with indirect coverage changes 🚀 New features to boost your workflow:
|
data = response.model_dump() | ||
choices_out: list[Choice] = [] | ||
for i, ch in enumerate(data.get("choices", [])): |
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 is 10000% scope creep but if you feel like migrating this to the utils file, that would probably be a good improvement
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.
I actually implemented this suggestion while working on the same issue. The duplicated code (lines 215-249) can be extracted to a to_chat_completion utility function in utils.py. This would make both methods cleaner and easier to maintain. Would you like me to push a commit with this refactoring to your branch?
I noticed the response conversion logic is duplicated between completion and acompletion methods. Consider extracting it to a utility function to reduce duplication. Happy to help if needed! |
Hey @anivar, thanks for the comment. Feel free to fork off this branch ( |
Thanks @besaleli! Since I can't push directly to your branch, I've created PR #272 with the utility function refactoring as suggested by @njbrake. The refactor extracts the duplicated response conversion logic and adds the missing reasoning_effort handling for consistency. Feel free to merge it into your branch if it looks good! |
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.
LGTM 🚀
Description
NOTE: Untested until I get my hands on a Together.ai API key 🥀 they rate limited me
PR Type: 🆕 New Feature
Relevant issues
Closes #217
Checklist