-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Improve message trimming #787
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great ! thanks for the contribution
if key == "name": | ||
num_tokens += tokens_per_name | ||
num_tokens += 3 # every reply is primed with <|start|>assistant<|message|> | ||
return num_tokens |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we just have one token counter ? Can you add these changes to token_counter
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just requesting this one change ^
unless there's a strong reason to have two token counters ? @duc-phamh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The openai_token_counter
recursively calls itself. Of course we can refactor it into one token counter, but leaving the OpenAI code separate like this would make it easier to apply future changes in case OpenAI update this function. Therefore, I believe it's better to leave it this way.
Please let me know if you think otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me
@duc-phamh going to wait until we can get a new release of our existing code base and then merge this |
Thank you, we really look forward to it. Our production relies on this, and occasionally we'd have error with message generation without the fix here. |
Changes:
process_system_message
.process_messages
.These changes should resolve issue #742, along with some other edge cases as in the test file.