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

Allow the injection of TokenCountEstimator #705

Merged
merged 1 commit into from
Jun 27, 2024

Conversation

andreadimaio
Copy link
Contributor

@andreadimaio andreadimaio commented Jun 26, 2024

This PR resolves #697.

The idea is to allow the injection of the TokenCountEstimator interface, which can be used in different ways, for example to know in advance the cost of processing a certain text, or to choose one model or another based on the maximum number of tokens.

Today, when I look at the code to see which LLM providers implement this interface, I see

  • bam
  • watsonx
  • azure-openai

For the azure-openai I found something strange, the tokenizer variable is never set, so when creating tests I always get a NullPointerException(There are some TODOs in the code to understand what the points are).

As far as I understand, the correct way to create the tokenizer for azure-openai is to use something like:

new AzureOpenAiTokenizer(<modelId>)

but I'm not able to test this, so what I've done is just put some TODOs in the code.

Is there anyone who can look into this? Does it make sense to have a TokenCountEstimator for azure-openai or should I just ignore it for now?

@andreadimaio andreadimaio requested a review from a team as a code owner June 26, 2024 21:18
@geoand
Copy link
Collaborator

geoand commented Jun 27, 2024

Is there anyone who can look into this? Does it make sense to have a TokenCountEstimator for azure-openai or should I just ignore it for now?

Maybe @csotiriou knows :)

Copy link
Collaborator

@geoand geoand left a comment

Choose a reason for hiding this comment

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

Very nice!

@geoand geoand merged commit ed9420e into quarkiverse:main Jun 27, 2024
12 checks passed
@andreadimaio andreadimaio deleted the token_count_estimator branch June 27, 2024 06:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow the injection of TokenCountEstimator
2 participants