Skip to content

Conversation

@DePasqualeOrg
Copy link
Contributor

I'm trying to surface error messages from Jinja so that consuming apps can show them in the UI. Currently they're thrown as a JinjaError, which means consuming apps would have to check for that error type in order to show the error message. Here I'm checking for JinjaError in applyChatTemplate and wrapping the error message in TokenizerError.chatTemplate. I'm not sure if this is the best solution, but it does expose the error message from Jinja in my app.

See huggingface/swift-jinja#39

@DePasqualeOrg DePasqualeOrg force-pushed the handle-jinja-error-messages branch from 3b0fb2f to ca0e333 Compare November 24, 2025 09:41
@mattt mattt requested a review from pcuenca November 24, 2025 09:48
@mattt
Copy link
Collaborator

mattt commented Nov 24, 2025

Thank you for your contributions to this package and swift-jinja. I agree with your changes here.

@pcuenca Does this look good to you? Or do you have any suggestions for how to surface these errors differently?

@pcuenca
Copy link
Member

pcuenca commented Nov 24, 2025

We could potentially use a different error, but this works for me if it works for @DePasqualeOrg!

@pcuenca pcuenca merged commit 6fdfa9e into huggingface:main Nov 24, 2025
2 checks passed
DePasqualeOrg added a commit to DePasqualeOrg/swift-transformers that referenced this pull request Nov 24, 2025
@DePasqualeOrg
Copy link
Contributor Author

@pcuenca, @mattt, this can be reverted, since I verified that the error is handled correctly here: https://github.com/DePasqualeOrg/swift-transformers/tree/revert-jinja-error-handling

I think that while I was testing the error propagation with changes in swift-transformers and swift-jinja, something went wrong and the error was not handled as expected, which led me to conclude that we need to handle JinjaError explicitly. In fact, we don't, since it's now a LocalizedError.

pcuenca pushed a commit that referenced this pull request Nov 26, 2025
@pcuenca
Copy link
Member

pcuenca commented Nov 26, 2025

@DePasqualeOrg not a fan of touching history unless necessary, but will merge #293 as you prefer it, (and the code is a bit easier). I cherry-picked the commit from your repo to correctly attribute the fix.

I'll also create a release with the latest commits.

pcuenca added a commit that referenced this pull request Nov 26, 2025
This reverts commit 6fdfa9e.

Co-authored-by: Anthony DePasquale <anthony@depasquale.org>
@DePasqualeOrg
Copy link
Contributor Author

Thank you. It's not a matter of preference, but of correctness. The check is not necessary, so we should remove it.

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.

3 participants