Add types to modal.functions #1328

savarin · 2024-02-10T09:28:45Z

Describe your changes

This PR adds mypy types to functions.py, mypy --follow-imports=skip modal/functions.py will now pass.

modal-pr-review-automation

Auto-approved 👍. This diff qualified for automatic approval and doesn't need follow up review.

savarin · 2024-02-10T09:29:57Z

modal/functions.py

@@ -1386,7 +1387,7 @@ async def get_current_stats(self) -> FunctionStats:
 Function = synchronize_api(_Function)


-class _FunctionCall(_Object, type_prefix="fc"):
+class _FunctionCall(_Object, type_prefix="fc"):  # type: ignore[call-arg]


I'm less familiar with type_prefix, but on a grep looks like it adds a two char prefix to objects.

@freider I've removed this. I was getting mypy error previously but might also have been when I was doing mypy checks before running inv etc.

savarin · 2024-02-10T09:31:48Z

Hi there! I add types to open source projects for funsies, recently for instructor. If this is helpful, I can create an issue, start from upstream nodes and work my way downstream, in addition to including CI checks for the files changed.

ekzhang

Hi @savarin — we already have type annotations in our client library, and we run type-checking in CI with mypy .

This is passing already so we're curious how you're thinking about going through this or any best practices you were following to edit these types?

ekzhang · 2024-02-12T16:39:30Z

modal/functions.py

-_current_input_id = ContextVar("_current_input_id")
-_current_function_call_id = ContextVar("_current_function_call_id")
+_current_input_id: ContextVar = ContextVar("_current_input_id")
+_current_function_call_id: ContextVar = ContextVar("_current_function_call_id")


This type annotation conflicts with our auto-generated inv type-stubs machinery, which generates type stubs for the client library. That's it was omitted. Not sure what the difference between omitting and having it is here?

I'm also less clear why this is raised; I usually see mypy requiring a declaration over compound data structures like lists and dictionaries (say needing List[bool] and Dict[str, int] vs List and Dict). Let me try getting inv mypy to pass locally and see what else I find.

An explanation as for why this makes a difference:

We use synchronicity's type-stub generation to emit .pyi-files for these files in order to get type info for the runtime-generated blocking/async interfaces of Function and not just the internal _Function type.

To do that, synchronicity reads annotations, conditionally transforms them, and then re-emits them in the output .pyi-files. In the case of module globals they are only emitted in the .pyi-file if they are explicitly annotated in the original file, so without this annotation here these variables aren't even included in the .pyi-files

Got it! My understanding here is to incorporate these changes, let me know otherwise.

That being said, I do see these in my .pyi file (after running inv protoc and inv type-stubs:

_current_input_id: _contextvars.ContextVar _current_function_call_id: _contextvars.ContextVar

Those should be fine

ekzhang · 2024-02-12T16:40:18Z

modal/functions.py

@@ -924,7 +925,7 @@ def from_parametrized(
        obj,
        from_other_workspace: bool,
        options: Optional[api_pb2.FunctionOptions],
-        args: Iterable[Any],
+        args: Sized,


I haven't heard of this before, what is the difference between Iterable[Any] -> Sized?

Iterable supports any object that can be iterated over (implements __iter__) vs Sized supports any object that has a length (implements __len__). Generator can be iterated over but will fail this line here.

Simple example:

def custom_print(lines: Iterable[Any]) -> None: for line in lines: print(line) # OK def custom_len(lines: Iterable[Any]) -> None: print(len(lines)) # run.py:10: error: Argument 1 to "len" has incompatible type "Iterable[Any]"; expected "Sized" [arg-type]

Fix:

def custom_len(lines: Sized) -> None: print(len(lines)) # OK

savarin · 2024-02-12T18:36:51Z

Hi @savarin — we already have type annotations in our client library, and we run type-checking in CI with mypy .

This is passing already so we're curious how you're thinking about going through this or any best practices you were following to edit these types?

@ekzhang Got it. Let me try getting the mypy checks in CI to pass locally, using this successful Github Actions run as example.

In the meantime, I can share that running locally mypy --follow-imports=skip on modal.functions gets me these errors (setting --python-version=3.8 or --python-version=3.10 raises about the same). Running mypy . gets this.

The errors raised appear directionally correct, for example the secrets argument being passed to FunctionEnv here should be a Sequence (matching definition in FunctionEnv) and not a Collection (as per definition in Function). I'll address the Sized comment below.

savarin · 2024-02-12T22:42:08Z

@ekzhang Got it. Let me try getting the mypy checks in CI to pass locally, using this successful Github Actions run as example.

In the meantime, I can share that running locally mypy --follow-imports=skip on modal.functions gets me these errors (setting --python-version=3.8 or --python-version=3.10 raises about the same). Running mypy . gets this.

The errors raised appear directionally correct, for example the secrets argument being passed to FunctionEnv here should be a Sequence (matching definition in FunctionEnv) and not a Collection (as per definition in Function). I'll address the Sized comment below.

@ekzhang OK cool got inv mypy to pass locally.

I haven't done much type checking against .pyi stub files, but my understanding is that this checks that the function signatures in the .pyi files are being used correctly. In other words, it's a check against the interfaces rather than the implementation itself.

For example, in the functions.pyi file I see the following. This will raise an error if FunctionEnv is used downstream with an incorrect type for the argument mounts.

class FunctionEnv:
    ...

    def __init__(self, image: typing.Union[modal.image._Image, None], mounts: typing.Sequence[modal.mount._Mount], ...) -> None:
        ...

However suppose a modal.mount._Mount object is appended to FunctionEnv.mounts, this should raise a type error because Sequence is read-only, which will be raised under a standard mypy check but not for the check against .pyi files.

class FunctionEnv:
    ...

    def __init__(self, image: typing.Union[modal.image._Image, None], mounts: typing.Sequence[modal.mount._Mount], ...) -> None:
        mounts.append(modal.mount._Mount(...))

Now whether checking .pyi is sufficient (enforcing type constraints only with mypy modal/functions.pyi) vs checking the implementation (enforcing behavioral constraints like mutability with mypy modal/functions.py), that's the team's call.

I just wanted to raise my hand to offer to help out checking that implementation is typed correctly, should this be desired. What I've done in the past is add the types file-by-file, and ensure CI only runs on the files that have been typed.

ekzhang · 2024-02-13T02:08:48Z

First, thanks so much for contributing here!

Next, with this in mind we're looking into the differences in type annotations between the .pyi and .py files. There definitely seems to be some slightly inconsistent type annotations that we haven't thoroughly audited yet. :)

savarin · 2024-02-13T06:24:53Z

First, thanks so much for contributing here!

Next, with this in mind we're looking into the differences in type annotations between the .pyi and .py files. There definitely seems to be some slightly inconsistent type annotations that we haven't thoroughly audited yet. :)

