-
-
Notifications
You must be signed in to change notification settings - Fork 264
fix forward type reference in Pydantic schemas #1171
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
fix forward type reference in Pydantic schemas #1171
Conversation
|
This error happened to me after upgrading to version This led me to the assumption that it must be some change/regression in pydantic itself. I took a look at the pydantic diff: pydantic/pydantic@v2.10.1...v2.10.2, but without having a deeper understanding of the pydantic code base I can't really pinpoint what changes might lead to this issue. Manually adding your changes also fixes the issue, but I guess it should be clarified if there should be a fix in pydantic instead and potentially the pinning of |
|
Can confirm, was also struggling with this. |
|
It sounds likely that pydantic's behavior did change with that patch release, but I'm not sure that that's the whole story. The pydantic upgrade has been present for a couple of months; my fork includes that change, and I've been working on my fork a lot during that time, and testing with specs that do include a reference from But even if it's only pydantic >2.10.1 that has the problem in these cases, I have to assume, based on the docs I linked to, that this was already a known issue in some cases: they specifically tell you that if you have a circular reference and it fails to resolve automatically, the workaround is to call |
|
Hi, Pydantic maintainer here. In 2.10, we did a complete refactor of the forward annotations evaluation logic to be more correct: it supports more edge cases, but also removes support for invalid use cases. This created some churn in libraries having invalid use cases implemented, but fixing it will result in safer code and better consistency. Regarding the openapi-python-client/openapi_python_client/schema/openapi_schema_pydantic/encoding.py Lines 7 to 22 in 861ef56
Here, Before 2.10, this used to somehow work because our namespace management was poorly designed, and symbols that shouldn't resolve (like in the example above, where To fix the issue, you can either:
I'm working on a PR to suggest changes following the second option. @eli-bl, do note that benchling#223 is not a complete fix. You might want to apply the same changes from my PR on the fork. Footnotes |
|
@Viicos, thanks for all of that context, and for submitting the alternate PR! I'll close this one and I'll also update the fix on our fork as per your advice. |
This small change addresses an issue that, unfortunately, I don't have a good test case to demonstrate. I've only been able to reproduce it with one specific API spec file... and only in my fork. Ordinarily I wouldn't submit an upstream change due to something that's only ever happened in my fork. However, in this case:
openapi_schema_pydantic/in this fork.schemas.py, where it's trying to construct aParameterinstance. The error message ("Parameteris not fully defined") indicates that the problem is not the arguments to theParameterinitializer—it's theParameterclass itself, which Pydantic considers to be invalid. The message also indicates that the "not fully defined" part is a forward reference to theHeaderclass.encoding.py) existed in order to avoid an import cycle betweenParameter,Encoding, andHeader. That should be fine, and was working fine till now with exactly the same code in these Pydantic models. However...model_rebuild()on the class that's getting the error, after the previously-undefined class has been defined.Because of all that, my theory is that there is some extremely subtle interaction between my other changes in the fork which, under some very specific conditions related to the particular spec I was testing with, maybe led to modules being imported in a different order... or something like that. This apparently had no effect on anything else, but made a difference in Pydantic's lazy-resolve mechanism. I can't characterize it any better than that, and I don't know how to write a test for it (although this fix does solve my use case). But I do believe this workaround is valid according to the docs and doesn't cause problems under any other circumstances. And I suspect that the circumstances under which it was working were brittle enough that some other completely unrelated change could've eventually caused it to manifest in an equally confusing way, so you could see this as a fix-in-advance for that.
(The problem is not that
header.pyhadn't actually been imported before that line inschemas.pyexecuted. I verified 100% for sure that it had been. It's something more subtle than that.)