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

Fix Jinja2Cpp bug that broke system msg detection in templates #3325

Merged
merged 2 commits into from
Dec 19, 2024

Conversation

cebtenzzre
Copy link
Member

@cebtenzzre cebtenzzre commented Dec 19, 2024

Description

This PR fixes system message detection in the chat template of models such as Mistral Instruct which handle it specially. The bug is not always easy to reproduce, but it seems to occur most reliably on Windows. The symptom is that when trying to chat with Mistral Instruct with a system prompt set, you get this instead of a response:

Error: Failed to parse chat template: 1:1: error: Unexpected exception occurred during template processing. Exception: Jinja template error: After the optional system message, conversation roles must alternate user/assistant/user/assistant/...

Details

The prompt template in question:

{%- if messages[0]['role'] == 'system' %}
    {%- set system_message = messages[0]['content'] %}
    {%- set loop_start = 1 %}
{%- else %}
    {%- set loop_start = 0 %}
{%- endif %}
{%- for message in messages %}
    {%- if loop.index0 >= loop_start %}
        {%- if (message['role'] == 'user') != ((loop.index0 - loop_start) % 2 == 0) %}
            {{- raise_exception('After the optional system message, conversation roles must alternate user/assistant/user/assistant/...') }}
        {%- endif %}
        # ... message processing omitted for brevity ...
    {%- endif %}
{%- endfor %}

With Mistral Instruct, the Jinja2Cpp bug (see below) manifested as it missing the system prompt at messages[0], but then finding it as the first message in the {%- for message in messages %} loop, which caused it to call raise_exception as it was expecting it to be a user message instead.

The Fix

From the commit I added to the submodule:

template_parser: fix failure to clear errno before strtoll

This caused chaos if errno was ERANGE while calling Load +
RenderAsString and parsing a template with integer literals in
subscripts (e.g. messages[0]['content']).

The integer literals would parse as floating point numbers, which
ListAdapter doesn't implement subscript for, and all subscripts with
constant integers would return EmptyValue.

@cebtenzzre cebtenzzre changed the title jinja2cpp: update submodule for integer subscript fix Fix Jinja2Cpp bug that broke system msg detection in templates Dec 19, 2024
Signed-off-by: Jared Van Bortel <[email protected]>
@cebtenzzre cebtenzzre marked this pull request as ready for review December 19, 2024 17:11
@cebtenzzre cebtenzzre requested a review from manyoso December 19, 2024 17:11
@manyoso manyoso merged commit 3819842 into main Dec 19, 2024
4 of 8 checks passed
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.

2 participants