OK I see this now - there's a bit more 'magic' here that I expected (with stubs codegen from protos etc), very cool though. I was curious what the difference was between contextvars.ContextVar and _contextvars.ContextVar, ChatGPT had this to say:

This is because the stub generator recognizes ContextVar as coming from the _contextvars module at a lower implementation level. Python's contextvars module is a high-level interface that may use _contextvars internally, and the stub generator opts to reference this more specific implementation detail in the generated stub file.

However, this should not affect the usability or the correctness of your stubs. From a type-checking perspective, _contextvars.ContextVar and contextvars.ContextVar are considered equivalent, assuming the type checker properly respects the relationship between these modules. The stub files are primarily for type checkers and tools that analyze your code rather than for direct use in your Python runtime.

Sounds good, will keep an eye out for changes!

freider · 2024-02-13T08:50:57Z

modal/functions.py

@@ -1260,7 +1263,7 @@ async def remote_gen(self, *args, **kwargs) -> AsyncGenerator[Any, None]:
        async for item in self._call_generator(args, kwargs):  # type: ignore
            yield item

-    def call(self, *args, **kwargs) -> Awaitable[Any]:
+    def call(self, *args, **kwargs) -> Awaitable[Any]:  # type: ignore[return]


Why was this added? It also won't get transferred to the generated type stubs in the pyi-file so it shouldn't make a difference

mypy sees this function as returning None. This doesn't raise error when we type check against stubs but does so when we type check the implementation (which this PR is suggesting we do).

To avoid confusion I'll move this PR back to draft (though also happy to close it).

Ah, I see now that it's the deprecated/non-function call method you're changing. We might as well just remove the return type (or change it to None). No need to close the PR, I think it's valuable and we'll merge it as soon as we've ironed out the questionmarks :)

freider · 2024-02-14T13:27:25Z

I was quite surprised that mypy appears to entirely skip the implementation checks since we don't directly refer to our .py-files and when it does directory traversal it will prefer to use the pyi-files. Realizing there are quite a few places we need to fix in our implemenation to get rid of incorrect types or similar, so thanks for bringing attention to that!

I'd be happy to merge this if we get rid of the # type: ignore additions which I don't think are needed? (I seem to get no mypy warnings for the functions.py when removing them and the incorrect return type on call)

savarin · 2024-02-14T20:24:49Z

I was quite surprised that mypy appears to entirely skip the implementation checks since we don't directly refer to our .py-files and when it does directory traversal it will prefer to use the pyi-files. Realizing there are quite a few places we need to fix in our implemenation to get rid of incorrect types or similar, so thanks for bringing attention to that!

I'd be happy to merge this if we get rid of the # type: ignore additions which I don't think are needed? (I seem to get no mypy warnings for the functions.py when removing them and the incorrect return type on call)

@freider Glad to hear this is helpful! I've since removed the type ignores, agreed no longer seeing errors now.

I've seen codebases running .pyi checks or running .py checks but I don't believe I've seen both - hence my thought of putting this PR on hold pending discussion. Happy to merge this as is, one idea is to add in a follow-up PR an additional mypy check on an allowlist of files that succeed .py type checks (example here).

