search

guidance-ai / guidance

A guidance language for controlling large language models.
MIT License
18.8k stars 1.04k forks source link

[Bug] Update Mistral chat template #918

Closed riedgar-ms closed 3 months ago

riedgar-ms commented 3 months ago

It appears that the Mistral chat template has had an update, so we need to match this

codecov-commenter commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Please upload report for BASE (main@10fccd3). Learn more about missing BASE report.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #918 +/- ## ======================================= Coverage ? 51.43% ======================================= Files ? 64 Lines ? 4705 Branches ? 0 ======================================= Hits ? 2420 Misses ? 2285 Partials ? 0 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Harsha-Nori commented 3 months ago

I'd also like to think more about if we should force a direct update to the template cache like this, or if we should e.g. have a separate version for the new mistral template update. I'm not sure how frequently model hosts/deployed models will pull new weights/config files down, so this would break them upon a guidance update, while keeping an extra template cache doesn't really cost us anything

riedgar-ms commented 3 months ago

I'd also like to think more about if we should force a direct update to the template cache like this, or if we should e.g. have a separate version for the new mistral template update. I'm not sure how frequently model hosts/deployed models will pull new weights/config files down, so this would break them upon a guidance update, while keeping an extra template cache doesn't really cost us anything

I'm not sure how that would work, although learning the details of these chat templates is another fixture on my ToDo list. One immediate question: what if the templates only differed in whitespace (e.g. for this particular one, suppose another update added a second space between the quotes). Since we strip out whitespace when matching keys, we wouldn't be able to detect that.

Harsha-Nori commented 3 months ago

good q -- I'm really not sure that whitespace stripping the way I did it for cache matching was actually the best decision. jinja ignores some whitespaces but not others, so I figured we'd try to go for maximal coverage at the beginning until figuring out something more clever

Harsha-Nori commented 3 months ago

(because the alternative was for the "space misaligned" template to default to the chatml format which would be way worse than just matching to something reasonably close...)