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.

Sign up for GitHub

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

Jump to bottom

PYTHON-4533 Convert essential tests to async for Async PyMongo beta release #1724

Closed
wants to merge 15 commits into from
Closed

PYTHON-4533 Convert essential tests to async for Async PyMongo beta release #1724

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
8c96646
PYTHON-4530 - Synchronized test code should be in top-level test dire…
NoahStapp Jun 27, 2024
17a6761
Convert test_client to async
NoahStapp Jul 1, 2024
ad48086
Fix typechecking
NoahStapp Jul 1, 2024
73d3b19
Convert test/test_cursor.py to async
NoahStapp Jul 1, 2024
46fbc97
Fix test/__init__.py docstring
NoahStapp Jul 2, 2024
d588167
Convert test/test_transactions.py
NoahStapp Jul 2, 2024
427e4b5
Convert test/test_session.py to async
NoahStapp Jul 3, 2024
9c1bf99
Fix typechecking
NoahStapp Jul 3, 2024
985c25c
Merge branch 'master' into async-beta-tests
NoahStapp Jul 3, 2024
665f730
Use compatibility anext/next
NoahStapp Jul 3, 2024
0172ff3
fix failing replicaset tests
NoahStapp Jul 3, 2024
a0652ce
Fix tearDownClass for async tests
NoahStapp Jul 3, 2024
c00c15b
Only test contextlib.aclosing on >= 3.10
NoahStapp Jul 3, 2024
2850ed7
Fix async test_contextlib
NoahStapp Jul 3, 2024
6457f6c
Merge branch 'master' into async-beta-tests
NoahStapp Jul 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion gridfs/asynchronous/grid_file.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ async def put(self, data: Any, **kwargs: Any) -> Any:
"""
async with AsyncGridIn(self._collection, **kwargs) as grid_file:
await grid_file.write(data)
return await grid_file._id
return grid_file._id

async def get(self, file_id: Any, session: Optional[AsyncClientSession] = None) -> AsyncGridOut:
"""Get a file from GridFS by ``"_id"``.
Expand Down
17 changes: 13 additions & 4 deletions pymongo/asynchronous/client_session.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,18 +138,21 @@
import collections
import time
import uuid
from asyncio import iscoroutinefunction
from collections.abc import Mapping as _Mapping
from typing import (
TYPE_CHECKING,
Any,
AsyncContextManager,
Callable,
Coroutine,
Mapping,
MutableMapping,
NoReturn,
Optional,
Type,
TypeVar,
Union,
)

from bson.binary import Binary
Expand Down Expand Up @@ -598,7 +601,10 @@ def _inherit_option(self, name: str, val: _T) -> _T:

async def with_transaction(
self,
callback: Callable[[AsyncClientSession], _T],
callback: Union[
Callable[[AsyncClientSession], Coroutine[Any, Any, _T]],
Copy link
Member

Choose a reason for hiding this comment

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

Should we use Coroutine here or Awaitable? Which is a better fit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We've used Coroutine consistently so far, Awaitable is a less-specific type hint that allows several other objects besides asynchronous functions.

Callable[[AsyncClientSession], _T],
Copy link
Member

Choose a reason for hiding this comment

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

Why are there two callables here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An artifact of the reasoning below, removed.

],
read_concern: Optional[ReadConcern] = None,
write_concern: Optional[WriteConcern] = None,
read_preference: Optional[_ServerMode] = None,
Expand Down Expand Up @@ -693,7 +699,10 @@ async def callback(session, custom_arg, custom_kwarg=None):
read_concern, write_concern, read_preference, max_commit_time_ms
)
try:
ret = callback(self)
if not _IS_SYNC and iscoroutinefunction(callback):
Copy link
Member

Choose a reason for hiding this comment

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

Why check IS_SYNC or iscoroutinefunction here? Shouldn't it just be:

ret = await callback(self)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was trying to be more lenient and allow both async and sync callbacks, but it's better to require all asynchronous callbacks for the Async API.

ret = await callback(self) # type: ignore[assignment]
else:
ret = callback(self) # type: ignore[assignment]
except Exception as exc:
if self.in_transaction:
await self.abort_transaction()
Expand All @@ -708,7 +717,7 @@ async def callback(session, custom_arg, custom_kwarg=None):