savarin · 2024-02-14T23:08:58Z

@freider I added this additional .py mypy check. What's a tad strange is I now need to add this line.

freider · 2024-02-16T19:08:05Z

@freider I added this additional .py mypy check. What's a tad strange is I now need to add this line.

I think that's unrelated to your change - I'm getting that warning too when I run mypy locally on main. It's due to the mypy version of the repo being pinned to version ~1.4, and github actions caching has probably cached an older version than 1.8 that I have locally which triggers this warning. Should upgrade mypy in the requirements file too, but I can do that separately

savarin · 2024-02-19T06:08:48Z

@freider I'm afraid I can't merge (I don't have write access), let me know if there's anything else I can do to be helpful. Thanks!

freider · 2024-02-19T09:30:36Z

Thanks for the contribution, it's merged now!

savarin · 2024-02-19T20:40:53Z

Thanks for the contribution, it's merged now!

@freider Would it be helpful if I did similar changes to other files?

freider · 2024-02-20T07:14:18Z

@savarin Definitely, much appreciated! :)

savarin · 2024-02-20T20:20:52Z

@freider Sounds good, I created the issue #1372 and will start from the top of the dependency tree.

modal-pr-review-automation bot approved these changes Feb 10, 2024

View reviewed changes

savarin commented Feb 10, 2024

View reviewed changes

savarin marked this pull request as ready for review February 10, 2024 09:33

ekzhang reviewed Feb 12, 2024

View reviewed changes

savarin marked this pull request as draft February 12, 2024 18:43

savarin requested a review from ekzhang February 12, 2024 20:00

savarin marked this pull request as ready for review February 12, 2024 22:42

savarin force-pushed the include-types-1 branch from 16ba69a to 16ed523 Compare February 12, 2024 22:44

freider reviewed Feb 13, 2024

View reviewed changes

savarin marked this pull request as draft February 13, 2024 16:13

savarin added 3 commits February 14, 2024 12:24

add types to functions.py

7acb4c7

add asserts for hidden attributes

1cd52d3

remove type ignore

3c855c7

savarin force-pushed the include-types-1 branch from 5c29784 to 3c855c7 Compare February 14, 2024 20:25

savarin marked this pull request as ready for review February 14, 2024 20:25

savarin requested a review from freider February 14, 2024 20:25

add .py mypy check

eb79049

freider approved these changes Feb 16, 2024

View reviewed changes

Merge branch 'main' into include-types-1

4a90e86

Merge branch 'main' into include-types-1

bace082

freider merged commit 2cb7fae into modal-labs:main Feb 19, 2024
3 checks passed

savarin changed the title ~~Add types to functions.py~~ Add types to modal.functions Feb 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add types to modal.functions #1328

Add types to modal.functions #1328

savarin commented Feb 10, 2024

modal-pr-review-automation bot left a comment

savarin Feb 10, 2024

savarin Feb 14, 2024

savarin commented Feb 10, 2024 •

edited

Loading

ekzhang left a comment

ekzhang Feb 12, 2024

savarin Feb 12, 2024

freider Feb 13, 2024

savarin Feb 14, 2024

freider Feb 16, 2024

ekzhang Feb 12, 2024

savarin Feb 12, 2024 •

edited

Loading

savarin commented Feb 12, 2024 •

edited

Loading

savarin commented Feb 12, 2024

ekzhang commented Feb 13, 2024

savarin commented Feb 13, 2024

freider Feb 13, 2024

savarin Feb 13, 2024 •

edited

Loading

freider Feb 14, 2024

freider commented Feb 14, 2024

savarin commented Feb 14, 2024 •

edited

Loading

savarin commented Feb 14, 2024

freider commented Feb 16, 2024

savarin commented Feb 19, 2024

freider commented Feb 19, 2024

savarin commented Feb 19, 2024

freider commented Feb 20, 2024

savarin commented Feb 20, 2024

Add types to modal.functions #1328

Add types to modal.functions #1328

Conversation

savarin commented Feb 10, 2024

Describe your changes

modal-pr-review-automation bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

savarin commented Feb 10, 2024 • edited Loading

ekzhang 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

savarin Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

savarin commented Feb 12, 2024 • edited Loading

savarin commented Feb 12, 2024

ekzhang commented Feb 13, 2024

savarin commented Feb 13, 2024

Choose a reason for hiding this comment

savarin Feb 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freider commented Feb 14, 2024

savarin commented Feb 14, 2024 • edited Loading

savarin commented Feb 14, 2024

freider commented Feb 16, 2024

savarin commented Feb 19, 2024

freider commented Feb 19, 2024

savarin commented Feb 19, 2024

freider commented Feb 20, 2024

savarin commented Feb 20, 2024

savarin commented Feb 10, 2024 •

edited

Loading

savarin Feb 12, 2024 •

edited

Loading

savarin commented Feb 12, 2024 •

edited

Loading

savarin Feb 13, 2024 •

edited

Loading

savarin commented Feb 14, 2024 •

edited

Loading