Skip to content

Conversation

@ericcurtin
Copy link
Collaborator

Porting the fix from simple-chat.

Porting the fix from simple-chat.

Signed-off-by: Eric Curtin <[email protected]>
Comment on lines +736 to +737
if (llama_tokenize(vocab, prompt.c_str(), prompt.size(), prompt_tokens.data(), prompt_tokens.size(),
llama_get_kv_cache_used_cells(llama_data.context.get()) == 0, true) < 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (llama_tokenize(vocab, prompt.c_str(), prompt.size(), prompt_tokens.data(), prompt_tokens.size(),
llama_get_kv_cache_used_cells(llama_data.context.get()) == 0, true) < 0) {
if (llama_tokenize(vocab, prompt.c_str(), prompt.size(), prompt_tokens.data(), prompt_tokens.size(),
is_first, true) < 0) {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Following the simple-chat example, the first call to llama_tokenize uses this parameter:

is_first

and the second uses:

llama_get_kv_cache_used_cells(llama_data.context.get()) == 0

Copy link
Member

@ggerganov ggerganov Jan 20, 2025

Choose a reason for hiding this comment

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

Ah, I didn't even notice. Maybe this way it is better than introducing the is_first parameter.

Copy link
Collaborator Author

@ericcurtin ericcurtin Jan 20, 2025

Choose a reason for hiding this comment

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

Not pretending I know the details and just copying simple-chat :)

Copy link
Member

Choose a reason for hiding this comment

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

I pushed this variant to both simple-chat and run - seems much simpler than the is_first solution: #11311

@ericcurtin ericcurtin closed this Jan 20, 2025
@ericcurtin ericcurtin deleted the bos-run branch January 20, 2025 14:31
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.

3 participants