Skip to content
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 incorrect 'smart' union discrimination #1844

Merged
merged 2 commits into from Nov 5, 2023

Conversation

ml-evs
Copy link
Member

@ml-evs ml-evs commented Oct 30, 2023

Closes #1843.

pydantic v2 made "smart" union discrimination by default, which seems to favour dicts over models.

For now I've made it so that any Union[Model, dict[str, Any]] fields use the newly-added workaround in pydantic v2.3, union_mode = 'left_to_right'. Unfortunately we can't set this globally at the level of StrictField as it crashes out in schema generation if the schema type is not 'union'. I might make an issue about this on pydantic.

@CasperWA you may have thoughts -- will probably merge but feel free to do a post-review!

Copy link

codecov bot commented Nov 1, 2023

Codecov Report

Merging #1844 (e5f4e3a) into master (55e2dd0) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1844   +/-   ##
=======================================
  Coverage   91.20%   91.20%           
=======================================
  Files          75       75           
  Lines        4629     4629           
=======================================
  Hits         4222     4222           
  Misses        407      407           
Flag Coverage Δ
project 91.20% <100.00%> (ø)
validator 90.66% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
optimade/models/responses.py 97.61% <100.00%> (ø)

@ml-evs ml-evs force-pushed the ml-evs/test_enum_deserilization branch 3 times, most recently from e2e587a to bd30d17 Compare November 5, 2023 14:11
@ml-evs ml-evs changed the title Add tests that enums are properly deserialzed for links and links Fix incorrect 'smart' union discrimatinion Nov 5, 2023
@ml-evs ml-evs force-pushed the ml-evs/test_enum_deserilization branch 2 times, most recently from 352f6d2 to 4105648 Compare November 5, 2023 15:42
@ml-evs ml-evs marked this pull request as ready for review November 5, 2023 15:46
@ml-evs ml-evs changed the title Fix incorrect 'smart' union discrimatinion Fix incorrect 'smart' union discrimination Nov 5, 2023
- Add tests that enums are properly deserialzed for links and links
@ml-evs ml-evs force-pushed the ml-evs/test_enum_deserilization branch from 980c1ff to e5f4e3a Compare November 5, 2023 18:38
@ml-evs ml-evs merged commit 0a34a49 into master Nov 5, 2023
11 checks passed
@ml-evs ml-evs deleted the ml-evs/test_enum_deserilization branch November 5, 2023 21:25
@CasperWA
Copy link
Member

CasperWA commented Nov 6, 2023

(...)
For now I've made it so that any Union[Model, dict[str, Any]] fields use the newly-added workaround in pydantic v2.3, union_mode = 'left_to_right'. Unfortunately we can't set this globally at the level of StrictField as it crashes out in schema generation if the schema type is not 'union'. I might make an issue about this on pydantic.

@CasperWA you may have thoughts -- will probably merge but feel free to do a post-review!

My thoughts 😉: Using union_mode='left_to_right' is the exact right thing to do, and should've been done from the beginning.

I think it can be implemented in StrictField. I can try to make a PR for that? But expect me to also come back with a "you can now say 'I told you so'" .... 😅

@CasperWA
Copy link
Member

CasperWA commented Nov 6, 2023

Welp - you can say "I told you so".

To be more explicit, it seems to me the reason for not being able to do this is that we cannot know if the type annotation is a Union when resolving StrictField. And if union_mode is set for a non-Union type field, then an error is raised. At least I think that's what's happening.

So yeah. Either it has to be set explicitly in every place where we want it used - or we can create another wrapper *Field to add the field where we want it. I'd personally prefer the former if it's not in too many places, and it seems indeed not to be in too many places :)

However, it's worth investigating whether this needs to be propagated down to entry-specific models as well to ensure Attributes models are instantiated properly and not as Python dicts.

@ml-evs
Copy link
Member Author

ml-evs commented Nov 6, 2023

To be more explicit, it seems to me the reason for not being able to do this is that we cannot know if the type annotation is a Union when resolving StrictField. And if union_mode is set for a non-Union type field, then an error is raised. At least I think that's what's happening.

I lost an hour to trying to hack this too, probably best to avoid digging in pydantic internals for a while...

So yeah. Either it has to be set explicitly in every place where we want it used - or we can create another wrapper *Field to add the field where we want it. I'd personally prefer the former if it's not in too many places, and it seems indeed not to be in too many places :)

However, it's worth investigating whether this needs to be propagated down to entry-specific models as well to ensure Attributes models are instantiated properly and not as Python dicts.

Yeah, I did a search for anywhere we're doing a union with a dict[str, Any] and it was all confined to the response classes. I think ultimately this is triggering a bug with the new smart unions and enums, hence my raised issue over at pydantic (linked above). Will see what they say.

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.

Deserialization regressions: cannot resolve child databases in client
2 participants