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

Add JSON Benchmark #1344

Merged
merged 4 commits into from
Apr 15, 2020
Merged

Conversation

StephenBrown2
Copy link
Contributor

Change Summary

Pulling out the JSON Benchmarks from #1291 (comment) to be merged first and offer easier performance comparison.

Related issue number

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
Copy link

codecov bot commented Mar 26, 2020

Codecov Report

Merging #1344 into master will not change coverage by %.
The diff coverage is n/a.

@@            Coverage Diff            @@
##            master     #1344   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           21        21           
  Lines         3723      3723           
  Branches       735       735           
=========================================
  Hits          3723      3723           

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 aaec3c9...104fe25. Read the comment docs.

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

otherwise LGTM.

benchmarks/run.py Outdated Show resolved Hide resolved
benchmarks/run.py Outdated Show resolved Hide resolved
@@ -182,7 +169,10 @@ def main():
test = test_class(True)
for j in range(3):
for case in cases:
passed, result = test.validate(case)
if json:
Copy link
Member

Choose a reason for hiding this comment

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

i think we should try and omit the parsing/validation time from the json test.

Can we do something like

if json:
    models = [m for passed, m in (test.validate(c) for c in cases) if passed]
    start = datetime.now()
    ...
        ...
        for m in models:
            test.to_json(m)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Certainly, I did want to exclude the parsing time as well. I wasn't sure how deep we'd want to get with that bit.

@StephenBrown2
Copy link
Contributor Author

FastAPI seems to be failing its db testing test:

=================================== FAILURES ===================================
_______________________________ test_testing_dbs _______________________________

    def test_testing_dbs():
        # Import while creating the client to create the DB after starting the test session
        from sql_databases.sql_app.tests.test_sql_app import test_create_user
    
        test_db = Path("./test.db")
        app_db = Path("./sql_app.db")
>       test_create_user()

tests/test_tutorial/test_sql_databases/test_testing_databases.py:10: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

    def test_create_user():
        response = client.post(
            "/users/",
            json={"email": "deadpool@example.com", "password": "chimichangas4life"},
        )
>       assert response.status_code == 200
E       assert 400 == 200
E        +  where 400 = <Response [400]>.status_code

docs_src/sql_databases/sql_app/tests/test_sql_app.py:37: AssertionError
======================== 1 failed, 796 passed in 9.12s =========================
Makefile:129: recipe for target 'test-fastapi' failed
make: *** [test-fastapi] Error 1
##[error]Process completed with exit code 2.

Pretty sure that's unrelated?

@samuelcolvin
Copy link
Member

looks pretty good, I'm making some tweaks but partially to work out about the fastAPI failures.

@tiangolo
Copy link
Member

tiangolo commented Apr 9, 2020

Yep. FastAPI tests were failing, but they are fixed now. I also triggered the tests on this PR again after that and they are now passing again 🎉 (that was actually yesterday).

@StephenBrown2
Copy link
Contributor Author

Just a heads up, I should have some time tonight and Friday to work on finishing this up; I think I'd like to add some custom subclasses with a Config.json_encoders setting to handle them and compare before #1291 gets applied.

@samuelcolvin samuelcolvin merged commit 0ba7710 into pydantic:master Apr 15, 2020
@StephenBrown2 StephenBrown2 deleted the json_benchmarks branch May 1, 2020 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants