Skip to content

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

Merged
merged 5 commits into from
Aug 22, 2025

Conversation

besaleli
Copy link
Contributor

@besaleli besaleli commented Aug 18, 2025

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

  • I have added unit tests that prove my fix/feature works
  • New and existing tests pass locally
  • Documentation was updated where necessary
  • I have read and followed the contribution guidelines

@besaleli besaleli linked an issue Aug 18, 2025 that may be closed by this pull request
Copy link

codecov bot commented Aug 18, 2025

Codecov Report

❌ Patch coverage is 20.00000% with 28 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/any_llm/providers/together/together.py 21.73% 18 Missing ⚠️
src/any_llm/providers/together/utils.py 16.66% 10 Missing ⚠️
Files with missing lines Coverage Δ
src/any_llm/providers/together/utils.py 17.07% <16.66%> (-2.93%) ⬇️
src/any_llm/providers/together/together.py 40.98% <21.73%> (-3.25%) ⬇️

... and 26 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@besaleli besaleli requested review from njbrake and daavoo August 18, 2025 11:34
njbrake
njbrake previously approved these changes Aug 18, 2025
Comment on lines 215 to 217
data = response.model_dump()
choices_out: list[Choice] = []
for i, ch in enumerate(data.get("choices", [])):
Copy link
Contributor

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

Copy link
Contributor

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?

@anivar
Copy link
Contributor

anivar commented Aug 18, 2025

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!

@besaleli
Copy link
Contributor Author

Hey @anivar, thanks for the comment. Feel free to fork off this branch (217-migrate-together-provider-to-async) and submit a PR to merge onto this branch if you want to take care of the utils refactor. We will keep this PR as the source of truth for the final enhancement integration.

@anivar
Copy link
Contributor

anivar commented Aug 20, 2025

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!

Copy link
Contributor

@njbrake njbrake left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@besaleli besaleli marked this pull request as ready for review August 22, 2025 13:36
@besaleli besaleli merged commit c1f23b7 into main Aug 22, 2025
9 of 10 checks passed
@besaleli besaleli deleted the 217-migrate-together-provider-to-async branch August 22, 2025 13:38
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.

Migrate together provider to async
3 participants