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

Better deserialization handling for QTKEnum #30

Merged
merged 1 commit into from
Nov 10, 2023

Conversation

gpetretto
Copy link
Contributor

QTKEnum had an issue if deserialized inside a pydantic BaseModel. Normally pydantic converts the value to the Enum used to define the field. However, since MSONable have a custom way of dealing with pydantic deserialization, an MSONable Enum was not converted back to an Enum automatically. This should fix it for QTKEnum.

Copy link

codecov bot commented Nov 10, 2023

Codecov Report

Merging #30 (227570e) into develop (c15463a) will decrease coverage by 0.17%.
The diff coverage is 30.00%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop      #30      +/-   ##
===========================================
- Coverage    43.52%   43.35%   -0.17%     
===========================================
  Files           15       15              
  Lines         1119     1128       +9     
  Branches       221      222       +1     
===========================================
+ Hits           487      489       +2     
- Misses         604      611       +7     
  Partials        28       28              
Files Coverage Δ
src/qtoolkit/__init__.py 100.00% <100.00%> (ø)
src/qtoolkit/core/base.py 65.00% <22.22%> (-35.00%) ⬇️

@ml-evs
Copy link
Member

ml-evs commented Nov 10, 2023

QTKEnum had an issue if deserialized inside a pydantic BaseModel. Normally pydantic converts the value to the Enum used to define the field. However, since MSONable have a custom way of dealing with pydantic deserialization, an MSONable Enum was not converted back to an Enum automatically. This should fix it for QTKEnum.

This might also be related to something I ran into recently with pydantic v2, see Materials-Consortia/optimade-python-tools#1844 and related issues. Basically if the model containing the Enum involves a union, and the value is passed as a string, by default in pydantic v2, the deserializer will prefer to use the model without the enum.

@gpetretto
Copy link
Contributor Author

If I understand correctly your point, the case you are mentioning is more related to how pydantic deals with the deserialization if you have multiple types associated to a value and their priority. For example, I have seen that if you have a yaml config file and an attribute is defined as x: Union[str, bool], if in the yaml file you set it to x: false pydantic will set it as the false string. If you define it as x: Union[bool, str], it will convert to a bool if possible.
This is a different case, since MSONable implements __get_pydantic_core_schema__:
https://github.com/materialsvirtuallab/monty/blob/5d37679e1f0cd58a8d165d76f3a095cb71266831/monty/json.py#L291C9-L291C37
So the default behaviour with Enum is entirely lost and, even if an element is defined as a QTKEnum, if it is not serialized as the monty as_dict, it will not be deserialized. Even if there is no Union on types. Here it first tries the standard monty deserialization, if it fails it will try to simply convert to the Enum before failing entirely.

Honestly I was even thinking if it would be better getting rid of QTKEnum entirely.

@gpetretto gpetretto merged commit 6184be9 into Matgenix:develop Nov 10, 2023
5 of 6 checks passed
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