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

together[minor]: Update endpoint to non deprecated version #19649

Merged
merged 6 commits into from Mar 31, 2024

Conversation

dev-yashmathur
Copy link
Contributor

  • Updating Together.ai Endpoint: "langchain_together: Updated Deprecated endpoint for partner package"

  • Description: The inference API of together is deprecates, do replaced with completions and made corresponding changes.

  • Twitter handle: @dev_yashmathur

@efriis efriis added the partner label Mar 27, 2024
@efriis efriis self-assigned this Mar 27, 2024
@dosubot dosubot bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Mar 27, 2024
Copy link

vercel bot commented Mar 27, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
langchain ⬜️ Ignored (Inspect) Visit Preview Mar 31, 2024 9:20pm

@dosubot dosubot bot added the 🤖:improvement Medium size change to existing code to handle new use-cases label Mar 27, 2024
together_api_key: SecretStr
"""Together AI API key. Get it here: https://api.together.xyz/settings/api-keys"""
model: str
"""Model name. Available models listed here:
https://docs.together.ai/docs/inference-models
Base Models: https://docs.together.ai/docs/inference-models#language-models
Chat Models: https://docs.together.ai/docs/inference-models#chat-models
Copy link
Contributor

Choose a reason for hiding this comment

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

@dev-yashmathur max_tokens is now a required parameter (https://docs.together.ai/reference/completions).

also as a matter of style, I don't think these changes are minor :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @sepiatone , appreciate the feedback. I seem to have missed that, as even without it; i think a default of 20 is being set.
I have added a new commit which should take this into consideration.
Looking forward to any other feedback. Thanks :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be simpler to do this:
max_tokens: Optional[int] = None to
max_tokens: int = 128

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @sepiatone . While that is the simpler solution, I have opted for this approach so as to inform the users of the requirement. Just setting it ourselves, would not include the warning. If you think that the warning can be skipped, then I'll make this change as well.

Copy link
Contributor

@sepiatone sepiatone Mar 30, 2024

Choose a reason for hiding this comment

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

@dev-yashmathur I checked the implementation of a few other models and we don't usually give a warning, that said, I'm ok with it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sepiatone So, can this PR be merged, or would you like me to make the modification?

P.S. I appreciate you taking the time to help me out with this :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@dev-yashmathur lgtm! now the actual maintainers of the langchain-together package, @baskaryan & @efriis will have to be satisfied :)

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Mar 28, 2024
@dosubot dosubot bot added the lgtm PR looks good. Use to confirm that a PR is ready for merging. label Mar 31, 2024
@baskaryan baskaryan enabled auto-merge (squash) March 31, 2024 21:20
@baskaryan baskaryan merged commit c42ec58 into langchain-ai:master Mar 31, 2024
21 checks passed
marlenezw pushed a commit to marlenezw/langchain that referenced this pull request Apr 2, 2024
…-ai#19649)

- **Updating Together.ai Endpoint**: "langchain_together: Updated
Deprecated endpoint for partner package"

- Description: The inference API of together is deprecates, do replaced
with completions and made corresponding changes.
- Twitter handle: @dev_yashmathur

---------

Co-authored-by: Bagatur <22008038+baskaryan@users.noreply.github.com>
Co-authored-by: Bagatur <baskaryan@gmail.com>
hinthornw pushed a commit that referenced this pull request Apr 26, 2024
- **Updating Together.ai Endpoint**: "langchain_together: Updated
Deprecated endpoint for partner package"

- Description: The inference API of together is deprecates, do replaced
with completions and made corresponding changes.
- Twitter handle: @dev_yashmathur

---------

Co-authored-by: Bagatur <22008038+baskaryan@users.noreply.github.com>
Co-authored-by: Bagatur <baskaryan@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤖:improvement Medium size change to existing code to handle new use-cases lgtm PR looks good. Use to confirm that a PR is ready for merging. partner size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants