mypy: upgrade to 1.10.0 #156

achille-roussel · 2024-04-25T02:10:04Z

This PR upgrade mypy to 1.10.0, and enable --check-untyped-defs which does a much stricter pass. I then fixed all the type checks that were newly reported.

Signed-off-by: Achille Roussel <achille.roussel@gmail.com>

achille-roussel · 2024-04-25T02:14:46Z

src/dispatch/function.py

@@ -256,9 +256,9 @@ def primitive_func(input: Input) -> Output:
            return OneShotScheduler(func).run(input)

        primitive_func.__qualname__ = f"{name}_primitive"
-        primitive_func = durable(primitive_func)
+        durable_primitive_func = durable(primitive_func)


mypy complained here that the type of primitive_func didn't match after being wrapped by durable.

achille-roussel · 2024-04-25T02:14:54Z

src/dispatch/http.py

@@ -117,7 +117,7 @@ def do_POST(self):
            )
            max_age = timedelta(minutes=5)
            try:
-                verify_request(signed_request, verification_key, max_age)
+                verify_request(signed_request, self.verification_key, max_age)


This was an actual bug.

achille-roussel · 2024-04-25T02:15:18Z

src/dispatch/status.py

@@ -83,7 +83,7 @@ def __str__(self):
 _OUTPUT_TYPES: Dict[Type[Any], Callable[[Any], Status]] = {}


-def status_for_error(error: Exception) -> Status:
+def status_for_error(error: BaseException) -> Status:


Turned out KeyboardInterrupt is not a subclass of Exception.

achille-roussel · 2024-04-25T02:15:48Z

src/dispatch/test/service.py

+        self.roundtrips: OrderedDict[DispatchID, List[RoundTrip]] = OrderedDict()
+        self.collect_roundtrips = collect_roundtrips


This simplifies call sites where we accessed the roundtrips field since it cannot be None anymore.

achille-roussel · 2024-04-25T02:16:30Z

tests/dispatch/signature/test_signature.py

+        assert isinstance(verification_key, Ed25519PublicKey)
+        assert verification_key.public_bytes_raw(), public_key2_bytes


Using assert instead of the methods helped mypy understand that verification_key couldn't be None.

achille-roussel · 2024-04-25T02:16:40Z

tests/dispatch/test_error.py

@@ -65,7 +65,7 @@ def test_conversion_without_traceback(self):
            original_exception = e

        error = Error.from_exception(original_exception)
-        error.traceback = ""
+        error.traceback = b""


This was an actual bug.

achille-roussel · 2024-04-25T02:17:11Z

tests/test_http.py

