Skip to content

New Feat: Added Embedding Callbacks #1859

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

Open
wants to merge 26 commits into
base: main
Choose a base branch
from

Conversation

ppramesi
Copy link
Contributor

@ppramesi ppramesi commented Jul 4, 2023

Added new callbacks:

handleEmbeddingStart: called before embedding is called. It passes serialized embedding class instance and array of texts to the callback function. It passes an array of texts because embedDocuments accepts multiple documents at once. As such, it also passes the single piece of text from embedQuery as an array with length 1.
handleEmbeddingEnd: called after embedding. It passes an array of vectors to the callback function.
handleEmbeddingError: called if there's an error during embed call.

Moreover, there are also some changes that needed to be made to implement these:

Changed Embeddings class such that now child classes implements _embedDocuments and _embedQuery functions since embedDocuments and embedQuery calls handle the callback calls.

Embedding classes are serializable, and added their secrets and aliases.

Moved BaseRetriever into its own file from schema to prevent circular dependency.

Added unit tests for the new callback.

@vercel
Copy link

vercel bot commented Jul 4, 2023

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

Name Status Preview Updated (UTC)
langchainjs-docs ✅ Ready (Inspect) Visit Preview Aug 13, 2023 8:37am

@nfcampos nfcampos self-requested a review July 5, 2023 08:53
@nfcampos nfcampos added the close PRs that need one or two touch-ups to be ready label Jul 5, 2023

// TODO Implement handleAgentEnd, handleText

// onAgentEnd?(run: ChainRun): void | Promise<void>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove these comments, this has been done

@@ -0,0 +1,12 @@
import { Callbacks } from "../callbacks/manager.js";
Copy link
Collaborator

Choose a reason for hiding this comment

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

We actually would want this file to be moved to schema/retriever.ts

@jacoblee93
Copy link
Collaborator

jacoblee93 commented Jul 7, 2023

Looks solid to me - I know we're waiting on some other factors but excited to have this merged!

@jacoblee93 jacoblee93 added the hold On hold label Jul 7, 2023
@ppramesi
Copy link
Contributor Author

ppramesi commented Jul 9, 2023

I see that there's a new commit that changes how callback manager works (e.g. adding metadata, use BaseCallbackConfig, etc). Should I merge those changes to this pull request or should I wait?

@the-powerpointer
Copy link
Contributor

@ppramesi Would it be possible to include not only the vector into the callback payload, but also the token usage? If in doubt, just the complete API response?
I'm only working with OpenAI models at the moment, there the token usage is returned with the API call. I don't know if other embedding models have this too.
However, it would be nice if we could get from the API call not only the pure result, but also the token usage. Having this transported via the callbacks allows the embedding models methods to stay streamlined with their signature, only retrieving the vectors.

@ppramesi
Copy link
Contributor Author

@the-powerpointer hmmm I'll have to take a look at the code first. I'll get back to you as soon as I can.

@mprytoluk
Copy link

Any news on this? Tracking embeddings token usage is very important, and this PR seems like the viable way to it.

@jacoblee93
Copy link
Collaborator

Still delayed unfortunately - will reraise internally!

@techwizzdom
Copy link

I was just investigating the usage of embedding models, happy to found this PR. Hope you'll merge it soon, this is much needed 🙏🏼

@DoKoB0512
Copy link

Would be great if this can be merged. We are currently keep track of each embedding done by different models so if we can somehow get the usage from the vendor itself then it would be amazing.

@msalafia
Copy link

msalafia commented Jan 24, 2025

Waiting this feature so much. But why token usage is not included in the embedding result and just the vectors are returned?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
close PRs that need one or two touch-ups to be ready hold On hold
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants