-
Notifications
You must be signed in to change notification settings - Fork 494
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
chore: use modern type annotation syntax #419
base: main
Are you sure you want to change the base?
Conversation
I believe I've tried this out before but it breaks the parsing logic when passing tools as function - could you verify? |
Wouldn't there be a failing test here if that was the case? 😁 |
@akx I would hope so HAHA. I'm pretty sure I had a bunch of tests for parsing logic but maybe it's fine. I'd still want to just run the example for a sanity check. |
7f896f0
to
1fb7b5f
Compare
@ParthSareen Yes, the example still works on various versions of Python. Love that it's nondeterministic though and sometimes the model decides to pass in strings instead of ints, and sometimes it apologizes about a confusing response. This will require #422 to be merged first so CI can be green at all.
|
@akx what model are you using? I find it funny that it does that and also a bit annoying. Working on sampling in the new engine and so also thinking about improving tool calling as a whole through structured generation. Hopefully can have something soon which can improve this experience. I'll check both the PRs out in the AM. Really appreciate these especially since I've been heads down on a few other areas!! |
|
@akx we should be able to rebase and get this in :) Actually - do we need the extra package - could we do without it? We try to keep the package very minimal with low external dependencies |
1fb7b5f
to
61e68f3
Compare
It's marked to be installed only when the environment has an old Python. I can elide it from this library's deps, but then users will need to install it on those versions by hand. 🤷 |
I'm on the fence for this PR - to a degree I think it is good to have the old typing here as it forces us to think about those cases when working on any typing-related feature. I don't want to bring in unnecessary external dependencies unless it's vastly improving the developer experience. I'll think a bit more about it but honestly this PR improves our experience as devs not really anyone else's. |
If you only have old typing here, then you're not thinking about new typing when working on any typing-related features. You can mix and match old and new typing in tests as you like 🤷
Based on https://pypistats.org/packages/ollama, about 6.5% of users would ever see the new dependency. |
That's a fair point. Going to leave this open for now and come back to this. Thanks! |
Using
from __future__ import annotations
makes futuristic annotations usable in any Python version.This adds
eval_type_backport
as a dep for older Pythons so Pydantic can robustly evaluate these annotations.