Skip to content

feat(js): ai.countTokens support#5116

Open
ifielker wants to merge 2 commits intomainfrom
if-countTokens-2
Open

feat(js): ai.countTokens support#5116
ifielker wants to merge 2 commits intomainfrom
if-countTokens-2

Conversation

@ifielker
Copy link
Copy Markdown
Collaborator

Checklist (if applicable):

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a new countTokens API to the Genkit AI framework, allowing users to calculate token usage for generative models. Key changes include the addition of a countTokens function in generate.ts, integration into the GenkitAI class with multiple overloads, and updates to the model definition logic to support optional token counting actions. The PR also refactors core middleware to support non-generation outputs and updates the GenerationUsageSchema to deprecate the custom field in favor of raw. Review feedback suggests improving the robustness of the token counter lookup for dynamic models, ensuring background models also register their token counters, and refining the configuration handling when preparing token count requests. A simplification for handling PromiseLike options in the GenkitAI class was also recommended.

Comment on lines +844 to +849
const counterActionName = `/model-token-counter/${resolvedModel.modelAction.__action.name}`;
const counterAction = await registry.lookupAction<
typeof GenerateRequestSchema,
typeof GenerationUsageSchema,
TokenCounterAction<CustomOptions>
>(counterActionName);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

For dynamic models (created via the model() helper) that are not explicitly registered in the registry, lookupAction will fail. Since the ModelAction object already contains the __tokenCounterAction property when countTokens is provided, you should check this property first before attempting a registry lookup.

Suggested change
const counterActionName = `/model-token-counter/${resolvedModel.modelAction.__action.name}`;
const counterAction = await registry.lookupAction<
typeof GenerateRequestSchema,
typeof GenerationUsageSchema,
TokenCounterAction<CustomOptions>
>(counterActionName);
const counterAction =
resolvedModel.modelAction.__tokenCounterAction ??
(await registry.lookupAction<
typeof GenerateRequestSchema,
typeof GenerationUsageSchema,
TokenCounterAction<CustomOptions>
>(`/model-token-counter/${resolvedModel.modelAction.__action.name}`));

Comment on lines +768 to +774
if (isModelAction(resolvedAction) && resolvedAction.__tokenCounterAction) {
registry.registerAction(
'model-token-counter',
resolvedAction.__tokenCounterAction,
{ namespace: plugin.name }
);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

This logic only registers token counters for standard models. Background models also support token counting (as DefineBackgroundModelOptions extends DefineModelOptions), but their counters are not being registered here because they are handled in a separate block (line 752). Ensure that background models also have their __tokenCounterAction registered in the registry.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

There are currently no background models with token counting capability. We will add it when those models are available.

Comment on lines +839 to +842
request.config = {
...(resolvedModel.config || {}),
...(request.config || {}),
};
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The configuration logic here should ensure that the resolved model's configuration replaces the original request's configuration rather than merging with it, to ensure the model operates with its intended settings. Additionally, include the model version and use stripUndefinedOptions to ensure that undefined values don't cause issues.

Suggested change
request.config = {
...(resolvedModel.config || {}),
...(request.config || {}),
};
request.config = {
version: resolvedModel.version,
...stripUndefinedOptions(resolvedModel.config),
};
References
  1. By default, a fallback model's configuration should replace the original request's configuration, not merge with it. This ensures that the fallback operates with its intended, specific configuration.

Comment on lines +348 to +356
if (options instanceof Promise) {
resolvedOptions = await options;
} else if (typeof options === 'string' || Array.isArray(options)) {
resolvedOptions = {
prompt: options,
};
} else {
resolvedOptions = options as GenerateOptions<O, CustomOptions>;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The instanceof Promise check is unreliable for PromiseLike objects (thenables) and adds unnecessary complexity. Since the underlying countTokens function already handles PromiseLike arguments, you can simplify this implementation to match the pattern used in generateStream.

  async countTokens<
    O extends z.ZodTypeAny = z.ZodTypeAny,
    CustomOptions extends z.ZodTypeAny = typeof GenerationCommonConfigSchema,
  >(
    options:
      | string
      | Part[]
      | GenerateOptions<O, CustomOptions>
      | PromiseLike<GenerateOptions<O, CustomOptions>>
  ): Promise<GenerationUsage> {
    if (typeof options === 'string' || Array.isArray(options)) {
      return countTokens(this.registry, { prompt: options });
    }
    return countTokens(this.registry, options);
  }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant