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

Migration from v2.2 to v3 breaks mypy typing of my_flow.deploy() #15275

Closed
benjamincerigo opened this issue Sep 8, 2024 · 4 comments · Fixed by #15327
Closed

Migration from v2.2 to v3 breaks mypy typing of my_flow.deploy() #15275

benjamincerigo opened this issue Sep 8, 2024 · 4 comments · Fixed by #15327
Labels
bug Something isn't working

Comments

@benjamincerigo
Copy link
Contributor

Bug summary

After migrating from v2.2 to v3 there is an error for mypy due to the typing of the return of my_flow.deploy(...).

It would seem that the mypy has changed how it types the return value of some_flow.deploy(...) between prefect v2.2 and v3. In v2.2 it is being typed as Coroutine and in v3 is being typed as UUID.

Here is the MR for an open-source project that uses prefect that makes the version update.

Error out CI: "Incompatible return value type (got "list[UUID]", expected "list[Coroutine[Any, Any, Any]]") [return-value]"

In the code base, we are running the deployment of flows in parallel. This is done by collecting the returns of some_flow.deploy() and then using them in await asyncio.gather(deployment_coroutines). An example of the some_flow.deploy(...) and the example of asyncio.gather(deployment_coroutines).

My proposed solution to it is here. Could you recommend if this is the best option?

I also think that this is a breaking change and should be documented.

Version info (prefect version output)

Version:             3.0.0
API version:         0.8.4
Python version:      3.11.8
Git commit:          c40d069d
Built:               Tue, Sep 3, 2024 11:13 AM
OS/Arch:             linux/x86_64
Profile:             default
Server type:         cloud
Pydantic version:    2.9.0

Additional context

No response

@benjamincerigo benjamincerigo added the bug Something isn't working label Sep 8, 2024
@desertaxle
Copy link
Member

Thanks for the bug report, @benjamincerigo! Also, huge thanks for sharing your MR; it was very helpful in understanding the issue!

It looks like you're getting bit by some of the oddities of the @sync_compatible decorator. We use @sync_compatible to write async code that will automatically dispatch to sync in the right context. I think @sync_compatible isn't behaving as expected in your case because you're calling .deploy from a sync function (so .deploy should be sync), but your sync function is being called from an async function, so @sync_compatible dispatches to async.

Issues like these are why we plan to adjust our approach to sync and async utilities. You can read more and comment on the proposed approach in #15008.

Back to your setup, I can suggest one adjustment that might help. You can use the deploy utility to deploy multiple flows in one call, which might work well in your create_all_deployments function. Instead of calling .deploy on your flows, you would call .to_deployment and pass the returned deployment configuration from all those calls into deploy. Here's a link to the example in the docs. The advantage of this approach would be that you don't need to manage coroutines for simultaneous deployment, so your create_all_deployments could be sync instead of async. Let me know what you think!

@benjamincerigo
Copy link
Contributor Author

@desertaxle thanks for the quick response and great suggestion.

I will definitely refactor the implementation to use .to_deployment as described here.

To clarify from my experiments mypy's typing for prefect it would seem that all functions in prefect that use sync_compatible have changed from always typing as async return types, in v2.2, to sync return types, in v3. Might it be an idea to update the docs of sync_compatible function, here so that it indicates this behaviour. Unless this is not correct.

@desertaxle
Copy link
Member

Yeah, typing on @sync_compatible has always been lacking because its behavior is runtime-dependent. It was updated in #14226 to default to sync typing since that was determined to be the more common use case. Challenges like this are a motivating factor for moving towards explicit sync and async interfaces.

I like your idea of adding a note to the docstring for sync_compatible. Would you be willing to submit a PR adding that note?

benjamincerigo added a commit to benjamincerigo/prefect that referenced this issue Sep 11, 2024
@benjamincerigo
Copy link
Contributor Author

benjamincerigo commented Sep 11, 2024

I would vote for explicit sync and async interfaces. In my opinion, transparency around async code makes this easier.

MR is open :) but it does seem to be failing for reasons that seem unrelated to the changes I made :(.

benjamincerigo added a commit to benjamincerigo/prefect that referenced this issue Sep 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants