PYTHON-3052 Add Typings to PyMongo Itself #829

blink1073 · 2021-12-23T16:19:22Z

Finish manual port of monitoring.py, server_description, server_type, topology_description, url_parser (all failed when using retype)
Double check all public interfaces in the auto-ported files:

     modified:   pymongo/__init__.py
	modified:   pymongo/collation.py
	modified:   pymongo/common.py
	modified:   pymongo/event_loggers.py
	modified:   pymongo/operations.py
	modified:   pymongo/read_concern.py
	modified:   pymongo/read_preferences.py
	modified:   pymongo/results.py
	modified:   pymongo/write_concern.py

Also look at addressing Investigate if we can use something better than Any for documents returned from find, aggregate etc... mongodb-labs/pymongo-stubs#15
- Used Union[MutableMapping[str, Any], RawBSONDocumentRef]
- Investigate if we can make Collection a generic object

ShaneHarvey

Great work so far! I've reviewed the bson/ and gridfs/ files and think it's probably a good time to stop and address the various comments. Some of them are applicable to all the changes as a whole.

bson/binary.py

bson/__init__.py

ShaneHarvey · 2022-01-11T18:50:15Z

bson/codec_options.py

@@ -92,6 +98,9 @@ class TypeCodec(TypeEncoder, TypeDecoder):
    pass


+Codec = Union[TypeEncoder, TypeDecoder, TypeCodec]
+Fallback = Callable[[Any], Any]


Should these names be private to avoid exporting them?

Do we want to do that for all types, including the ones in pymongo.typing?

I'd say yes since we don't want people to accidentally import them or depend on them yet. We try to make everything private by default.

Sounds good, updated

ShaneHarvey · 2022-01-11T19:25:39Z

bson/json_util.py

@@ -781,8 +789,8 @@ def default(obj, json_options=DEFAULT_JSON_OPTIONS):
            if not obj.tzinfo:
                obj = obj.replace(tzinfo=utc)
            if obj >= EPOCH_AWARE:
-                off = obj.tzinfo.utcoffset(obj)
-                if (off.days, off.seconds, off.microseconds) == (0, 0, 0):
+                off = obj.tzinfo.utcoffset(obj)  # type: ignore


Instead of type: ignore, can we tag this as timedelta?

I added an assert obj.tzinfo is not None above and was able to remove the ignore.

Oh I see. Is this a bug/limitation in mypy? I would expect it to understand that tzinfo always exists here since we have these lines above:

if not obj.tzinfo: obj = obj.replace(tzinfo=utc)

I moved the assert below that line for clarity

That does seem like a limitation, but I wouldn't call it a bug, that would take some serious introspection

gridfs/__init__.py

ShaneHarvey · 2022-01-12T00:09:13Z

gridfs/__init__.py

-                                   session=None):
+    def download_to_stream_by_name(self, filename: str, destination: Any,
+                                   revision: Optional[int] = -1,
+                                   session: Optional[ClientSession] = None) -> None:


I saw elsewhere you used NoReturn. Should we change all the -> None: methods to use it too?

Those are for functions that always raise an exception

Oh TIL. Thanks.

ShaneHarvey · 2022-01-12T00:14:12Z

gridfs/grid_file.py

        """Read one line or up to `size` bytes from the file.

        :Parameters:
         - `size` (optional): the maximum number of bytes to read
        """
+        if size is None:
+            size = -1


Why this change? The api does not support size=None and I'd like to avoid adding misc features in this PR.

This is another case of Optional[int]. In this case the superclass also uses it, we'd have to add an # ignore: override:

gridfs/grid_file.py:575: error: Argument 1 of "readline" is incompatible with supertype "IOBase"; supertype defines the argument type as "Optional[int]" gridfs/grid_file.py:575: note: This violates the Liskov substitution principle gridfs/grid_file.py:575: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides

I think the issue is the type should be size: int instead of size: Optional[int]. Size is not compatible with Optional because None is not supported (the default is -1, not None).

Agreed, but because io.IOBase uses Optional[int], we have to add an ignore override comment. I'll do that.

Oh I see. Yeah override is what typeshed does: https://github.com/python/typeshed/blob/df0fe1645681638359a4ab706732e79c62db6a71/stdlib/io.pyi#L128

