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

Updated EmbeddingsCohere node to include latest Cohere embeddings V3 #9887

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

KhDu
Copy link

@KhDu KhDu commented Jun 27, 2024

Summary

Just a simple PR updating the EmbeddingsCohere node to include the latest models for generating text embeddings. It adds new model options (Cohere Embedding V3 English + Multilang...).

Changes made:

  • Added new model options: embed-english-v3.0, embed-english-light-v3.0, embed-multilingual-v3.0, embed-multilingual-light-v3.0.
  • Updated the supplyData method to use the new models and API format.

How to test:

  1. Configure the EmbeddingsCohere node with appropriate credentials.
  2. Select one of the new models from the dropdown menu.
  3. Execute the node and verify that embeddings are generated correctly.

Related Linear tickets, Github issues, and Community forum posts

  • None that I am aware of, just noticed it on the node properties and quickly edited the code to change the model.

Review / Merge checklist

  • PR title and summary are descriptive. (conventions)
  • Docs updated or follow-up ticket created.
  • Tests included.
  • PR Labeled with release/backport (if the PR is an urgent fix that needs to be backported)

@CLAassistant
Copy link

CLAassistant commented Jun 27, 2024

CLA assistant check
All committers have signed the CLA.

@n8n-assistant n8n-assistant bot added community Authored by a community member in linear Issue or PR has been created in Linear for internal review labels Jun 27, 2024
@Joffcom
Copy link
Member

Joffcom commented Jun 27, 2024

Hey @KhDu,

Thanks for the PR, At a quick glance it looks like you have removed the v2 options which still appear to be valid so this would be a breaking change as the default value is being changed.

Instead of replacing v2 can you add more properties and include v3 that way making sure the default remains as v2 to prevent anything unexpected for existing users?

The documentation url should also point to our docs not cohere.

Did you also run the linter to make sure there were no issues before opening the PR?

@KhDu
Copy link
Author

KhDu commented Jun 28, 2024

Hey there @Joffcom,

I ran linter and got 7 warnings but 0 errors:

  • 47:32 warning Unsafe member access .AiEmbedding on an any value @typescript-eslint/no-unsafe-member-access
  • 50:53 warning Unsafe member access .AiVectorStore on an any value @typescript-eslint/no-unsafe-member-access
  • 100:23 warning Unsafe member access .logger on an any value @typescript-eslint/no-unsafe-member-access
  • 103:21 warning Unsafe call of an any typed value @typescript-eslint/no-unsafe-call
  • 103:26 warning Unsafe member access .getNodeParameter on an any value @typescript-eslint/no-unsafe-member-access
  • 104:30 warning Unsafe call of an any typed value @typescript-eslint/no-unsafe-call
  • 104:35 warning Unsafe member access .getCredentials on an any value @typescript-eslint/no-unsafe-member-access

@KhDu
Copy link
Author

KhDu commented Jun 28, 2024

Just a quick fix. I’m going to send another commit today to clean up and restore it to the source formatting. (It looks like my IDE auto removed comments…etc)

@KhDu
Copy link
Author

KhDu commented Jun 28, 2024

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Authored by a community member in linear Issue or PR has been created in Linear for internal review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants