Tracking for setting attributes #389

dgasmith · 2019-02-10T02:30:28Z

Change Summary

Tracking when default arguments are override and allows a skip_defaults kwarg in the dict method.

A problem that I have not yet solved is that a BaseModel goes through the dict_validator and is then passed back into BaseModel.validate after being cast to a dict, this does not allow the __fields_set__ to come through at the moment and recursive models do not work. Any suggestions here?

make benchmark-pydantic
Before:           pydantic best=29.726μs/iter avg=30.439μs/iter stdev=0.563μs/iter
After:            pydantic best=29.061μs/iter avg=29.695μs/iter stdev=0.473μs/iter

Related issue number

Discussion in #378.

Checklist

Unit tests for the changes exist
Tests pass on CI and coverage remains at 100%
Documentation reflects the changes where applicable
HISTORY.rst has been updated
- if this is the first change since a release, please add a new section
- include the issue number or this pull request number #<number>
- include your github username @<whomever>

samuelcolvin · 2019-02-10T23:57:05Z

Single quotes please everywhere, also you'll need to respect black. Use make format.

dgasmith · 2019-02-11T14:26:32Z

Of course, I was holding off on that area until we had an idea of what to do with the recursive copy operation. Easiest thing would be to write a custom copy validator for BaseModels rather than relying on the standard dict validator. However, that solution isn't as elegant as the current code.

samuelcolvin · 2019-02-11T14:39:17Z

Oh I see. Maybe related to #265?

Do you know how to proceed or do you have a specific question?

dgasmith · 2019-02-11T14:43:31Z

Similar to #265. If you are fine with having a custom validator for BaseModels rather than the dict_validator we could solve both the issue here and #265.

samuelcolvin · 2019-02-11T14:43:51Z

great

dgasmith · 2019-02-11T15:59:44Z

#265 was deeper than I expected. I think that can be solved however, but I will leave it for another PR. This is now ready to review.

codecov · 2019-02-11T16:06:22Z

Codecov Report

Merging #389 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #389   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          14     14           
  Lines        2031   2059   +28     
  Branches      409    419   +10     
=====================================
+ Hits         2031   2059   +28

samuelcolvin

Are you sure about the benchmark numbers? They sound to good to be true.

samuelcolvin · 2019-02-12T10:27:35Z

pydantic/main.py


    def __init__(self, **data: Any) -> None:
        if TYPE_CHECKING:  # pragma: no cover
            self.__values__: Dict[str, Any] = {}
-        self.__setstate__(self._process_values(data))
+            self.__fields_set__: Set[str] = set()
+        fields_set = data.pop("__fields_set__", set(data.keys()))


Strict rule in pydantic that you can;t pass anything other than values to the class like this.

Users should be able to create a model with MyModel(**whatever) without having to think about what's in whatever.

Remove this and either call __setstate__ directly, or set model.__fields_set__ = thing directly.

pydantic/main.py

samuelcolvin · 2019-02-12T10:29:02Z

pydantic/main.py

@@ -324,7 +340,8 @@ def construct(cls, **values: Any) -> 'BaseModel':
        Chances are you don't want to use this method directly.
        """
        m = cls.__new__(cls)
-        m.__setstate__(values)
+        fields_set = values.pop("__fields_set__", None)


Single quotes always please unless the string contains single quotes.

But again, can we avoid doing values.pop().

I think I need to add the ability to pass in some sort of state from the calling instance. Can I change construct to take in a fields_set or should I add a new function like construct_with_fields or similar?

construct can take more arguments, just __init__ that should remain pure.

samuelcolvin · 2019-02-12T10:29:57Z

pydantic/main.py

@@ -342,15 +359,13 @@ def copy(
        """
        if include is None and exclude is None and update is None:
            # skip constructing values if no arguments are passed
-            v = self.__values__
+            v = self.__values__.copy()


do we need this copy?

No, not if we remove the explicit add of __field_set__ below.

samuelcolvin · 2019-02-12T10:30:33Z

pydantic/main.py

+        self, include: Set[str] = None, exclude: Optional[Set[str]] = set(), skip_defaults: bool = False
+    ) -> Set[str]:
+        if skip_defaults:
+            keys = self.__fields_set__.copy()


i doubt we need copy() here.

We do as we modify the set in-place later for speed.

samuelcolvin · 2019-02-12T10:30:50Z

pydantic/main.py

+        if skip_defaults:
+            keys = self.__fields_set__.copy()
+        else:
+            keys = set(self.__values__.keys())


I doubt we need set() here.

If we do not convert it to a set (there is no copy for dict keys), we rely on the fact that dict keys are special sets which return LHS references if modified in place. Seems a bit ugly, when we can explicitly copy the data here and move on with things.

ok, that's fine.

samuelcolvin · 2019-02-12T10:31:08Z

pydantic/main.py

+            keys = set(self.__values__.keys())
+
+        if include:
+            keys &= set(include)


I doubt we need set() here.

I was erring on the side of safety, not everyone uses mypy. But happy to remove it.

I think this one really can be removed. If someone passes something weird here they should get an error.

samuelcolvin · 2019-02-12T10:35:07Z

pydantic/main.py