gridfs/grid_file.py

ShaneHarvey · 2022-01-12T23:08:51Z

bson/codec_options.py

@@ -92,6 +98,9 @@ class TypeCodec(TypeEncoder, TypeDecoder):
    pass


+Codec = Union[TypeEncoder, TypeDecoder, TypeCodec]
+Fallback = Callable[[Any], Any]


I'd say yes since we don't want people to accidentally import them or depend on them yet. We try to make everything private by default.

ShaneHarvey · 2022-01-12T23:13:03Z

bson/__init__.py

-BSONDEC = b"\x13" # Decimal128
-BSONMIN = b"\xFF" # Min key
-BSONMAX = b"\x7F" # Max key
+EPOCH_AWARE: datetime.datetime = datetime.datetime.fromtimestamp(0, utc)


Can you expand on that? In my testing I found they are not required even with strict mode:

$ cat typ.py NUM = 1 def meth(x: int) -> int: return x * 2 print(meth(NUM)) $ mypy --strict typ.py Success: no issues found in 1 source file

ShaneHarvey · 2022-01-12T23:18:28Z

bson/regex.py

@@ -49,7 +50,7 @@ class Regex(object):
    _type_marker = 11

    @classmethod
-    def from_native(cls, regex):
+    def from_native(cls: Type["Regex"], regex: Pattern[Any]) -> "Regex":


Great thanks.

ShaneHarvey · 2022-01-12T23:20:10Z

gridfs/__init__.py

-                                   session=None):
+    def download_to_stream_by_name(self, filename: str, destination: Any,
+                                   revision: Optional[int] = -1,
+                                   session: Optional[ClientSession] = None) -> None:


Oh TIL. Thanks.

ShaneHarvey · 2022-01-12T23:23:22Z

gridfs/grid_file.py

        """Read one line or up to `size` bytes from the file.

        :Parameters:
         - `size` (optional): the maximum number of bytes to read
        """
+        if size is None:
+            size = -1


I think the issue is the type should be size: int instead of size: Optional[int]. Size is not compatible with Optional because None is not supported (the default is -1, not None).

ShaneHarvey · 2022-01-12T23:53:36Z

bson/son.py

-            return memo.get(val_id)
+            value = memo.get(val_id)
+            assert value is not None
+            return value


Can we replace the redundant assert and temp variable with: return memo[val_id]?

Ideally if val_id in memo would guard against the get() being None, but it does not, which is why I added the intermediate variable and guard.

Yeah I agree but I think it's similar to the other issue since mypy can't determine if the val_id might be removed by another thread in between the if-statement and the get() call. Regardless, I think we can replace these 3 lines with return memo[val_id] since we know val_id is always there. Then we don't need to add redundant code just to appease mypy.

Used type ignore instead

bson/son.py

bson/timestamp.py

gridfs/__init__.py

blink1073 · 2022-01-13T15:45:52Z

Note: I removed the changes to pymongo and test in order to make this PR easier to review. I will follow up with those in other PRs.

ShaneHarvey

Thanks for pushing out the pymongo/+test/ changes!

ShaneHarvey · 2022-01-13T20:12:54Z

gridfs/grid_file.py

        """Read one line or up to `size` bytes from the file.

        :Parameters:
         - `size` (optional): the maximum number of bytes to read
        """
+        if size is None:
+            size = -1


Oh I see. Yeah override is what typeshed does: https://github.com/python/typeshed/blob/df0fe1645681638359a4ab706732e79c62db6a71/stdlib/io.pyi#L128

test/mockupdb/operations.py

ShaneHarvey · 2022-01-13T20:17:57Z

test/performance/perf_test.py

@@ -44,7 +45,7 @@

 OUTPUT_FILE = os.environ.get('OUTPUT_FILE')

-result_data = []
+result_data: List[Any] = []


Isn't this annotation redundant?

test/performance/perf_test.py:48: error: Need type annotation for "result_data" (hint: "result_data: List[<type>] = ...")

Oh that's surprising. I thought it would default to List. TIL.

It defaulted to List[unknown], but adding : List allowed it to reference as List[Any], so the [Any] was redundant.

doc/changelog.rst

ShaneHarvey · 2022-01-13T20:24:22Z

