-
Notifications
You must be signed in to change notification settings - Fork 278
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
Jinja - add missing features for DeepSeek R1 #1142
Conversation
I'm not sure why the CI tests don't run on my branch, but they pass when I run them on my machine |
Hey! I'm in the process of reviewing the PR and I'll trigger the CI shortly! I found a few minor issues with the current implementation, so I'm just fixing a few things. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! I cleaned up & fixed a few issues with the implementation of split, but other than that, it looks good!
If you're interested, here were the failing strings, which I've now added as unit tests
SPLIT_4: `|{{ " 1 2 3 ".split() | tojson }}|{{ "babbaccabbb".split("b") | tojson }}|{{ "babbaccabbb".split("b", 2) | tojson }}|`,
SPLIT_5: `|{{ " 1 2 3 4 5 ".split(none, 0) | join(",") }}|{{ " 1 2 3 4 5 ".split(none, 3) | join(",") }}|{{ " 1 2 3 4 5 ".split(" ", 0) | join(",") }}|{{ " 1 2 3 4 5 ".split(" ", 3) | join(",") }}|{{ " 1 2 3 4 5 ".split(" ", 10) | join(",") }}|`,
I'll merge after a final check by @giladgd :) |
@xenova LGTM |
I just updated the e2e unit test to include text so that the split branch is taken. Ready to merge now! Thanks again! |
This PR adds support for the following Jinja features:
.split
on strings - the implementation matches Python'sstr.split(sep=None, maxsplit=-1)
function behaviorDeepSeek R1's chat template used to fail on this expression, which now works in this PR:
Fixes #1141