-            yield k, self._get_value(v, by_alias=by_alias)
+            yield k, self._get_value(v, by_alias=by_alias, skip_defaults=skip_defaults)
+
+    def _calculate_keys(


I think we can probably do this in a more efficient way, eg. if include=None, exclude=set() and skip_defaults=False (all the defaults), then we just take all keys.

Can't we take care of that case but just returning None or something here and not checking key containment?

Sure, I can work on optimizing that out.

On this note, the include/exclude defaults are not consistent (copy for example has them both defaulting to None). I can clean this up as well.

HISTORY.rst

Co-Authored-By: dgasmith <dgasmith@icloud.com>

dgasmith · 2019-02-12T15:39:31Z

Timings:

defaultfields  BENCHMARK_REPEATS=20 make benchmark-pydantic
python benchmarks/run.py pydantic-only
                                pydantic time=0.940s, success=48.45%
...
                                pydantic best=0.886s, avg=0.911s, stdev=0.017s

                                pydantic best=7.387μs/iter avg=7.592μs/iter stdev=0.138μs/iter

master  BENCHMARK_REPEATS=20 make benchmark-pydantic
python benchmarks/run.py pydantic-only
                                pydantic time=0.919s, success=48.45%
...
                                pydantic best=0.888s, avg=0.914s, stdev=0.020s

                                pydantic best=7.397μs/iter avg=7.620μs/iter stdev=0.165μs/iter

Not entirely sure why. Could depend heavily on benchmark construction and a bit of extra set manipulation over raw lists.

samuelcolvin · 2019-02-12T15:47:00Z

Tried it myself and I get a ~5% time increase with this PR. Not ideal.

To put this in proportion, this increase is roughly the same as the increase which was initially introduced by the typing PR #373, I spent a couple of hours fighting with it to mostly eliminate that performance deterioration. I'm therefore somewhat reticent about introducing that slowdown here.

Get the PR ready and I'll see what I can do.

dgasmith · 2019-02-12T16:10:21Z

Odd on timings, those were 3.6. For 3.7 I see:

this branch               pydantic best=6.871μs/iter avg=7.165μs/iter stdev=0.169μs/iter
master                    pydantic best=6.841μs/iter avg=7.238μs/iter stdev=0.170μs/iter

Agreed on the keys comment, patched.

samuelcolvin · 2019-02-13T11:53:08Z

For me this is now ready to be merged. Please check my changes and let me know if you disagree with anything or have any questions.

We might need to update history to state that contruct's signature has changed.

dgasmith · 2019-02-13T15:02:11Z

Looks good! I was hesitant to change constructor, but this is much better and is extensible if there are other slots needed in the future. Thanks for working through this and the great feedback.

Add CoreSchemaType Literal

dgasmith added 2 commits February 9, 2019 21:09

Tracking for setting attributes

03f4e62

Fixes accidental leak of fields

3a11bfd

dgasmith added 2 commits February 11, 2019 10:52

Allows defaults fields to be recursively set

2e2b0fa

Docs and history for skip_defaults

c234cf5

Mypy fix on calculate keys

22423d0

samuelcolvin reviewed Feb 12, 2019

View reviewed changes

samuelcolvin and others added 5 commits February 12, 2019 09:23

Update pydantic/main.py

43cee21

Co-Authored-By: dgasmith <dgasmith@icloud.com>

Update pydantic/main.py

11625aa

Co-Authored-By: dgasmith <dgasmith@icloud.com>

Update HISTORY.rst

52cf3f4

Co-Authored-By: dgasmith <dgasmith@icloud.com>

Cleanup pass based off review

328905a

Simplifies constructors based on feedback

f924ef4

dgasmith added 2 commits February 12, 2019 10:52

Makes mypy happy with exlicit KeysView

493dbb6

SetOrKeys and faster key search

ba1489c

dgasmith and others added 5 commits February 12, 2019 11:14

Formats files once more

28a998b

Merge branch 'master' into defaultfields

470656d

add tests for dict, pickle and construct

e0272a9

fixes for dict, pickle and construct

eadc755

correct field_set for extra.ignore

291cd55

Fixes format

f4abe74

samuelcolvin merged commit 96e3e74 into pydantic:master Feb 13, 2019

samuelcolvin mentioned this pull request Feb 19, 2019

submodel validation doesn't check the model is of the correct type #403

Closed

alexdrydew pushed a commit to alexdrydew/pydantic that referenced this pull request Dec 23, 2023

Add CoreSchemaType Literal (pydantic#389)

7dcf502

Add CoreSchemaType Literal

Uh oh!

Tracking for setting attributes #389

Tracking for setting attributes #389

Uh oh!

Conversation

dgasmith commented Feb 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Change Summary

Related issue number

Checklist

Uh oh!

samuelcolvin commented Feb 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dgasmith commented Feb 11, 2019

Uh oh!

samuelcolvin commented Feb 11, 2019

Uh oh!

dgasmith commented Feb 11, 2019

Uh oh!

samuelcolvin commented Feb 11, 2019

Uh oh!

dgasmith commented Feb 11, 2019

Uh oh!

codecov bot commented Feb 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

samuelcolvin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgasmith Feb 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dgasmith commented Feb 12, 2019

Uh oh!

samuelcolvin commented Feb 12, 2019

Uh oh!

dgasmith commented Feb 12, 2019

Uh oh!

dgasmith commented Feb 10, 2019 •

edited

Loading

samuelcolvin commented Feb 10, 2019 •

edited

Loading

codecov bot commented Feb 11, 2019 •

edited

Loading

dgasmith Feb 12, 2019 •

edited

Loading