PYTHON-4476 Separate data and IO classes more effectively #1678

NoahStapp · 2024-06-12T22:25:18Z

This PR refactors a significant amount of our codebase to reduce code duplication and more clearly separate network IO from data handling and business logic. There's more to be done, but breaking up future changes into smaller, more manageable PRs is a much better and more maintainable strategy. The changes here are largely circular in nature with import and dependency graphs, making it difficult to separate them out into multiple PRs.

Core changes:

Moved client_options.py, collation.py, common.py, compression_support.py, encryption_options.py, event_loggers.py, hello.py, logger.py, max_staleness_selectors.py, message.py, monitoring.py, operations.py, read_preferences.py, response.py, server_description.py, server_selectors.py, srv_resolver.py, topology_description.py, uri_parser.py out of asynchronous and into the top-level package. This significantly reduces the amount of code duplication done by synchronization, and should result in only files with actual async def methods staying inside asynchronous.
Separated PoolOptions and most of the helpers and constants out from pool.py into their own shared pool_options.py file.
Moved many of the helpers, constants, and data classes of the remaining asynchronous files into NAME_shared.py files inside the top-level package.
Removed all network IO from message.py. _Query, _GetMore, _BulkWriteContext, _EncryptedBulkWriteContext are all now pure data classes that rely on their users (bulk.py and server.py) to perform network IO.

The coloring nature of asynchronous code encourages very clear separation of network IO from the rest of the codebase. Going forward, I recommend we try to avoid bundling IO inside classes in order to reduce code duplication in the synchronization process.

ShaneHarvey

+8,955 −19,746

nice. Just one quick comment. I'll need to spend more time to review this in the next day or two.

gridfs/synchronous/grid_file.py

test/utils_selection_tests.py

ShaneHarvey · 2024-06-20T21:38:45Z

test/unified_format.py

@@ -616,7 +616,7 @@ def get_lsid_for_session(self, session_name):
 session = self[session_name]
 if not isinstance(session, ClientSession):
 self.test.fail(
- f"Expected entity {session_name} to be of type ClientSession, got {type(session)}"
+ f"Expected entity {session_name} to be of type AsyncClientSession, got {type(session)}"


This one should be ClientSession, right?

ShaneHarvey · 2024-06-20T21:43:46Z

test/synchronous/test_collection.py

@@ -39,13 +39,13 @@
 wait_until,
 )

-from bson import encode
+from bson import RawBSONDocument, encode


Does this file ever get run? Seems like a duplicate of test/test_collection.py

This is generated from test/asynchronous/test_collection.py, left as an example result for our future test asynchronization work

ShaneHarvey · 2024-06-20T21:45:11Z

test/asynchronous/test_collection.py

@@ -40,21 +40,17 @@
 wait_until,
 )

-from bson import encode
+from bson import RawBSONDocument, encode


Does test/asynchronous/ ever get run in CI?

No, left in as an example for our future test asynchronization work

pymongo/message.py

ShaneHarvey · 2024-06-20T21:52:45Z

pymongo/synchronous/monitoring.py

@@ -1,1903 +0,0 @@
-# Copyright 2015-present MongoDB, Inc.


How come this file doesn't show up as a rename like message.py does? Any way we can have git recognize it to minimize code churn?

I have the same question for many of the other files as well.

Seems like it's just git being confused. Let me do some commit reworking, I think I can significantly improve it.

I was wrong sadly, there isn't a good way to do this. Because we are essentially removing a lot of the asynchronous and synchronous files and putting a single version into the top-level pymongo package, it complicates the history a lot for git. Git doesn't track renames/moves really, just the changes, so a "rename" just means it had few enough changes along with the location move to qualify.

ShaneHarvey · 2024-06-20T21:56:41Z

pymongo/synchronous/server.py