if not self.in_transaction:
# Assume callback intentionally ended the transaction.
return ret
return ret # type: ignore[return-value]

while True:
try:
Expand All @@ -730,7 +739,7 @@ async def callback(session, custom_arg, custom_kwarg=None):
raise

# Commit succeeded.
return ret
return ret # type: ignore[return-value]

async def start_transaction(
self,
Expand Down
2 changes: 1 addition & 1 deletion pymongo/asynchronous/command_cursor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -424,4 +424,4 @@ def _unpack_response( # type: ignore[override]
return raw_response # type: ignore[return-value]

def __getitem__(self, index: int) -> NoReturn:
raise InvalidOperation("Cannot call __getitem__ on RawBatchCursor")
raise InvalidOperation("Cannot call __getitem__ on AsyncRawBatchCommandCursor")
2 changes: 1 addition & 1 deletion pymongo/asynchronous/cursor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1308,4 +1308,4 @@ async def explain(self) -> _DocumentType:
return await clone.explain()

def __getitem__(self, index: Any) -> NoReturn:
raise InvalidOperation("Cannot call __getitem__ on RawBatchCursor")
raise InvalidOperation("Cannot call __getitem__ on AsyncRawBatchCursor")
9 changes: 8 additions & 1 deletion pymongo/asynchronous/mongo_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -861,6 +861,10 @@ def __init__(
# This will be used later if we fork.
AsyncMongoClient._clients[self._topology._topology_id] = self

async def connect(self) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

"Connect" might not be the best name here since we're not actually connecting. I know we have the "connect=True" argument but I can't help but feel like "open()" would be more appropriate. If a user actually wanted to connect I would expect them to run something like "db.command('ping')". What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It feels a bit odd to me for the synchronous API to have a connect=True option but the equivalent async option is called open instead even though they do the same thing. Consistency of naming makes more sense to me here.

Copy link
Member

Choose a reason for hiding this comment

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

In that case I think the docstring just needs to explain the concept more clearly.

"""Explicitly connect asynchronously."""
await self._get_topology()

def _init_background(self, old_pid: Optional[int] = None) -> None:
self._topology = Topology(self._topology_settings)
# Seed the topology with the old one's pid so we can detect clients
Expand Down Expand Up @@ -1360,7 +1364,7 @@ async def __aexit__(self, exc_type: Any, exc_val: Any, exc_tb: Any) -> None:
__iter__ = None

def __next__(self) -> NoReturn:
raise TypeError("'MongoClient' object is not iterable")
raise TypeError("'AsyncMongoClient' object is not iterable")

next = __next__

Expand Down Expand Up @@ -1517,6 +1521,9 @@ async def close(self) -> None:
# TODO: PYTHON-1921 Encrypted MongoClients cannot be re-opened.
await self._encrypter.close()

async def aclose(self) -> None:
await self.close()
Copy link
Member

Choose a reason for hiding this comment

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

Public API should have a docstring.

Copy link
Member

Choose a reason for hiding this comment

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

Why do we need aclose() when we already have close()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

contextlib expects that asynchronous context managers use aclose. We could simplify by just setting aclose = close instead.

Copy link
Member

Choose a reason for hiding this comment

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

In that case should we remove "close()"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that's better, yeah.


async def _get_topology(self) -> Topology:
"""Get the internal :class:`~pymongo.topology.Topology` object.

Expand Down
16 changes: 12 additions & 4 deletions pymongo/synchronous/client_session.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@
import collections
import time
import uuid
from asyncio import iscoroutinefunction
from collections.abc import Mapping as _Mapping
from typing import (
TYPE_CHECKING,
Expand All @@ -150,6 +151,7 @@
Optional,
Type,
TypeVar,
Union,
)

from bson.binary import Binary
Expand Down Expand Up @@ -598,7 +600,10 @@ def _inherit_option(self, name: str, val: _T) -> _T:

def with_transaction(
self,
callback: Callable[[ClientSession], _T],
callback: Union[
Callable[[ClientSession], _T],
Callable[[ClientSession], _T],
],
Comment on lines +603 to +606
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the type definition is the same twice here. Is that needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this is an artifact of supporting both kinds of callbacks in the async API. Will be resolved in the separate PR for test_transaction

read_concern: Optional[ReadConcern] = None,
write_concern: Optional[WriteConcern] = None,
read_preference: Optional[_ServerMode] = None,
Expand Down Expand Up @@ -691,7 +696,10 @@ async def callback(session, custom_arg, custom_kwarg=None):
while True:
self.start_transaction(read_concern, write_concern, read_preference, max_commit_time_ms)
try:
ret = callback(self)
if not _IS_SYNC and iscoroutinefunction(callback):
ret = callback(self) # type: ignore[assignment]
blink1073 marked this conversation as resolved.
Show resolved Hide resolved
else:
ret = callback(self) # type: ignore[assignment]
except Exception as exc:
if self.in_transaction:
self.abort_transaction()
Expand All @@ -706,7 +714,7 @@ async def callback(session, custom_arg, custom_kwarg=None):

if not self.in_transaction:
# Assume callback intentionally ended the transaction.
return ret
return ret # type: ignore[return-value]

while True:
try:
Expand All @@ -728,7 +736,7 @@ async def callback(session, custom_arg, custom_kwarg=None):
raise

# Commit succeeded.
return ret
return ret # type: ignore[return-value]

def start_transaction(
self,
Expand Down
2 changes: 1 addition & 1 deletion pymongo/synchronous/command_cursor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -424,4 +424,4 @@ def _unpack_response( # type: ignore[override]
return raw_response # type: ignore[return-value]

def __getitem__(self, index: int) -> NoReturn:
raise InvalidOperation("Cannot call __getitem__ on RawBatchCursor")
raise InvalidOperation("Cannot call __getitem__ on RawBatchCommandCursor")
7 changes: 7 additions & 0 deletions pymongo/synchronous/mongo_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -860,6 +860,10 @@ def __init__(
# This will be used later if we fork.
MongoClient._clients[self._topology._topology_id] = self

def connect(self) -> None:
"""Explicitly connect synchronously."""
self._get_topology()
Copy link
Member

Choose a reason for hiding this comment

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

We should avoid adding connect() and aclose() to the sync client.

Copy link
Contributor Author

@NoahStapp NoahStapp Jul 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

What if we rename them to be private for the synchronous client? Like aconnect -> _aconnect and aclose -> _aclose? There isn't a clean way of having the synchro process just not convert specific methods.

Copy link
Member

Choose a reason for hiding this comment

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

How about something like this?:

_IS_SYNC = False
class AsyncMongoClient:
    if not _IS_SYNC:
        async def aclose(self):
            pass

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A better solution for aclose is to just rename it to close when it's synchronized. That way all async code will use aclose and all sync code will use close without any extra work. For aconnect what about this:

async def aconnect(self):
	if _IS_SYNC:
		pass
	else:
		...

Copy link
Member

@ShaneHarvey ShaneHarvey Jul 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

aclose -> close SGTM. For aconnect() I would prefer the approach I outlined above so that the method is never added to the sync client.

Copy link
Member

Choose a reason for hiding this comment

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

Actually. renaming aconnect -> _aconnect is fine too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The synchro process will still convert the aconnect method within the if not _IS_SYNC block and add it to the sync client source. Is that acceptable?

Copy link
Member

Choose a reason for hiding this comment

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

I'm good with any approach as long as aconnect() or connect() isn't a public method on MongoClient.


def _init_background(self, old_pid: Optional[int] = None) -> None:
self._topology = Topology(self._topology_settings)
# Seed the topology with the old one's pid so we can detect clients
Expand Down Expand Up @@ -1512,6 +1516,9 @@ def close(self) -> None:
# TODO: PYTHON-1921 Encrypted MongoClients cannot be re-opened.
self._encrypter.close()

def aclose(self) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as the comment from @ShaneHarvey before. We can remove close

self.close()

def _get_topology(self) -> Topology:
"""Get the internal :class:`~pymongo.topology.Topology` object.

Expand Down
Loading
Loading