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

Allow using pydantic for from_dict, schema for FastList #479

Merged
merged 4 commits into from
Mar 9, 2025

Conversation

NeejWeej
Copy link
Collaborator

@NeejWeej NeejWeej commented Mar 7, 2025

Allow utilizing pydantic validaiton in from_dict via a new boolean flag. Add a core schema for the FastList to work with pydantic.

Running some simple benchmarks, it seems that the default to_dict and to_json for csp Structs are significantly faster than going the pydantic route. While pydantic does allow for custom serialization and deserialization, the callbacks in to_dict and to_json allow that flexibility as well. Thus, for most use-cases, I believe those are probably preferred.

However, going through pydantic for from_dict is faster and more correct than the current csp implementation (more correct as in respects the type annotations more strictly). Thus, it should be preferred. However, this might cause breaking changes for users, so I propose we introduce it as an optional flag in the from_dict function, and then later down the road switch the default to be True to use pydantic validation for constructing csp Structs from dicts. Here are some plots noting the benchmark results

image

image

image

image

image

image

image

image

Signed-off-by: Nijat Khanbabayev <nijat.khanbabayev@cubistsystematic.com>
@timkpaine timkpaine added the type: enhancement Issues and PRs related to improvements to existing features label Mar 7, 2025
Signed-off-by: Nijat Khanbabayev <nijat.khanbabayev@cubistsystematic.com>
@NeejWeej
Copy link
Collaborator Author

NeejWeej commented Mar 7, 2025

Adding a from_json that goes the pydantic route would also be pretty trivial, though maybe we want to put that in a separate PR

robambalu
robambalu previously approved these changes Mar 7, 2025
Signed-off-by: Nijat Khanbabayev <nijat.khanbabayev@cubistsystematic.com>
robambalu
robambalu previously approved these changes Mar 7, 2025
@timkpaine
Copy link
Member

would be good to include in #332

@NeejWeej NeejWeej merged commit 56e7d32 into main Mar 9, 2025
27 checks passed
@NeejWeej NeejWeej deleted the nk/from_dict_pydantic branch March 9, 2025 03:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement Issues and PRs related to improvements to existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants