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

feat(google-common): Support get num tokens #7818

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hans00
Copy link

@hans00 hans00 commented Mar 9, 2025

Support getNumTokens for Google LLM / chat LLM

@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Mar 9, 2025
Copy link

vercel bot commented Mar 9, 2025

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

Name Status Preview Comments Updated (UTC)
langchainjs-docs ✅ Ready (Inspect) Visit Preview Mar 9, 2025 6:42pm
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
langchainjs-api-refs ⬜️ Ignored (Inspect) Mar 9, 2025 6:42pm

@dosubot dosubot bot added the auto:improvement Medium size change to existing code to handle new use-cases label Mar 9, 2025
@afirstenberg
Copy link
Contributor

We didn't support this before? Ooof. Good catch!

A few thoughts, however.

  1. This only has an implementation on the connection object.
    • This object is lower level and typically not used by developers.
    • The connection object is somewhat abstract. It doesn't have specific methods or implementations attached to it.
  2. The definition of getNumTokens() is on BaseLanguageModelInterface with a definition in BaseLanguageModel, so we should implement it in ChatGoogleBase and GoogleBaseLLM
    • It would make sense that both of these would create and call a GoogleNumTokensConnection or something along those lines that would subclass GoogleAIConnection (or maybe AbstractGoogleLLMConnection) which would define the method for buildUrlMethod() to "countTokens". I'm not sure what, if anything, can be done for Claude or any future API.

Does this make sense @hans00?
Thoughts @benjamincburns?

@benjamincburns
Copy link
Collaborator

benjamincburns commented Mar 10, 2025

We didn't support this before? Ooof. Good catch!

Agreed - thanks for this @hans00!

@afirstenberg I defer to your expertise on the design here. Just for my own education, though - it looks like countTokens is only relevant for LLMs (not text embeddings), so it makes sense to me to move it out of GoogleConnection, as that's used for embeddings and LLMs. The bit I can't answer for myself however, is why it wouldn't be appropriate to add something to call the countTokens method directly to AbstractGoogleLLMConnection. That would be reachable via the connection classes that service both GoogleChatBase and GoogleBaseLLM, but wouldn't expose it to BaseGoogleEmbeddings.

Probably a heavier lift, but Google's recommendation appears to be to use the tokenizer that ships with the Python SDK. I don't suppose there's a JS port for that, is there? If so, it'd be relatively straightforward to set it (or some function that wraps it) as the _encoder field of GoogleChatBase and GoogleBaseLLM. In theory that would work with the existing machinery in BaseLanguageModel and require even less change.

@benjamincburns
Copy link
Collaborator

This came up in a quick search - doesn't support gemini-1.0-pro-vision, nor does it list support for any of the gemini-2.0 models (although I'm not sure if that's because they're using a new tokenizer for those, or because they haven't updated the docs).

https://www.npmjs.com/package/@lenml/tokenizer-gemini

Copy link
Collaborator

@jacoblee93 jacoblee93 left a comment

Choose a reason for hiding this comment

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

.

@dosubot dosubot bot added the lgtm PRs that are ready to be merged as-is label Mar 11, 2025
@jacoblee93 jacoblee93 added hold On hold and removed lgtm PRs that are ready to be merged as-is labels Mar 11, 2025
Copy link
Collaborator

@benjamincburns benjamincburns left a comment

Choose a reason for hiding this comment

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

Sorry for the thrashing, but this needs some revision before it can be merged. See discussion on the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto:improvement Medium size change to existing code to handle new use-cases hold On hold size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants