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

Add types to modal.functions #1328

Merged
merged 6 commits into from
Feb 19, 2024
+16 −7
Merged

Add types to modal.functions #1328

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7acb4c7
add types to functions.py
savarin Feb 10, 2024
1cd52d3
add asserts for hidden attributes
savarin Feb 13, 2024
3c855c7
remove type ignore
savarin Feb 14, 2024
eb79049
add .py mypy check
savarin Feb 14, 2024
4a90e86
Merge branch 'main' into include-types-1
freider Feb 16, 2024
bace082
Merge branch 'main' into include-types-1
freider Feb 19, 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
1 change: 1 addition & 0 deletions modal/cli/profile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ async def list(json: Optional[bool] = False):
# Catch-all for other exceptions, like incorrect server url
workspace = "Unknown (profile misconfigured)"
else:
assert hasattr(resp, "username")
workspace = resp.username
content = ["•" if active else "", profile, workspace]
rows.append((active, content))
Expand Down
17 changes: 10 additions & 7 deletions modal/functions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
Optional,
Sequence,
Set,
Sized,
Type,
Union,
)
Expand Down Expand Up @@ -82,7 +83,7 @@
from .secret import _Secret
from .volume import _Volume

OUTPUTS_TIMEOUT = 55 # seconds
OUTPUTS_TIMEOUT = 55.0 # seconds
ATTEMPT_TIMEOUT_GRACE_PERIOD = 5 # seconds


Expand Down Expand Up @@ -563,13 +564,13 @@ def from_args(
stub,
image=None,
secret: Optional[_Secret] = None,
secrets: Collection[_Secret] = (),
secrets: Sequence[_Secret] = (),
schedule: Optional[Schedule] = None,
is_generator=False,
gpu: GPU_T = None,
# TODO: maybe break this out into a separate decorator for notebooks.
mounts: Collection[_Mount] = (),
network_file_systems: Dict[Union[str, os.PathLike], _NetworkFileSystem] = {},
network_file_systems: Dict[Union[str, PurePosixPath], _NetworkFileSystem] = {},
allow_cross_region_volumes: bool = False,
volumes: Dict[Union[str, os.PathLike], Union[_Volume, _S3Mount]] = {},
webhook_config: Optional[api_pb2.WebhookConfig] = None,
Expand Down Expand Up @@ -926,7 +927,7 @@ def from_parametrized(
obj,
from_other_workspace: bool,
options: Optional[api_pb2.FunctionOptions],
args: Iterable[Any],
args: Sized,
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@savarin savarin Feb 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

kwargs: Dict[str, Any],
) -> "_Function":
async def _load(provider: _Function, resolver: Resolver, existing_object_id: Optional[str]):
Expand Down Expand Up @@ -1017,6 +1018,7 @@ async def lookup(
@property
def tag(self):
"""mdmd:hidden"""
assert hasattr(self, "_tag")
return self._tag

@property
Expand All @@ -1033,6 +1035,7 @@ def env(self) -> FunctionEnv:

def get_build_def(self) -> str:
"""mdmd:hidden"""
assert hasattr(self, "_raw_f") and hasattr(self, "_build_args")
return f"{inspect.getsource(self._raw_f)}\n{repr(self._build_args)}"

# Live handle methods
Expand Down Expand Up @@ -1262,7 +1265,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) -> None:
"""Deprecated. Use `f.remote` or `f.remote_gen` instead."""
# TODO: Generics/TypeVars
if self._is_generator:
Expand Down Expand Up @@ -1462,8 +1465,8 @@ async def _gather(*function_calls: _FunctionCall):
gather = synchronize_api(_gather)


_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")
Copy link
Member

Choose a reason for hiding this comment

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

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?

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'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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Those should be fine



def current_input_id() -> Optional[str]:
Expand Down
5 changes: 5 additions & 0 deletions tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,12 @@ def lint(ctx):

@task
def mypy(ctx):
mypy_allowlist = [
"modal/functions.py",
]

ctx.run("mypy .", pty=True)
ctx.run(f"mypy {' '.join(mypy_allowlist)} --follow-imports=skip", pty=True)


@task
Expand Down
Loading