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.

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

modal.functions.gather type-aware via generics #2188

Closed
zsiegel92 opened this issue Sep 4, 2024 · 2 comments · Fixed by #2199
Closed

modal.functions.gather type-aware via generics #2188

zsiegel92 opened this issue Sep 4, 2024 · 2 comments · Fixed by #2199

Comments

@zsiegel92
Copy link
Contributor

zsiegel92 commented Sep 4, 2024

Now that .spawn() is type-aware via this PR, outputs = modal.functions.gather(*[my_function.spawn(job) for job in jobs]) is accepted by type-checkers, and since .spawn returns a generic FunctionCall[R] (where my_function returns R), it should be possible for outputs to be type-checked to the return type of my_function without any additional annotations.

Would have to make the gather function generic.

Note - I manually edit the generated functions.pyi in the Modal package to have this:

class __gather_spec(typing_extensions.Protocol):
    def __call__(self, *function_calls: FunctionCall[R])->typing.Iterable[R]: ...

making my IDE the only IDE that knows the type of gather(*fn_calls), which is nice (the annotation may be a bit wrong idk).

I'm not sure how y'all generate your .pyi files but maybe you could have a build step that edits them to match the true behavior; the modal SDK annotations seem to be overly pessimistic/broad.

@freider
Copy link
Contributor

freider commented Sep 6, 2024

Yeah I think that would be a nice improvement and I think it should work, I can make a quick patch to see!

FYI the .pyi files are generated by calling inv type-stubs in the client repo (requires requirements.dev.txt to be installed).

@zsiegel92
Copy link
Contributor Author

Thanks @freider for moving this along! Big quality of life improvement for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants