✨ Add new SyncEngine, support async and sync code #225
Conversation
Codecov Report
@@ Coverage Diff @@
## master #225 +/- ##
===========================================
- Coverage 100.00% 99.94% -0.06%
===========================================
Files 38 38
Lines 2725 3374 +649
Branches 413 477 +64
===========================================
+ Hits 2725 3372 +647
- Misses 0 1 +1
- Partials 0 1 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
|
(also added as an edit in the PR description) 👇 Tweaking tox I realized that In that case, it could make sense to make
Note: check the comment below, I think it makes more sense to just make PyMongo required as it's always gonna be required either way. But let me know if it doesn't make sense to you! |
|
I realized that Motor deeply depends on PyMongo, for example, it requires PyMongo objects for bulk_write(). So I think it doesn't really make much sense to consider PyMongo an "optional" dependency, and it would just add maintenance burden. I went ahead and removed the "optional" PyMongo parts and made them required. As a side note, I'm working on adding support for bulk writes, that's how I came to this realization. |
I agree, having pymongo required makes complete sense! Closing this one in favor of #231 |
✨ Add new SyncEngine, support async and sync code
Even the same models can be shared, a model can be retrieved by one engine and saved by the other, etc.
This allows non-async code to use ODMantic, and more importantly, to gradually migrate an existing and sync codebase to ODMantic, migrating to async only/first for the parts that need optimization, but being able to keep the rest of the code.
It would also be, as far as I know, the first sync ODM for Mongo based on Pydantic and type annotations.
This is all backwards compatible.
Note 🚨
It's probably better to merge #227 first, as that fixes a bug with sessions and transactions.
Also, if that one with the session fix is accepted, then there's this PR: #231 that would supersede this one, as that one includes those session fixes for the new
SyncEngine. In that case, that would be the one that should be merged and not this PR.Implementation Details
There's a new
SyncEnginebesides theAIOEngine. Internally it usespymongo, the official sync driver. It's API is pretty much the same as that one ofmotor, so the code changes are minimal, most of the code is copied from the existing implementation and adapted to not useasyncandawaitin those cases.I also refactored the Engine into a
BaseEnginewith all the logic that can be shared (query preparation, etc) and the two engines that inherit from it,AIOEngineandSyncEngine.Tests
For the tests, I copied each one of the tests that interact with the async engine and updated the copy to use the sync engine, making it non-async, updating mock checks, wrapping the iterator of
sync_engine.find()intolistfor the tests, etc.I considered having a separate file for the sync tests, but I thought it would be better to have each test very close to the original, that way it will be easier to maintain them, apply refactors, make sure they are in sync, and whenever a new test is added it will be easier to remember to add the async and sync version.
All the tests are currently passing.
I'm still figuring out tox, I have never used it. Tox includes pymongo versions. ✅Dependencies
Note: see the edit note in this section below.
Previous / old comment from 2022-06-09
It would make sense to not require
motoralways, as people not usingasyncwould still be able to use all of ODMantic in a sync way. I think it would make sense to have optional extra requirements to installmotororpymongo.That way developers can install with:
$ pip install "odmantic[motor]"or:
$ pip install "odmantic[pymongo]"Or even both:
$ pip install "odmantic[motor,pymongo]"Those names could also be changed, for example
odmantic[async]andodmantic[sync]if that feels better, it would still installmotorandpymongorespectively. For now, I put them as justodmantic[motor]andodmantic[pymongo].2022-06-09T16:00:00Z Edit
Tweaking tox I realized that
motordepends onpymongo, so,pymongowould always be required, for just sync or also for async. And thenmotorwould be needed only for async.In that case, it could make sense to make
pymongoalways required and make the optional extras just forodmantic[motor].The other option is to keep both optional extras, I think the only possible advantage of keepingodmantic[pymongo]as an independent option would be if for any chance Motor could end up not depending on PyMongo, but I wouldn't think that's really feasible.What do you think?2022-06-22T13:38:00Z Edit
I realized Motor deeply depends on PyMongo, for example for bulk_write(), so I think it makes sense to make PyMongo always required, and Motor optional for async support. But let me now if it doesn't make sense to you!
Installing with Motor support looks like:
$ pip install "odmantic[motor]"Next steps
Figure out tox and update it to include.pymongoI'll work on that now. Done! ✅I'll update the docs next but I want to first wait for feedback on this PR first.
Future work
I also want to propose a new feature to allow changing the collection used during an operation that I'm needing. Similar to MongoEngine's
switch_collection()... although the implementation for that one is not thread nor async safe, so that one is not really very useful in all the cases and might be dangerous. I'll propose something equivalent but safe for threads and contextvars (e.g. safe with FastAPI and any other web frameworks).After that, I want to propose other features to use Mongo's batch updates for saving multiple documents instead of many atomic operations per document.
But all this would be after this initial step with the sync engine.