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

modify behaviour of the construct method #898

Merged
merged 6 commits into from Oct 23, 2019
Merged

modify behaviour of the construct method #898

merged 6 commits into from Oct 23, 2019

Conversation

@samuelcolvin
Copy link
Owner

samuelcolvin commented Oct 15, 2019

Change Summary

Change construct, fix #897

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • changes/<pull request or issue id>-<github username>.md file added describing change
    (see changes/README.md for details)
@codecov

This comment has been minimized.

Copy link

codecov bot commented Oct 15, 2019

Codecov Report

Merging #898 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff          @@
##           master   #898   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          16     16           
  Lines        2799   2799           
  Branches      543    543           
=====================================
  Hits         2799   2799

Continue to review full report at Codecov.

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

pydantic/main.py Outdated Show resolved Hide resolved
@dmontagu

This comment has been minimized.

Copy link
Collaborator

dmontagu commented Oct 15, 2019

This is very close to what I want. A few points:

  1. I think there might be a minor opportunity for performance improvement by caching the defaults map, but even without that this would obviously be a huge improvement.

  2. It's still not an in-place swap for BaseModel.__init__ due to the lack of a **kwargs argument; that would be really nice 😬

  3. (Related to the previous point:) Because validation does not occur on initialization, I think it is especially important to support a type-checkable API for this function, at least if we wanted to encourage it's use for known-valid inputs. I probably wouldn't feel comfortable using this function in my code without it.

    My mypy plugin can be used to typecheck inputs to Model.__init__, and it would be straightforward to adapt that same approach to any other method where the field values were passed as keyword arguments. But I think it would be hard/impossible to get type-checking to work nicely with a dict as the input argument like this.

    Given my understanding of PyCharm's type checking, I am also confident it would be straightforward to add type-checking support for this to PyCharm (@koxudaxi could confirm).

    If I can't convince you to add a method with essentially the same signature as BaseModel.__init__ that basically just calls the construct function with fields_set=None, that will be okay -- I'll just implement the method in a base class and write a custom extension to the existing (and hopefully eventually merged) mypy plugin. But I think many use cases could benefit from this API, and I would be harder-pressed to recommend the use of construct (as it is now) for this purpose now given how well supported type-checking is when using the BaseModel.__init__ with PyCharm and mypy (soon).

    Tl;dr: I'd rather be slow than unsafe, but maybe this doesn't have to be the tradeoff?

@dmontagu

This comment has been minimized.

Copy link
Collaborator

dmontagu commented Oct 15, 2019

To be more explicit, I think a method like this would be easier to make type-safe (here, I'm using the "old" version of construct):

    @classmethod
    def safe_construct(cls: Type[Model], **data: Any) -> Model:
        # TODO: Cache defaults
        defaults = {name: deepcopy(field.default) for name, field in cls.__fields__.items() if not field.required}
        values = {**defaults, **data}
        fields_set = set(data.keys())
        return cls.construct(values, fields_set)
@samuelcolvin

This comment has been minimized.

Copy link
Owner Author

samuelcolvin commented Oct 16, 2019

@dmontagu I'm fine with that, the only reason I didn't do that was that I thought it avoided the possibility to provide fields_set, this isn't generally necessary, but there might be some situations where you want it.

E.g. you do something like:

model_data = dict(model1)
...
# lots of process, RPC etc.
...
model2 = Model.construct(model_data)

But obviously model_data contains some default fields, so model1.__fields_set__ != model2.__fields_set__.

So I was allowing for

model_data = dict(model1)
model_fields_set = model1.__fields_set__
...
# lots of process, RPC etc.
...
model2 = Model.construct(model_data, model_fields_set)

I know this won't be used much, but it should be possible.

However I think we can have the best of both worlds:

since we have to trust the input data to construct() we know it can't have any keys starting with _, so we can have an extra keyword-only field _fields_set which can never conflict with field keys.

So the signature of construct becomes something like:

def construct(cls: Type[Model], *, _fields_set: Optional['SetStr'] = None, **model_data: Any) -> Model:

Would this work for you?

Regarding caching defaults I don't think it's necessary, but if you prefer we can.

@ashears ashears mentioned this pull request Oct 16, 2019
0 of 4 tasks complete
@dmontagu

This comment has been minimized.

Copy link
Collaborator

dmontagu commented Oct 16, 2019

Starting with the _ is a nice trick; that would work great for me. Let me see how much of a difference caching the defaults makes while profiling using my example above. It’s probably easiest to not cache and probably fast enough.

@ashears ashears mentioned this pull request Oct 17, 2019
4 of 4 tasks complete
@dmontagu

This comment has been minimized.

Copy link
Collaborator

dmontagu commented Oct 17, 2019

I did the profiling -- I get about a 30x speedup with this implementation over the with-validation-approach in my benchmarking script. I get about a 36x speedup if I cache the default values. But not caching removes a nuisance around mutable default values: we'd have to worry about deepcopying the cached values (I wasn't doing that when getting the 36x).

Given the "slow" version still offers a 30x speed improvement, and this method seems somewhat rarely used now, I'm happy with this (more maintainable) version. If this method starts getting heavier use, and if further performance improvements to it become a priority, we could always revisit.

+1 for merging as it is now implemented

@samuelcolvin samuelcolvin marked this pull request as ready for review Oct 17, 2019
@@ -414,14 +414,17 @@ def from_orm(cls: Type['Model'], obj: Any) -> 'Model':
return m

@classmethod
def construct(cls: Type['Model'], values: 'DictAny', fields_set: 'SetStr') -> 'Model':
def construct(cls: Type['Model'], _fields_set: Optional['SetStr'] = None, **values: Any) -> 'Model':

This comment has been minimized.

Copy link
@dmontagu

dmontagu Oct 18, 2019

Collaborator

@samuelcolvin Would it make sense to rename cls to __pydantic_cls__ (similar to in BaseModel.__init__), or at least something that starts with a _ to prevent keyword argument conflicts?

* add example for construct

* edit exporting_models

* typo

* add changes file

* code review changes

* fix bad copy paste
m = cls.__new__(cls)
object.__setattr__(m, '__dict__', values)
object.__setattr__(m, '__fields_set__', fields_set)
object.__setattr__(m, '__dict__', {**defaults, **values})

This comment has been minimized.

Copy link
@dmontagu

dmontagu Oct 21, 2019

Collaborator

We can replace defaults with cls.__defaults__ once #915 is merged; that can be a separate PR though if this is ready first.

@samuelcolvin

This comment has been minimized.

Copy link
Owner Author

samuelcolvin commented Oct 21, 2019

I think this is ready, I'll merge it after #915 and the defaults tweak above.

@samuelcolvin samuelcolvin merged commit 677677e into master Oct 23, 2019
11 checks passed
11 checks passed
Header rules No header rules processed
Details
Pages changed 5 new files uploaded
Details
Redirect rules No redirect rules processed
Details
Mixed content No mixed content detected
Details
codecov/project 100% remains the same compared to 6b5adcc
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
samuelcolvin.pydantic Build #20191023.11 succeeded
Details
samuelcolvin.pydantic (Job Python36) Job Python36 succeeded
Details
samuelcolvin.pydantic (Job Python37) Job Python37 succeeded
Details
@samuelcolvin samuelcolvin deleted the modify-construct branch Oct 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.