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

Render chat template tojson filter as unicode #31041

Merged
merged 3 commits into from
May 28, 2024

Conversation

CISC
Copy link
Contributor

@CISC CISC commented May 26, 2024

What does this PR do?

Rant about lack of special capabilties templates...

I wish more models that have extra capabilities like f.ex. function calling would start using the power available in chat templates instead of implementing their own inference mini-stacks (Mistral, Berkeley, et al.; I'm looking at you) that you would have to add as model-specific dependencies to use those features "as intended".

Shout out to Cohere for actually making an effort here, however it's a shame they did not follow it to completion to also include the final tool role to enable the full round-trip with natural language response to the function call (see my Mistral GGUF for a fully functional example of this).

I've been using the jinja2 tojson filter in a couple of chat templates to render function calling instructions, which is working very well, however sometimes this will introduce escaped unicode characters, which is undesirable.

I'm mainly using GGUFs, so have not used tojson with transformers yet, but I imagine this could be a useful feature for many recent models if only they started using chat templates properly (see rant).

This PR simply changes the default parameters of tojson so that the JSON is rendered in unicode.

Examples of (GGUF) models using tojson:

Submitted similar PR to abetlen/llama-cpp-python#1486

Who can review?

@ArthurZucker

@CISC
Copy link
Contributor Author

CISC commented May 26, 2024

The failing tests seem to be an unrelated issue in main branch, and not this PR.

Copy link
Member

@Rocketknight1 Rocketknight1 left a comment

Choose a reason for hiding this comment

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

LGTM!

@Rocketknight1
Copy link
Member

Also @CISC and @junrae6454, since you're both interested in tool use with chat templates, we're actually planning an overhaul of that, and several of the model templates as well. Please see PR #30621, and let me know if you have any feedback!

@CISC
Copy link
Contributor Author

CISC commented May 28, 2024

@Rocketknight1 Oh, hey, that might impact some of my ongoing work in llama-cpp-python, thanks for the heads up!

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@Rocketknight1
Copy link
Member

Merging without core maintainer review because it's a one-line fix that only affects the tojson filter in Jinja, so it shouldn't have any other side-effects on the library. cc @amyeroberts and @ArthurZucker for visibility anyway!

@Rocketknight1 Rocketknight1 merged commit 22dab24 into huggingface:main May 28, 2024
21 checks passed
Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

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

Thanks and LGTM

@CISC CISC deleted the tojson-unicode branch May 28, 2024 14:32
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.

4 participants