bson/son.py

-            return memo.get(val_id)
+            value = memo.get(val_id)
+            assert value is not None
+            return value


Yeah I agree but I think it's similar to the other issue since mypy can't determine if the val_id might be removed by another thread in between the if-statement and the get() call. Regardless, I think we can replace these 3 lines with return memo[val_id] since we know val_id is always there. Then we don't need to add redundant code just to appease mypy.

ShaneHarvey · 2022-01-13T21:21:36Z

bson/son.py

        val_id = id(self)
        if val_id in memo:
-            return memo.get(val_id)
+            return memo.get(val_id)  # type: ignore


We can make this type-safe and remove # type: ignore by using return memo[val_id] instead of .get().

Oh, ha, duh, thanks

ShaneHarvey

LGTM!

blink1073 added 30 commits December 23, 2021 10:18

PYTHON-3052 Add Typings to PyMongo Itself

194a5f3

fix install

b521234

fix install

f67b92e

finish bson

f885e6f

Finish gridfs typing

ed345c1

remove use of Final class

d4d87ee

remove use of typeguard

5e971a4

clean up more typings

6c326d8

clean up more typings

8202985

clean up more typings

2081719

clean up imports

ca069e8

clean up more typings

e4be658

clean up more typings

d838fa8

clean up more typings

d27c852

clean up more typings

1258b66

clean up more typings

539dd5b

fix use of class name vs variable

04d3a68

try fixing pypy

7b4a701

try fixing pypy

a0ee1d9

try fixing pypy

fa0de64

wip update pymongo typings

56a66ad

finish monitoring.py

c46c993

finish other manual files

0cd8a08

update auto-typed files

af8f0cf

fix syntax errors

37c4089

wip clean up typing errors

89a156a

wip clean up typing errors

c9cf483

all types passing

751e789

more type cleanup

789dc32

wip fix circular imports

ea8bc42

blink1073 added 5 commits January 11, 2022 15:08

clean up imports

a72e46e

add more generics

5979dc1

add more generics

d7b5395

check untyped definitions

42486d0

clean up test_database

c7e2c13

ShaneHarvey requested changes Jan 12, 2022

View reviewed changes

blink1073 added 7 commits January 11, 2022 18:19

clean up test_database

f1ab0a0

turn of check_untyped_defs for now

e17ed24

run isort on the files

0f12b61

address review

ece28aa

cleanup typings

fd5108a

fix test and typings

b896ddd

fix default value

ecd8493

ShaneHarvey requested changes Jan 13, 2022

View reviewed changes

blink1073 added 4 commits January 13, 2022 08:32

undo changes to pymongo tree

aeafa6e

undo changes to test tree

a0dbf9b

fix type erorr in gridfs

ba60eb3

address review

1e7cc35

fix rst formatting

3bb8388

ShaneHarvey requested changes Jan 13, 2022

View reviewed changes

address review

185ea17

ShaneHarvey reviewed Jan 13, 2022

View reviewed changes

address review

4586fd1

ShaneHarvey approved these changes Jan 13, 2022

View reviewed changes

blink1073 merged commit 52ed5a4 into mongodb:master Jan 13, 2022

blink1073 deleted the PYTHON-3052 branch January 13, 2022 22:09

blink1073 mentioned this pull request Jan 26, 2022

PYTHON-3060 Add typings to pymongo package #831

Merged

juliusgeo pushed a commit to juliusgeo/mongo-python-driver that referenced this pull request Apr 5, 2022

PYTHON-3052 Add Typings to PyMongo Itself (mongodb#829)

3b8b7bb

juliusgeo pushed a commit to juliusgeo/mongo-python-driver that referenced this pull request Apr 7, 2022

PYTHON-3052 Add Typings to PyMongo Itself (mongodb#829)

924216d

PYTHON-3052 Add Typings to PyMongo Itself #829

PYTHON-3052 Add Typings to PyMongo Itself #829

Uh oh!

Conversation

blink1073 commented Dec 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ShaneHarvey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

blink1073 commented Jan 13, 2022

Uh oh!

ShaneHarvey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

blink1073 commented Dec 23, 2021 •

edited

Loading

ShaneHarvey Jan 13, 2022 •

edited

Loading