| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
I'll fix the test suite first in a separate PR. [UPDATE: done] |
Sorry, something went wrong.
There was a problem hiding this comment.
Test suite for Python 3.13 is currently failing because pydantic-core==2.18.4 can't get installed with PyO3 0.21.2 on Python 3.13. We need PyO3 v0.22.0 or higher as that version supports Python 3.13, but I'm not sure yet why it's not pulling in the latest version - probably some other dependency is imposing an upper bound.
Sorry, something went wrong.
|
Ok, some progress: sqlmodel's pin on typing-extensions was preventing us to use the latest pydantic version compatible with Python 3.13. If we relax that pin, we can get correct Pydantic versions for Python 3.13. The CI is then still failing with a linting error, which I'll look into now. [UPDATE]: these linting errors appear because we're upgrading to Pydantic 2.10.6 (they don't appear when using Pydantic 2.9.2 for instance) |
Sorry, something went wrong.
|
Whoa, this was a lot of work! 😱 If dropping support for Python 3.7 would have made this easier, I would have definitely accepted it, just so you know you don't have to battle it so hard. 😅 If 3.7 is being problematic again in the future, let me know and we can just drop support for it. Maybe we can just do it soon, preemptively, it's already too old. I wanted to include any easy bug fixes in and make a final 3.7 release before dropping support, but if there's no single bug fix release to make, we can just drop it. We should also do it soon for 3.8 as well, we'll get into the same issues soon. 😬 About model_fieldsI'm pretty sure the problem is that Pydantic has it defined as a dict[str, pydantic.fields.FieldInfo] and dict doesn't support subclasses of the parameters defined (the erudite term is "invariant" I think 😅, dict is invariant). I suspect they don't really need it to be a dict, it could be any mapping, so Mapping[str, pydantic.fields.FieldInfo] would have probably worked as well, and Mapping allows subclasses in the value types (erudite term: Mapping is covariant in the second parameter, if I'm not wrong). It's also not common for people to extend that in Pydantic, it's not a fully public object (it's not documented), so I don't judge it and wouldn't necessarily expect them to change it. 😅 Nevertheless, our code should be okay, so we could add a # type: ignore there and it would be fine. Now, what do we get from our definition? Whenever someone tries to iterate on the model fields of a model, they would get autocompletion and inline errors for our custom extra attributes. E.g.: for f in Hero.model_fields.values():
print(f.index)In this case, they would get autocompletion for f.index. ...but, checking that code, I also realize that it is not properly typed, those extra attributes have no types, so they show as existing, but as Any. So, the "advantage" we could get is not even properly/fully implemented yet. I would say we can add a type ignore and later in another PR add the types, just to save the autocompletion there, not sure how useful and used it is, but maybe. What do you think? |
Sorry, something went wrong.
Hehe, gotcha. The most time was spent on actually figuring out WHY uv's dependency resolution tracked back to such an old version of Pydantic. Once I found out that the typing-extensions pin was the culprit, the fix was actually an easy one. But yes - I agree we could drop 3.7 (and maybe even 3.8) to avoid these kind of issues in the future.
Yep - exactly - that's the issue.
Yep - that's my understanding as well.
I see what you mean with respect to having (potentially future) access to better autocompletion & type hints when we keep the redefinition to the dictionary containing sqlmodel's FieldInfo objects. I assume we would never really expect to add Pydantic's FieldInfo objects into SQLModelMetaclass, right? In that case I agree that we can just add the ignore. I'll push that change. |
Sorry, something went wrong.
|
📝 Docs preview for commit 5b1f617 at: https://1205cfb7.sqlmodel.pages.dev |
Sorry, something went wrong.
Yep, I think so, we expect people to use sqlmodel.Field instead of pydantic.Field, so we would always have our own custom FieldInfo there. So, yep, I think that makes sense. 🤓 |
Sorry, something went wrong.
There was a problem hiding this comment.
Awesome, thank you for all the work put into this! 🚀
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
This ended up needing more changes than I had expected:
But I don't think we really need to redefine it? We'll be able to pass in a sqlmodel.main.FieldInfo object whereever a pydantic.fields.FieldInfo is expected, and the rest of the code base / tests / type checks don't seem to complain when we remove the redefinition (L482).
As discussed below with Tiangolo, this PR now just adds an ignore statement for this.