@@ -72,7 +72,7 @@ def test_content_length_missing(self):
    def test_content_length_too_large(self):
        resp = self.client.post(
            f"{self.endpoint}/dispatch.sdk.v1.FunctionService/Run",
-            data=b"a" * 16_000_001,
+            data={"msg": "a" * 16_000_001},


The post method apparently expected data to be an object.

achille-roussel · 2024-04-25T02:19:05Z

src/dispatch/experimental/durable/function.py

+                for i, (is_null, value) in enumerate(stack if stack is not None else []):
                    if is_null:
                        print(f"stack[{i}] = NULL")
                    else:
                        print(f"stack[{i}] = {value}")
                print(f"BP = {bp}")
-                for i, block in enumerate(blocks):
+                for i, block in enumerate(blocks if blocks is not None else []):


These would have broken in the case where stack and blocks were None.

Looks like mypy isn't smart enough to see that these are only None if frame_state >= FRAME_CLEARED, and that we're in a block where frame_state < FRAME_CLEARED.

This analysis would be pretty difficult to make right? There are no constants in Python, FRAME_CLEARED could be altered concurrently by a thread, potentially in a module that imported this one.

I don't think there are any points between checks where the GIL would be released and another thread allowed to run? There are calls to the C extension in there, but you have to explicitly release the GIL from C. mypy either assumes that all C extension calls may release the GIL, or it just doesn't propagate constraints across blocks.

Assumptions about when the GIL can be released seem like asking for trouble, the rules are a bit unclear as to when this may happen; I would expect mypy not to try to be that smart because it's a lot of efforts for unclear rewards (false positives make the program safer anyways).

We could verify by pulling the value into a local instead of depending on the global FRAME_CLEARED variable.

Oh but even with a local, it doesn't know that frame_state = ext.get_frame_state(g) will return a value less than FRAME_CLEARED right?

(I just tried with a local and it still complains that the values might be None)

stack and blocks are set to None after frame_state is set, and if and only if frame_state < FRAME_CLEARED. The frame_state variable does not change after they're set to None.

I suspect the issue is that mypy forwards constraints into blocks, but doesn't carry them across blocks, because of potential path explosions. It seems to be smart enough to tighten types as you extend into if blocks, but in this case it doesn't carry the frame_state < FRAME_CLEARED constraint forward to an adjacent block.

No big deal, false positives are expected occasionally 🙂

achille-roussel · 2024-04-25T02:19:28Z

Makefile

@@ -30,7 +30,7 @@ fmt-check:
    exit $$((isort_status + black_status))

 typecheck:
-	$(PYTHON) -m mypy src tests
+	$(PYTHON) -m mypy --check-untyped-defs src tests examples


We're doing type checks on the examples directory as well now.

Signed-off-by: Achille Roussel <achille.roussel@gmail.com>

…om-test-package test: drop FastAPI dependency

mypy: upgrade to 1.10.0

0b12997

Signed-off-by: Achille Roussel <achille.roussel@gmail.com>

achille-roussel added the enhancement New feature or request label Apr 25, 2024

achille-roussel requested review from chriso, vadimdemedes and Pryz April 25, 2024 02:10

achille-roussel self-assigned this Apr 25, 2024

achille-roussel added 2 commits April 24, 2024 19:12

fix type checking on Python 3.8

98c195e

Signed-off-by: Achille Roussel <achille.roussel@gmail.com>

exclude only the files that mypy has trouble with

c4487e4

Signed-off-by: Achille Roussel <achille.roussel@gmail.com>

achille-roussel commented Apr 25, 2024

View reviewed changes

Pryz approved these changes Apr 25, 2024

View reviewed changes

achille-roussel added 5 commits April 25, 2024 10:05

fix formatting

3e1fd3e

Signed-off-by: Achille Roussel <achille.roussel@gmail.com>

test: drop FastAPI dependency

d63ae80

Signed-off-by: Achille Roussel <achille.roussel@gmail.com>

fix examples

57a55c8

Signed-off-by: Achille Roussel <achille.roussel@gmail.com>

remove impossible test validated by mypy type checks

831feea

Signed-off-by: Achille Roussel <achille.roussel@gmail.com>

Merge pull request #155 from stealthrocket/drop-fastapi-dependency-fr…

8a92dbf

…om-test-package test: drop FastAPI dependency

achille-roussel merged commit a47ae12 into main Apr 25, 2024
10 checks passed

achille-roussel deleted the mypy-1.10.0 branch April 25, 2024 17:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mypy: upgrade to 1.10.0 #156

mypy: upgrade to 1.10.0 #156

achille-roussel commented Apr 25, 2024

achille-roussel Apr 25, 2024

achille-roussel Apr 25, 2024

achille-roussel Apr 25, 2024

achille-roussel Apr 25, 2024

achille-roussel Apr 25, 2024

achille-roussel Apr 25, 2024

achille-roussel Apr 25, 2024

achille-roussel Apr 25, 2024

chriso Apr 28, 2024

achille-roussel Apr 28, 2024

chriso Apr 28, 2024

achille-roussel Apr 28, 2024

achille-roussel Apr 28, 2024

achille-roussel Apr 28, 2024

chriso Apr 29, 2024

achille-roussel Apr 25, 2024

		self.roundtrips: OrderedDict[DispatchID, List[RoundTrip]] = OrderedDict()
		self.collect_roundtrips = collect_roundtrips

		assert isinstance(verification_key, Ed25519PublicKey)
		assert verification_key.public_bytes_raw(), public_key2_bytes

mypy: upgrade to 1.10.0 #156

mypy: upgrade to 1.10.0 #156

Conversation

achille-roussel commented Apr 25, 2024

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

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