Skip to content

Conversation

gabe-l-hart
Copy link
Collaborator

Description

This is a follow up to #16110 to fix the issue of the model not terminating correctly. It turned out to be a few lingering bugs in the tokenization:

  1. A duplicate <fake_token_around_image> before the first image slice
  2. Only a single \n before the start of the global image instead of a double newline
  3. The granite template implementation in llama-chat.cpp had an errant \n at the end when adding the assistant generation prompt

I'm pretty sure (3) was the main cause of the problem, but all three are fixed here. For reference, here are the chat templates for the various models that use the granite chat template:

Branch: GraniteDoclingStopping

Signed-off-by: Gabe Goodhart <[email protected]>
Branch: GraniteDoclingStopping

Signed-off-by: Gabe Goodhart <[email protected]>
… prompt

There should not be one, even for the language models.

Branch: GraniteDoclingStopping

Signed-off-by: Gabe Goodhart <[email protected]>
Copy link
Collaborator

@CISC CISC left a comment

Choose a reason for hiding this comment

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

You need to update the test as well (where interestingly no-one caught the additional \n at the end:

{
/* .name= */ "ibm-granite/granite-3.0-8b-instruct",
/* .template_str= */ "{%- if tools %}\n {{- '<|start_of_role|>available_tools<|end_of_role|>\n' }}\n {%- for tool in tools %}\n {{- tool | tojson(indent=4) }}\n {%- if not loop.last %}\n {{- '\n\n' }}\n {%- endif %}\n {%- endfor %}\n {{- '<|end_of_text|>\n' }}\n{%- endif %}\n{%- for message in messages %}\n {%- if message['role'] == 'system' %}\n {{- '<|start_of_role|>system<|end_of_role|>' + message['content'] + '<|end_of_text|>\n' }}\n {%- elif message['role'] == 'user' %}\n {{- '<|start_of_role|>user<|end_of_role|>' + message['content'] + '<|end_of_text|>\n' }}\n {%- elif message['role'] == 'assistant' %}\n {{- '<|start_of_role|>assistant<|end_of_role|>' + message['content'] + '<|end_of_text|>\n' }}\n {%- elif message['role'] == 'assistant_tool_call' %}\n {{- '<|start_of_role|>assistant<|end_of_role|><|tool_call|>' + message['content'] + '<|end_of_text|>\n' }}\n {%- elif message['role'] == 'tool_response' %}\n {{- '<|start_of_role|>tool_response<|end_of_role|>' + message['content'] + '<|end_of_text|>\n' }}\n {%- endif %}\n {%- if loop.last and add_generation_prompt %}\n {{- '<|start_of_role|>assistant<|end_of_role|>' }}\n {%- endif %}\n{%- endfor %}",
/* .expected_output= */ "<|start_of_role|>system<|end_of_role|>You are a helpful assistant<|end_of_text|>\n<|start_of_role|>user<|end_of_role|>Hello<|end_of_text|>\n<|start_of_role|>assistant<|end_of_role|>Hi there<|end_of_text|>\n<|start_of_role|>user<|end_of_role|>Who are you<|end_of_text|>\n<|start_of_role|>assistant<|end_of_role|> I am an assistant <|end_of_text|>\n<|start_of_role|>user<|end_of_role|>Another question<|end_of_text|>\n<|start_of_role|>assistant<|end_of_role|>\n",
/* .expected_output_jinja= */ "<|start_of_role|>system<|end_of_role|>You are a helpful assistant<|end_of_text|>\n<|start_of_role|>user<|end_of_role|>Hello<|end_of_text|>\n<|start_of_role|>assistant<|end_of_role|>Hi there<|end_of_text|>\n<|start_of_role|>user<|end_of_role|>Who are you<|end_of_text|>\n<|start_of_role|>assistant<|end_of_role|> I am an assistant <|end_of_text|>\n<|start_of_role|>user<|end_of_role|>Another question<|end_of_text|>\n<|start_of_role|>assistant<|end_of_role|>",
},

@tommarques56

This comment was marked as spam.

@ngxson
Copy link
Collaborator

ngxson commented Oct 6, 2025

The failed CI seems to be unrelated to the current PR

@CISC
Copy link
Collaborator

CISC commented Oct 6, 2025

The failed CI seems to be unrelated to the current PR

They are definitely related, see my comment. :)

@ngxson
Copy link
Collaborator

ngxson commented Oct 6, 2025

Ah yeah ok I misread the CI output

@gabe-l-hart
Copy link
Collaborator Author

Yikes, thanks! I'll get that fixed asap

Branch: GraniteDoclingStopping

Signed-off-by: Gabe Goodhart <[email protected]>
@gabe-l-hart gabe-l-hart requested a review from ggerganov as a code owner October 6, 2025 13:21
@github-actions github-actions bot added the testing Everything test related label Oct 6, 2025
@gabe-l-hart
Copy link
Collaborator Author

@CISC @ngxson I think the tests should be in good shape now

@CISC CISC merged commit c08002a into ggml-org:master Oct 6, 2025
67 of 68 checks passed
@gabe-l-hart gabe-l-hart deleted the GraniteDoclingStopping branch October 7, 2025 02:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

examples testing Everything test related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants