Skip to content

Conversation

@DePasqualeOrg
Copy link
Collaborator

Sorry, I should have done some more testing. I was wondering why the error message wasn't bubbling up even after adding LocalizedError conformance. In fact, it looks like the error message will be propagated if we remove Error and Sendable conformance, which JinjaError previously had but are not necessary after adding LocalizedError. After merging this, huggingface/swift-transformers#292 can be reverted.

@mattt
Copy link
Collaborator

mattt commented Nov 25, 2025

Sorry, I'm not sure I follow. LocalizedError itself conforms to Swift.Error, so the only change is removing Sendable conformance. How does this affect error messages bubbling up?

@DePasqualeOrg
Copy link
Collaborator Author

I find it confusing too. When I tested this in its current form (with the two unnecessary protocols), the error message didn't bubble up, and it was necessary to check for JinjaError in swift-transformers, which is not the expected behavior with LocalizedError. However, after further testing, it appears that removing the two unnecessary protocols makes this check unnecessary, and the error behaves as expected. To me it looks like there's some strange behavior with these error protocols when used between packages, but it's also possible that I made a mistake in my testing, which required swapping in modified versions of swift-jinja and swift-transformers.

@mattt
Copy link
Collaborator

mattt commented Nov 25, 2025

Yeah, I'm inclined to believe that we're being bamboozled by weird caching behavior in Xcode. The logic of (error as? LocalizedError)?.errorDescription ?? "\(error)" looks sound to me, and I can't imagine how removing Sendable conformance would affect that.

Let's give ourselves to time to sit with this, so we don't end up thrashing. I'm happy to work with you to unblock you on anything you need to get fixed — just let me know.

@mattt mattt marked this pull request as draft November 25, 2025 14:25
@DePasqualeOrg
Copy link
Collaborator Author

DePasqualeOrg commented Nov 25, 2025

(error as? LocalizedError) actually shows a warning, since JinjaError is always a LocalizedError. I believe the correct solution is the changes I submitted in this PR: remove the unnecessary protocols, and then we don't need to check for JinjaError, since it's a LocalizedError. This is the expected behavior.

I've tested this again with this branch of swift-transformers and verified that this works as expected: https://github.com/DePasqualeOrg/swift-transformers/tree/revert-jinja-error-handling

@DePasqualeOrg DePasqualeOrg marked this pull request as ready for review November 25, 2025 14:57
@DePasqualeOrg DePasqualeOrg merged commit fb5bc2f into huggingface:main Nov 25, 2025
3 checks passed
@DePasqualeOrg
Copy link
Collaborator Author

I've merged this since it's clearly correct, and it doesn't make sense to get hung up on it: Error and Sendable are not needed.

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.

2 participants