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

add arun_deployment #16605

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

add arun_deployment #16605

wants to merge 2 commits into from

Conversation

zzstoatzz
Copy link
Collaborator

@zzstoatzz zzstoatzz commented Jan 3, 2025

removes sync_compatible from run_deployment

Copy link

codspeed-hq bot commented Jan 3, 2025

CodSpeed Performance Report

Merging #16605 will not alter performance

Comparing run-deployment-async (010e4cb) with main (8b8eea2)

Summary

✅ 3 untouched benchmarks

@zzstoatzz zzstoatzz force-pushed the run-deployment-async branch from b7259ed to dc299b1 Compare January 3, 2025 23:53
@zzstoatzz zzstoatzz marked this pull request as ready for review January 4, 2025 00:08
Comment on lines +66 to +67
Callable[..., Coroutine[Any, Any, R]],
"classmethod[type[Any], ..., Coroutine[Any, Any, R]]",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is because the sync and async versions of run_deployment need to take different types of clients. i could make them both Optional[Any] but thought that would be more unclear and this way it pushes folks who care about typing to use the explicit async one

@async_dispatch(arun_deployment)
def run_deployment(
name: Union[str, UUID],
client: Optional["SyncPrefectClient"] = None,
Copy link
Member

Choose a reason for hiding this comment

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

While this is decorated with @async_dispatch, I think we might need to only accept PrefectClient instances. My thinking is that if a SyncPrefectClient is passed in an async context, then arun_deployment will fail.

The transition will be a little messy, but I think we can create a sync client internally if one isn't passed and handle a provided async client with run_coro_as_sync. Once we start warning on async dispatch, we can also start warning on passed async clients so that users know to provide a sync client or use arun_deployment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah that's a great point

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hmm the tricky thing about this is that run_coro_as_sync might try to put it in a new thread, which causes "bound to a different even loop" issues 🤔

Copy link
Collaborator Author

@zzstoatzz zzstoatzz Jan 6, 2025

Choose a reason for hiding this comment

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

in the case where an async client is passed to run deployment in a sync context, im not sure how to call those asynchronous methods without running into the bound to a different event loop

... i could just create a new client (which works) but not using the client that was explicitly passed feels wrong

@zzstoatzz zzstoatzz force-pushed the run-deployment-async branch from dc299b1 to 010e4cb Compare January 6, 2025 18:27
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.

2 participants