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

Remove Dump Validation #1304

Merged
merged 12 commits into from
Jul 31, 2019
Merged

Conversation

deckar01
Copy link
Member

Allow dump errors to bubble to the top to avoid building an error dictionary.

Fixes #1132

@deckar01 deckar01 mentioned this pull request Jul 17, 2019
tests/test_serialization.py Outdated Show resolved Hide resolved
Copy link
Member

@sloria sloria left a comment

Choose a reason for hiding this comment

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

I'm good with this so far. I think this is the right approach-- 💯 code simplification.

We'll definitely want to clarify somewhere in the docs (Quickstart, perhaps) that validation only happens on load.

docs/extending.rst Outdated Show resolved Hide resolved
@@ -813,15 +813,13 @@ def make_item(foo, bar):
with pytest.raises(ValidationError) as excinfo:
schema.dump(make_item(6, 1))
errors = excinfo.value.messages
assert "foo" in errors
Copy link
Member

Choose a reason for hiding this comment

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

Since we're not supporting validation-on-dump, we might want to just remove the (pre|post)_dump methods in this test and the assertions associated them, full stop.

@lafrech
Copy link
Member

lafrech commented Jul 18, 2019

+32 −454 😍
That's just great.

@sloria
Copy link
Member

sloria commented Jul 19, 2019

Speeds up tox -e benchmark by 15%.
Update: With deckar01#1 , speedup is ~28%

Update: also, positive results when running https://github.com/voidfiles/python-serialization-benchmark.

2.x-line

Library                  Many Objects (seconds)    One Object (seconds)    Relative
---------------------  ------------------------  ----------------------  ----------
Custom                               0.00995517              0.00542903     1
lima                                 0.0137742               0.00731897     1.37109
Pickle                               0.026233                0.022099       3.14166
serpy                                0.0368881               0.0190721      3.63751
Strainer                             0.0602009               0.030582       5.90105
Colander                             0.232925                0.112409      22.4473
Lollipop                             0.344873                0.172921      33.6575
Avro                                 0.410574                0.220067      40.9928
Marshmallow                          0.425449                0.211551      41.4061
kim                                  0.630801                0.319591      61.7771
Django REST Framework                0.643825                0.498208      74.2342

dev

Library                  Many Objects (seconds)    One Object (seconds)    Relative
---------------------  ------------------------  ----------------------  ----------
Custom                               0.00990605              0.00465417     1
lima                                 0.014091                0.00704193     1.45142
Pickle                               0.0154889               0.016032       2.16486
serpy                                0.0377049               0.018976       3.89286
Strainer                             0.057797                0.0296898      6.00861
Colander                             0.21589                 0.106215      22.1222
Marshmallow                          0.291953                0.148052      30.2196
Lollipop                             0.323693                0.155794      32.9313
Avro                                 0.383874                0.190489      39.4474
kim                                  0.609875                0.309434      63.1384
Django REST Framework                0.630455                0.473165      75.7969

1132-no-dump-validation--update

Library                  Many Objects (seconds)    One Object (seconds)    Relative
---------------------  ------------------------  ----------------------  ----------
Custom                                0.0123043              0.00623584     1
lima                                  0.0163803              0.00794411     1.31199
Pickle                                0.017761               0.0175638      1.90531
serpy                                 0.040544               0.0217121      3.35791
Strainer                              0.0642502              0.030252       5.09717
Colander                              0.236599               0.120314      19.2508
Marshmallow                           0.245161               0.130003      20.2352
Lollipop                              0.360758               0.171723      28.7204
Avro                                  0.418735               0.212147      34.0279
kim                                   0.685443               0.343925      55.521
Django REST Framework                 0.667474               0.513903      63.72

Note: I had to remove toasted-marshmallow from the benchmark because of lyft/toasted-marshmallow#9

@sloria
Copy link
Member

sloria commented Jul 20, 2019

Once deckar01#1 is merged in and this is brought up to date with dev, I think this is good to merge.

We can probably get rid of ErrorStore at this point, but that can happen in a follow-up PR.

@sloria
Copy link
Member

sloria commented Jul 22, 2019

Feel free to squash my commits into yours if you want; I don't care about the git creds =).

@sloria
Copy link
Member

sloria commented Jul 30, 2019

@deckar01 Are you working on this, or should I pick up where you left off?

@deckar01
Copy link
Member Author

I haven't made any changes locally that haven't been pushed. Feel free to pick it up and push on it. I think some tests started failing after merging deckar01#1. Not sure if it was that or the merge conflicts I resolved. There are also new merge conflicts I haven't looked into.

@sloria
Copy link
Member

sloria commented Jul 30, 2019

OK, I can look into the conflicts this week. Mind if I squash and merge after all is said and done?

@sloria sloria changed the title WIP: Remove Dump Validation Remove Dump Validation Jul 31, 2019
@sloria sloria merged commit a3e95ae into marshmallow-code:dev Jul 31, 2019
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.

Remove validation on serialization
3 participants