Fix pydantic validate and pickle for tagged union #207
Conversation
denghz
changed the title
|
@dbrattli I wanted to check in this PR. Could you let me know when it can be merged? If there are any concerns or further testing needed, please let me know. |
expression/core/tagged_union.py
Outdated
| @@ -38,13 +38,23 @@ def tagged_union( | |||
| items will be compared as the tuple (index, value) | |||
| eq: If True, the __eq__ method will be generated. | |||
| """ | |||
| # Performance can be improved if it is a issue in another day | |||
There was a problem hiding this comment.
What do you mean by this comment? Assigning two methods are not a performance issue. Is it the all of the transform function you are talking about? Please explain.
There was a problem hiding this comment.
I mean the __getstate__ and __setstate__ function. Sorry for putting it at wrong place.
These could be slightly more performant if we generated the code instead of iterating over fields dynamically, which is the approach I am using in my other projects
| # Performance can be improved if it is a issue in another day |
There was a problem hiding this comment.
But it is actually not really a performance issue here and the comment can be removed.
There was a problem hiding this comment.
@denghz Can we remove this line from the PR? I'm unsure why you added it?
|
@denghz Sorry for the delay. I've been busy lately. Let's work together to get this merged. |
|
FYI: I'm ready to merge this when conflicts are resolved, and we have removed that new comment in |
|
Thanks I will do it today. Also I am fixing another bug about model_dump_json and will try to finish it this weekend. |
expression/core/tagged_union.py
Outdated
| @@ -38,13 +38,23 @@ def tagged_union( | |||
| items will be compared as the tuple (index, value) | |||
| eq: If True, the __eq__ method will be generated. | |||
| """ | |||
| # Performance can be improved if it is a issue in another day | |||
There was a problem hiding this comment.
I mean the __getstate__ and __setstate__ function. Sorry for putting it at wrong place.
These could be slightly more performant if we generated the code instead of iterating over fields dynamically, which is the approach I am using in my other projects
| # Performance can be improved if it is a issue in another day |
expression/core/tagged_union.py
Outdated
| @@ -38,13 +38,23 @@ def tagged_union( | |||
| items will be compared as the tuple (index, value) | |||
| eq: If True, the __eq__ method will be generated. | |||
| """ | |||
| # Performance can be improved if it is a issue in another day | |||
There was a problem hiding this comment.
But it is actually not really a performance issue here and the comment can be removed.
expression/core/tagged_union.py
Outdated
| @@ -38,13 +38,23 @@ def tagged_union( | |||
| items will be compared as the tuple (index, value) | |||
| eq: If True, the __eq__ method will be generated. | |||
| """ | |||
| # Performance can be improved if it is a issue in another day | |||
I have removed the comment |
#206 pydantic validate issue
#208 pickle issue