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

community: Add Naver chat model & embeddings #25162

Merged
merged 63 commits into from
Oct 24, 2024

Conversation

hyper-clova
Copy link
Contributor

@hyper-clova hyper-clova commented Aug 8, 2024

Reopened as a personal repo outside the organization.

Description

Twitter handle: None. (if needed, contact with [email protected])


you can check our previous discussion below:

one question on namespaces - would it make sense to have these in .clova namespaces instead of .naver?

I would like to keep it as is, unless it is essential to unify the package name.
(ClovaX is a branding for the model, and I plan to add other models and components. They need to be managed as separate classes.)

also, could you clarify the difference between ClovaEmbeddings and ClovaXEmbeddings?

There are 3 models that are being serviced by embedding, and all are supported in the current PR. In addition, all the functionality of CLOVA Studio that serves actual models, such as distinguishing between test apps and service apps, is supported. The existing PR does not support this content because it is hard-coded.

Copy link

vercel bot commented Aug 8, 2024

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

Name Status Preview Comments Updated (UTC)
langchain ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 24, 2024 1:23am

@hyper-clova
Copy link
Contributor Author

Hello @efriis,

We've been preparing for this community package and waiting for review (following up our prior communication on early August via email), and I think we are all ready to go!

Could you check this PR?

@efriis
Copy link
Member

efriis commented Aug 29, 2024

Thanks for the ping! Could you get CI passing?

@hyper-clova
Copy link
Contributor Author

Thanks @efriis , seems we referred to some outdated docs, got failed linting test.
Now fixed!

@hyper-clova
Copy link
Contributor Author

Hi @vbarda , just found out that assignee to this pr has changed to you. Looking forward to cooperate!

It seems there was the only lint fail due to the updates in poetry.lock file for the recent test and now fixed.
Think we are almost there, but if there is any task of change needed, don't hesitate to reach out and let us know! :)

@vbarda
Copy link
Contributor

vbarda commented Oct 22, 2024

@hyper-clova hey folks -- the change looks good overall! i added a couple of small comments / suggestions. i think we're very close to merging this!

@dosubot dosubot bot added the lgtm PR looks good. Use to confirm that a PR is ready for merging. label Oct 24, 2024
@vbarda vbarda enabled auto-merge (squash) October 24, 2024 20:50
@vbarda vbarda merged commit 846a752 into langchain-ai:master Oct 24, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Related to langchain-community Ɑ: embeddings Related to text embedding models module 🤖:enhancement A large net-new component, integration, or chain. Use sparingly. The largest features lgtm PR looks good. Use to confirm that a PR is ready for merging. size:XXL This PR changes 1000+ lines, ignoring generated files.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants