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

resolve encoder for model #591

Merged
merged 2 commits into from
Jul 29, 2024
Merged

resolve encoder for model #591

merged 2 commits into from
Jul 29, 2024

Conversation

pelikhan
Copy link
Member

@pelikhan pelikhan commented Jul 27, 2024

  • Added a new function resolveTokenEncoder to obtain and return token encoders based on the model provided. This is an asynchronous function introduced in encoders.ts file inside the core package. 📑

  • Replaced the estimateTokens function from the tokens.ts file in the core package with estimateTokens from encoders.ts throughout the codebase. This essentially uses the new encoder for estimating tokens. 🔍

  • Updated createParsers function in parsers.ts to be asynchronous and wait for the token encoder. ⏳

  • Resolving and creating the prompt context in some files is also updated to be asynchronous, to ensure the token encoder is properly resolved. 🔄

  • In tokens.ts, removed the estimateChatTokens function as its responsibility is now taken up by a new function estimateChatTokens introduced in the newly added chatencoder.ts. 🆕

  • A new file chatencoder.ts has been added to the core package that contains its own implementation of estimateChatTokens function. 📁

  • The user-facing interface (as defined in the prompt_template.d.ts file) has not changed, meaning these changes are mostly internal and do not affect users directly. 👥

generated by pr-describe

Copy link

The changes seem to be part of a refactoring process to enhance the token estimation functionality. The estimateTokens function has been modified to use a new TokenEncoder type which is acquired via the resolveTokenEncoder function. This is a significant improvement over the previous model-specific approach, enabling more flexibility and simplifying token estimation across different models.

However, there's one concern:

  1. Even though the functionality seems to be improved, the changes are quite pervasive. This could potentially introduce bugs if not thoroughly tested. It would be better if the changes were introduced incrementally, allowing the codebase to gradually adapt to the new TokenEncoder approach.

For these reasons, I would say that the PR needs some work before it can be merged. 👷

Please make sure to test the changes thoroughly before proceeding, especially in the areas affected by the refactoring.

generated by pr-review

@pelikhan pelikhan merged commit 6fc2085 into main Jul 29, 2024
9 checks passed
@pelikhan pelikhan deleted the encoderresolution branch July 29, 2024 06:14
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.

1 participant