@@ -121,26 +135,26 @@ def run_operation(
 cursors.
 Can raise ConnectionFailure, OperationFailure, etc.

- :param conn: A Connection instance.
+ :param conn: An AsyncConnection instance.


"An AsyncConnection" is incorrect here.

ShaneHarvey · 2024-06-20T21:57:25Z

pymongo/synchronous/server.py

 :param operation: A _Query or _GetMore object.
 :param read_preference: The read preference to use.
 :param listeners: Instance of _EventListeners or None.
 :param unpack_res: A callable that decodes the wire protocol response.
+ :param client: An AsyncMongoClient instance.


AsyncMongoClient is incorrect here.

ShaneHarvey · 2024-06-24T22:06:55Z

pymongo/synchronous/bulk.py

@@ -180,7 +91,7 @@ def __init__(
 comment: Optional[str] = None,
 let: Optional[Any] = None,
 ) -> None:
- """Initialize a _Bulk instance."""
+ """Initialize a _AsyncBulk instance."""


_Bulk -> _AsyncBulk

ShaneHarvey · 2024-06-24T22:12:50Z

pymongo/asynchronous/bulk.py

@@ -407,7 +509,23 @@ async def _execute_command(
 if self.ordered and "writeErrors" in result:
 break
 else:
- to_send = await bwc.execute_unack(cmd, ops, client)
+ if self.is_encrypted:


Can this be refactored into a helper method? Something like:

result, to_send = await self._execute_batch_unack(cmd, ops, client)

We can also use the new helper in execute_op_msg_no_results down below.

ShaneHarvey · 2024-06-24T22:13:05Z

pymongo/asynchronous/bulk.py

@@ -387,7 +475,21 @@ async def _execute_command(

 # Run as many ops as possible in one command.
 if write_concern.acknowledged:
- result, to_send = await bwc.execute(cmd, ops, client)
+ if self.is_encrypted:


Can this be refactored into a helper method? Something like:

result, to_send = await self._execute_batch(cmd, ops, client)

ShaneHarvey · 2024-06-24T22:15:05Z

pymongo/asynchronous/bulk.py

+ result = await self.write_command(
+ bwc, cmd, request_id, msg, to_send, client
+ )
+ await client._process_response(result, bwc.session)


Shouldn't client._process_response be called in the encrypted case as well?

Jibola

Great so far! Got as far as all of the non-test files.

I decided I'd break it off and take the tests in a separate review!

Jibola · 2024-06-25T21:44:00Z

pymongo/message.py

+class _CursorAddress(tuple):
+ """The server address (host, port) of a cursor, with namespace property."""
+
+ __namespace: Any
+
+ def __new__(cls, address: _Address, namespace: str) -> _CursorAddress:
+ self = tuple.__new__(cls, address)
+ self.__namespace = namespace
+ return self


Out of scope for this PR, but why is this a __new__. call instead of an __init__?

I don't know, maybe something to do with tuple?

Yeah it's a tuple thing.

Jibola · 2024-06-25T22:02:37Z

pymongo/uri_parser.py

+if TYPE_CHECKING:
+ from pymongo.pyopenssl_context import SSLContext
+
+_IS_SYNC = False


is _IS_SYNC needed for a file like this?

Also, I think the file is synchronous.

Good catch! Those are all artifacts from moving these files out of pymongo/asynchronous, fixed.

Jibola · 2024-06-25T22:03:13Z

pymongo/topology_description.py

+ s.min_wire_version,
+ common.MAX_SUPPORTED_WIRE_VERSION,
+ )
+ )


Would you know if this if statements also needs a break?

I don't think it makes a difference in behavior, as if the if block is executed the elif block won't be.

ShaneHarvey

LGTM, not a fan of all the type ignores but I can't think of a better option right now. Nice work!

Jibola

Last comment!

Jibola · 2024-06-26T14:00:22Z

pymongo/collation.py

@@ -1,4 +1,4 @@
-# Copyright 2024-present MongoDB, Inc.
+# Copyright 2016-present MongoDB, Inc.


Suggested change

# Copyright 2016-present MongoDB, Inc.

# Copyright 2016-present MongoDB, Inc.

What's the criteria for the year changes? I see some read 2014, 2020, etc. How are we determining what year to set per-file?

They should reflect the year the original, pre-async file was committed.

Jibola

LGTM

NoahStapp added 6 commits June 6, 2024 16:58

PYTHON-4476 Separate data and IO classes more effectively

cf42518

WIP message refactor

7225675

Merge branch 'master' into PYTHON-4476

38d9853

Move message out of asynchronous

a5ca429

Remove unused abstract parent classes

7126a17

Merge branch 'master' into PYTHON-4476

b8d654a

mongodb-drivers-pr-bot bot requested a review from Jibola June 12, 2024 22:31

ShaneHarvey requested changes Jun 12, 2024

View reviewed changes

gridfs/synchronous/grid_file.py Outdated Show resolved Hide resolved

NoahStapp added 2 commits June 12, 2024 16:06

Fix gridfs synchronous docstrings

55ff5f0

Merge hello_compat and hello

1eb50d4

NoahStapp requested a review from ShaneHarvey June 17, 2024 16:36

Merge branch 'master' into PYTHON-4476

73b156c

ShaneHarvey requested changes Jun 20, 2024

View reviewed changes

Address review

74d7f61

NoahStapp requested a review from ShaneHarvey June 20, 2024 23:41

Update async test_collection

161de21

NoahStapp mentioned this pull request Jun 24, 2024

PYTHON-4520 Add test-async tox command and automated async testing #1699

Merged

ShaneHarvey requested changes Jun 24, 2024

View reviewed changes

NoahStapp added 4 commits June 24, 2024 15:18

Resolve merge conflicts

a3b15f0

Merge branch 'master' into PYTHON-4476

49bc887

Fix bulk typechecking errors

d4559b7

Merge branch 'master' into PYTHON-4476

8f1fd4d

NoahStapp requested a review from ShaneHarvey June 25, 2024 22:01

Jibola requested changes Jun 25, 2024

View reviewed changes

Remove _IS_SYNC from moved modules

b5574ec

NoahStapp requested a review from Jibola June 25, 2024 22:08

ShaneHarvey approved these changes Jun 25, 2024

View reviewed changes

Jibola requested changes Jun 26, 2024

View reviewed changes

NoahStapp requested a review from Jibola June 26, 2024 17:03

Jibola approved these changes Jun 26, 2024

View reviewed changes

NoahStapp merged commit ffa6555 into mongodb:master Jun 26, 2024
34 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PYTHON-4476 Separate data and IO classes more effectively #1678

PYTHON-4476 Separate data and IO classes more effectively #1678

NoahStapp commented Jun 12, 2024 •

edited

Loading

ShaneHarvey left a comment

ShaneHarvey Jun 20, 2024

ShaneHarvey Jun 20, 2024

NoahStapp Jun 20, 2024 •

edited

Loading

ShaneHarvey Jun 20, 2024 •

edited

Loading

NoahStapp Jun 20, 2024

ShaneHarvey Jun 20, 2024

NoahStapp Jun 20, 2024

NoahStapp Jun 20, 2024

ShaneHarvey Jun 20, 2024

ShaneHarvey Jun 20, 2024

ShaneHarvey Jun 24, 2024

ShaneHarvey Jun 24, 2024

ShaneHarvey Jun 24, 2024

ShaneHarvey Jun 24, 2024

Jibola left a comment

Jibola Jun 25, 2024

NoahStapp Jun 25, 2024

ShaneHarvey Jun 25, 2024

Jibola Jun 25, 2024

NoahStapp Jun 25, 2024

Jibola Jun 25, 2024

NoahStapp Jun 25, 2024

ShaneHarvey left a comment

Jibola left a comment

Jibola Jun 26, 2024

NoahStapp Jun 26, 2024

Jibola left a comment

		@@ -1,4 +1,4 @@
		# Copyright 2024-present MongoDB, Inc.
		# Copyright 2016-present MongoDB, Inc.

PYTHON-4476 Separate data and IO classes more effectively #1678

PYTHON-4476 Separate data and IO classes more effectively #1678

Conversation

NoahStapp commented Jun 12, 2024 • edited Loading

ShaneHarvey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NoahStapp Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

ShaneHarvey Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jibola left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaneHarvey left a comment

Choose a reason for hiding this comment

Jibola left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jibola left a comment

Choose a reason for hiding this comment

NoahStapp commented Jun 12, 2024 •

edited

Loading

NoahStapp Jun 20, 2024 •

edited

Loading

ShaneHarvey Jun 20, 2024 •

edited

Loading