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

feat: Computed field serialization for TypedDict #1018

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

michaelhly
Copy link
Contributor

@michaelhly michaelhly commented Oct 15, 2023

Change Summary

Updated computed field serialization to use the serialization function directly (if one exists) instead of always looking up a model attribute.

Related issue number

fix #657

Checklist

  • Unit tests for the changes exist
  • Documentation reflects the changes where applicable
  • Pydantic tests pass with this pydantic-core (except for expected changes)
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Selected Reviewer: @samuelcolvin

@codecov
Copy link

codecov bot commented Oct 15, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.15%. Comparing base (23d1065) to head (a4f0ca1).
Report is 316 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1018      +/-   ##
==========================================
+ Coverage   93.12%   93.15%   +0.03%     
==========================================
  Files         106      106              
  Lines       15952    15994      +42     
  Branches       35       35              
==========================================
+ Hits        14855    14899      +44     
+ Misses       1090     1088       -2     
  Partials        7        7              
Files with missing lines Coverage Δ
src/serializers/computed_fields.rs 97.04% <100.00%> (+0.97%) ⬆️

... and 2 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 23d1065...a4f0ca1. Read the comment docs.

@codspeed-hq
Copy link

codspeed-hq bot commented Oct 15, 2023

CodSpeed Performance Report

Merging #1018 will improve performances by 13.01%

Comparing michaelhly:computed-field-ser-func (a4f0ca1) with main (23d1065)

Summary

⚡ 1 improvements
✅ 139 untouched benchmarks

Benchmarks breakdown

Benchmark main michaelhly:computed-field-ser-func Change
test_generator_rust 35.3 µs 31.2 µs +13.01%

@michaelhly michaelhly changed the title feat: Serialize computed field without a model feat: Serialize computed field for dictionaries types Oct 15, 2023
@michaelhly michaelhly changed the title feat: Serialize computed field for dictionaries types feat: Computed field serialization for TypedDicts Oct 15, 2023
@michaelhly michaelhly changed the title feat: Computed field serialization for TypedDicts feat: Computed field serialization for TypedDict Oct 15, 2023
@michaelhly michaelhly marked this pull request as ready for review October 15, 2023 10:24
@michaelhly
Copy link
Contributor Author

please review

@michaelhly michaelhly force-pushed the computed-field-ser-func branch 3 times, most recently from e7d7a9a to 8525d28 Compare October 15, 2023 11:28
Copy link
Contributor

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks generally like a fine approach, with a couple of small thoughts. I wonder how this looks when applied to a TypedDict? I'm unsure if it's actually valid to have methods on a TypedDict?

src/serializers/computed_fields.rs Outdated Show resolved Hide resolved
tests/serializers/test_model.py Outdated Show resolved Hide resolved
@michaelhly
Copy link
Contributor Author

Looks generally like a fine approach, with a couple of small thoughts. I wonder how this looks when applied to a TypedDict? I'm unsure if it's actually valid to have methods on a TypedDict?

We compute based on the serialization function provided in the schema instead of adding a method on the TypedDict model. Here is the corresponding unit test for the described case:
147e917

@adriangb
Copy link
Member

I'm unsure if it's actually valid to have methods on a TypedDict?

At runtime yes. But type checkers hate it (insert meme reference).

Copy link
Contributor

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 so I think then in principle we can allow these fields on TypedDict and users just have to type: ignore them?

Comment on lines +212 to +214
// Backwards compatiability.
let mut legacy_attr_error: Option<PyErr> = None;
let legacy_result = match ob_type_lookup.get_type(input_value) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's fair to call this "backwards compatibility" when this is still expected to be the main code path for models and dataclasses.

I wonder if there might be a more unified way. For a model or dataclass A with computed field b, the analogous functionality really seems to be A.b.__get__(instance). For a TypedDict it looks like that also works:

>>> class Bar(TypedDict):
...      @property
...      def y(self):
...          return 434
...
>>> Bar.y.__get__({})
434

So maybe what we really want, in all cases, is

let property_value = input_value.get_type().getattr(field.property_name_py.as_ref(py))?.call_method1("__get__", (input_value,))?;

Copy link
Contributor Author

@michaelhly michaelhly Oct 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thinking was that a serialization function should be provided to a computed field, doesn't matter if the input type is a Model, TypedDict, Dataclass, etc.

The default behavior is to compute the computed value from the function provided in the serialization schema and then it gets set in the output_dict:

let value = self
.serializer
.to_python(next_value, next_include, next_exclude, extra)?;
if extra.exclude_none && value.is_none(py) {
return Ok(());
}
let key = match extra.by_alias {
true => self.alias_py.as_ref(py),
false => property_name_py,
};
output_dict.set_item(key, value)?;

This seems more generalizable to all computed fields instead of relying on the computed field defined as an attribute on the input value.

However, I am probably missing some context on how computed fields are used by https://github.com/pydantic/pydantic.

Copy link
Member

@adriangb adriangb Oct 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not following all that closely but my 2c is that ideally we extract the function from the thing in pydantic and not in pydantic-core so that:

  1. We have more flexibility. It's easier to hack things (like rebuild __mro__ based on __orig_bases__ which we do for TypedDict)
  2. It ensures that we do this at schema build time and not runtime

The con of that last one is that in theory someone could want us to use the method on a subclass they pass in as a value, which doesn't apply to TypedDict but also is not what we do for BaseModel and no one has complained 😄

@sydney-runkle
Copy link
Member

@davidhewitt, do you expect us to pick back up with this soon?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Computed fields don't work with TypedDict
5 participants