-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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 Block.aload
method and remove @sync_compatible
from Block.load
#16341
Conversation
CodSpeed Performance ReportMerging #16341 will not alter performanceComparing Summary
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
... | ||
|
||
|
||
@overload | ||
def inject_client( | ||
fn: Callable[P, R], | ||
) -> Callable[P, R]: | ||
... | ||
|
||
|
||
def inject_client( | ||
fn: Callable[P, Union[Coroutine[Any, Any, R], R]], | ||
) -> Callable[P, Union[Coroutine[Any, Any, R], R]]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@desertaxle Why did you add this overload?
The implementation uses await
on the callable argument (fn
) so it must produce an awaitable, and nowhere in the prefect codebase is @inject_client
used on a regular function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, you're absolutely right. I was working with @inject_client
while I was making Block.load
sync. This was from something that ultimately didn't work, and I forgot to remove it. I'll make a note to remove this, but feel free to remove it if you're in the neighborhood.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@desertaxle already on it 😁 See #16463.
This PR improves type hinting on
Block.load
by removing the@sync_compatible
decorator and using@async_dispatch
instead.I had to update
@async_dispatch
to handle class methods and update the dispatch behavior in tasks and flows to match the behavior provided by@sync_compatible
.Also,
@inject_client
might be a thorn in our side because it makes wrapped functions async. To handle that in this PR, I removed@inject_client
fromBlock.load
and moved the client retrieval inside the method. There's a weird split where sometimes we can use the sync client, and other times we have to use the async client, but we can clean that up once we remove@async_dispatch
.Related to #16292