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

✨ Use bulk_write for save operations, including referenced documents #271

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

tiangolo
Copy link
Collaborator

@tiangolo tiangolo commented Sep 28, 2022

✨ Use bulk_write for save operations, including referenced documents

@tiangolo tiangolo changed the title ✨ Use bulk_write for save operations, including referenced documents ✨ Use bulk_write for save operations, including referenced documents Sep 28, 2022
@codecov
Copy link

codecov bot commented Sep 28, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a99a258) 99.38% compared to head (867bfaf) 99.39%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #271   +/-   ##
=======================================
  Coverage   99.38%   99.39%           
=======================================
  Files          50       50           
  Lines        5081     5127   +46     
  Branches      485      491    +6     
=======================================
+ Hits         5050     5096   +46     
  Misses         31       31           
Flag Coverage Δ
tests-3.10-4.4-standalone 93.91% <96.33%> (+0.01%) ⬆️
tests-3.10-5-standalone 93.91% <96.33%> (+0.01%) ⬆️
tests-3.10-6-standalone 93.91% <96.33%> (+0.01%) ⬆️
tests-3.7-4.4-standalone 92.31% <96.29%> (+0.02%) ⬆️
tests-3.7-5-standalone 92.31% <96.29%> (+0.02%) ⬆️
tests-3.7-6-standalone 92.31% <96.29%> (+0.02%) ⬆️
tests-3.8-4-replicaSet 98.38% <100.00%> (+0.01%) ⬆️
tests-3.8-4.2-sharded 93.03% <96.33%> (+0.02%) ⬆️
tests-3.8-4.4-standalone 93.83% <96.33%> (+0.01%) ⬆️
tests-3.8-5-standalone 93.83% <96.33%> (+0.01%) ⬆️
tests-3.8-6-standalone 93.83% <96.33%> (+0.01%) ⬆️
tests-3.9-4.4-standalone 93.83% <96.33%> (+0.01%) ⬆️
tests-3.9-5-standalone 93.83% <96.33%> (+0.01%) ⬆️
tests-3.9-6-standalone 93.83% <96.33%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tiangolo tiangolo marked this pull request as ready for review September 28, 2022 16:27
@art049 art049 self-requested a review September 29, 2022 07:19
Copy link
Owner

@art049 art049 left a comment

Choose a reason for hiding this comment

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

Just a few details to fix but awesome work 🔥.
Also, really nice _get_collection_from_name refactor 😉

odmantic/engine.py Outdated Show resolved Hide resolved
odmantic/engine.py Outdated Show resolved Hide resolved
odmantic/engine.py Show resolved Hide resolved
odmantic/engine.py Show resolved Hide resolved
odmantic/exceptions.py Outdated Show resolved Hide resolved
@tiangolo
Copy link
Collaborator Author

Thanks for the review! I added the changes. 🤓

There's only one that I don't really know how to do, I added a comment about it.

I also included/merged #308 in this PR to be able to run CI and check if I'm breaking anything else.

@art049 art049 force-pushed the master branch 2 times, most recently from 5bd1656 to a99a258 Compare November 3, 2023 20:39
Copy link

codspeed-hq bot commented Nov 14, 2023

CodSpeed Performance Report

Merging #271 will improve performances by ×3.5

Comparing tiangolo:use-bulk-write (867bfaf) with master (a99a258)

Summary

⚡ 3 improvements
✅ 12 untouched benchmarks

Benchmarks breakdown

Benchmark master tiangolo:use-bulk-write Change
test_write_small_bulk[100] 198.4 ms 57.2 ms ×3.5
test_write_small_bulk[50] 99.4 ms 29.6 ms ×3.4
test_write_small_bulk[10] 20.1 ms 7.4 ms ×2.7

@tiangolo
Copy link
Collaborator Author

I'm struggling to debug the current errors, I had never used tox outside this project. I managed to install and setup all the dependencies needed by tox (a python installation for each version, with the exact name, etc). And I was able to replicate the error running a single tox environment with:

$ tox -e python3.9-motor24-pydantic18

And it consistently errors out the same way as in CI.

Then I'm trying to replicate it outside of tox to be able to investigate and debug it, I installed a local python3.9, motor 2.4, pydantic 1.8, and ran the same test, and it always passed.

I'm not sure what else to try as it seems like something particular to the tox-specific environment that I can't access and debug. Or maybe I'm doing something wrong, but I ran out of ideas of what else to try to check it. 🤔 ...also to see if it's an actual new error or some quirk of the setup I have.

@tiangolo tiangolo mentioned this pull request Nov 15, 2023
@tiangolo
Copy link
Collaborator Author

I made another PR that doesn't do anything, just to trigger CI, and it seems it's triggering the same errors: #373

So I suspect the current errors are not something I changed in this PR but something that is currently broken in CI, maybe a tox change or something?

Thinking of all that, would you consider GitHub Actions' integrated matrices instead of tox? https://docs.github.com/en/actions/using-jobs/using-a-matrix-for-your-jobs

@art049
Copy link
Owner

art049 commented Dec 12, 2023

I think we can probably merge after the v1 release :)
Btw with the bench, the speedup is amazing; this is super exciting, thank you. 👏

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.

2 participants