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

[MOD-3553]: Function.spawn() returns a FunctionCall for generators & add iter_gen to get results #2155

Merged
merged 6 commits into from
Sep 2, 2024

Conversation

kramstrom
Copy link
Contributor

@kramstrom kramstrom commented Aug 27, 2024

Related to #2145

  • Function.spawn() now always returns a _FunctionCall (no need to check for optional anymore)
  • raises exception when trying to use _FunctionCall.get() for generators
  • adds _FunctionCall.iter_gen() which should be used instead

@kramstrom kramstrom requested a review from freider August 27, 2024 09:29
@kramstrom kramstrom changed the title .spawn() returns a FunctionCall for generators & add iter_gen to get results [WIP] .spawn() returns a FunctionCall for generators & add iter_gen to get results Aug 27, 2024
@kramstrom kramstrom marked this pull request as draft August 27, 2024 11:27
@kramstrom kramstrom changed the title [WIP] .spawn() returns a FunctionCall for generators & add iter_gen to get results Function.spawn() returns a FunctionCall for generators & add iter_gen to get results Aug 27, 2024
@kramstrom kramstrom marked this pull request as ready for review August 27, 2024 11:33
@kramstrom kramstrom force-pushed the kramstrom/add-iter-gen-to-function-call branch from 7d98964 to 2619da9 Compare August 27, 2024 13:03
return await self._invocation().poll_function(timeout=timeout)

async def iter_gen(self) -> AsyncGenerator[Any, None]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should call it get_gen() instead? As a corollary to Function.remote() and Function.remote_gen()

Copy link
Contributor

Choose a reason for hiding this comment

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

Wdyt @aksh-at ? I have a vague memory we discussed adding generator support to FunctionCall like ~2 years ago 😂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep makes sense!

@zsiegel92
Copy link
Contributor

@kramstrom @freider thanks for moving this along!

Would've been extremely fun to have the PR I opened merged into Modal 😂 but glad to see this in-progress and done the right way

I'll close that PR and watch this one 🫡 thanks so much

@kramstrom kramstrom changed the title Function.spawn() returns a FunctionCall for generators & add iter_gen to get results [MOD-3553]: Function.spawn() returns a FunctionCall for generators & add iter_gen to get results Aug 29, 2024
@kramstrom
Copy link
Contributor Author

@kramstrom @freider thanks for moving this along!

Would've been extremely fun to have the PR I opened merged into Modal 😂 but glad to see this in-progress and done the right way

I'll close that PR and watch this one 🫡 thanks so much

Sorry about that! I did rebase from your branch so at least you will be in the commit history though!

@kramstrom kramstrom requested a review from freider September 2, 2024 12:43
Copy link
Contributor

@freider freider left a comment

Choose a reason for hiding this comment

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

lgtm! Not 100% happy that from_id requires is_generator, but it's probably sufficiently rare that we could leave it like that for now (instead of adding new rpcs to be able to fetch the function's generator/function type from server based on id) - in particular since the requesting party needs to know the function type anyways to be able to use it.

@kramstrom kramstrom merged commit bb4cab2 into main Sep 2, 2024
22 checks passed
@kramstrom kramstrom deleted the kramstrom/add-iter-gen-to-function-call branch September 2, 2024 14:10
@zsiegel92
Copy link
Contributor

@kramstrom @freider glad to say that I just ran into this again, did pip install -U modal, and the red underline in my IDE disappeared!

fn_calls: list[modal.functions.FunctionCall[MyType]] = [my_function.spawn(job) for job in jobs]

No type-checker complaints that my_function.spawn(job) might be None! 🙏

Now time to make gather generic so that results = modal.functions.gather(*fn_calls) is type-aware! Will open separate issue - TIA.

@zsiegel92
Copy link
Contributor

@kramstrom #2188

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 this pull request may close these issues.

3 participants