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

Handle Llama 3.2 chat template #4

Merged
merged 4 commits into from
Oct 3, 2024

Conversation

DePasqualeOrg
Copy link
Collaborator

This is a work in progress, and I'm not sure how to proceed, since I'm not very familiar with Jinja or this package. I think I managed to solve the test for if tools is defined, but now it's failing when it tries to iterate through an undefined tools.

@DePasqualeOrg DePasqualeOrg mentioned this pull request Sep 29, 2024
@johnmai-dev
Copy link
Owner

The chat template of Llama3.2 uses tojson, but Swift Jinja has not been implemented yet. You can refer to the commits of jinja.js for porting.

huggingface/huggingface.js@78e6a5e
huggingface/huggingface.js@717d69d

@johnmai-dev
Copy link
Owner

johnmai-dev commented Sep 30, 2024

Would you be willing to become a Swift Jinja Collaborator? I hope to find like-minded partners to co-build Swift Jinja, as my abilities are limited on my own. If you're willing, I will grant you access to Swift Jinja.

@DePasqualeOrg
Copy link
Collaborator Author

Sure, I'm happy to help out, although Jinja and the way it's used in these chat templates are still quite new to me.

@DePasqualeOrg
Copy link
Collaborator Author

The chat template of Llama3.2 uses tojson, but Swift Jinja has not been implemented yet. You can refer to the commits of jinja.js for porting.

huggingface/huggingface.js@78e6a5e huggingface/huggingface.js@717d69d

I think this line is not being evaluated as false:

{%- if tools is not none %}

Because then it tries to iterate over tools, which is not iterable when it is none.

@johnmai-dev
Copy link
Owner

The chat template of Llama3.2 uses tojson, but Swift Jinja has not been implemented yet. You can refer to the commits of jinja.js for porting.
huggingface/huggingface.js@78e6a5e huggingface/huggingface.js@717d69d

I think this line is not being evaluated as false:

{%- if tools is not none %}

Because then it tries to iterate over tools, which is not iterable when it is none.

Yes, none also needs to be implemented.

You can port the commit.
huggingface/huggingface.js@80cd456

@DePasqualeOrg
Copy link
Collaborator Author

Thank you. Is there a point in time at which this package fully mirrored the JavaScript implementation? That would be helpful to know so that I can see which new additions might need to be added here.

@johnmai-dev
Copy link
Owner

Thank you. Is there a point in time at which this package fully mirrored the JavaScript implementation? That would be helpful to know so that I can see which new additions might need to be added here.

This package started development on May 20 huggingface/swift-transformers#77 (comment) , but at that time it did not fully mirror the JavaScript implementation. It only completed the mainstream LLM chat template Tests/ChatTemplateTests.swift .

It should be at huggingface/huggingface.js@80cd456.

@pcuenca
Copy link
Collaborator

pcuenca commented Sep 30, 2024

Both none and the use of tools were introduced very recently in jinja.js, to support Llama 3.1. I think that implementing them would go a long way towards supporting most of the chat templates currently in use.

@DePasqualeOrg
Copy link
Collaborator Author

These Jinja packages are complex enough that at the moment the best I can do is hand the problem off to Claude 3.5 Sonnet, and that seems to only get part of the way to a solution. So feel free to take it from here if you want to. It would probably take me several days to get up to speed, and I unfortunately don't have enough capacity to do that right now.

@johnmai-dev
Copy link
Owner

It's okay, I can help you solve the problem.

@DePasqualeOrg DePasqualeOrg force-pushed the handle-llama-3.2 branch 3 times, most recently from 0f062f4 to e73afaa Compare October 1, 2024 17:16
@DePasqualeOrg
Copy link
Collaborator Author

DePasqualeOrg commented Oct 1, 2024

Okay, I got this working with Llama 3.2. I haven't implemented the tools part with tojson, but it will work in a normal chat scenario. We still need to add tests like the ones here: huggingface/huggingface.js@80cd456

@DePasqualeOrg DePasqualeOrg marked this pull request as ready for review October 1, 2024 17:19
@DePasqualeOrg
Copy link
Collaborator Author

I added a test for none, and I had to make some changes from the TypeScript version to make it pass. Please check if this is correct.

@DePasqualeOrg
Copy link
Collaborator Author

I checked huggingface/huggingface.js#846 again and fixed a few things that I think now bring this in line with the TypeScript implementation. I think this is now ready for review.

Sources/Parser.swift Outdated Show resolved Hide resolved
@johnmai-dev
Copy link
Owner

Thanks a lot for the PR!

@johnmai-dev johnmai-dev merged commit b435eb6 into johnmai-dev:main Oct 3, 2024
3 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.

Runtime error: Unknown node type: TestExpression
3 participants