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

[typing] prefect.client #16265

Merged
merged 1 commit into from
Dec 8, 2024

Conversation

mjpieters
Copy link
Contributor

Code now passes pyright checking in strict mode.

@mjpieters mjpieters force-pushed the prefect_client_annotations branch 2 times, most recently from 622ebb8 to e5e608c Compare December 7, 2024 18:07
Copy link

codspeed-hq bot commented Dec 7, 2024

CodSpeed Performance Report

Merging #16265 will not alter performance

Comparing mjpieters:prefect_client_annotations (67d1b4f) with main (1c30778)

Summary

✅ 3 untouched benchmarks

@mjpieters mjpieters force-pushed the prefect_client_annotations branch from e5e608c to b602080 Compare December 7, 2024 18:40
@mjpieters mjpieters changed the title [typing]: prefect.client [typing] prefect.client Dec 7, 2024
@mjpieters mjpieters force-pushed the prefect_client_annotations branch from b602080 to aa13337 Compare December 7, 2024 18:43
@mjpieters mjpieters marked this pull request as draft December 7, 2024 18:59
src/prefect/main.py Outdated Show resolved Hide resolved
@mjpieters mjpieters force-pushed the prefect_client_annotations branch 3 times, most recently from fb5e9fa to 4050ee8 Compare December 7, 2024 21:35
@mjpieters mjpieters marked this pull request as ready for review December 7, 2024 21:40
@mjpieters mjpieters force-pushed the prefect_client_annotations branch from 4050ee8 to 735ecb1 Compare December 7, 2024 21:48
Code now passes pyright checking in strict mode.
@mjpieters mjpieters force-pushed the prefect_client_annotations branch from 735ecb1 to 67d1b4f Compare December 7, 2024 21:50
@mjpieters mjpieters requested a review from desertaxle December 8, 2024 00:08
Copy link
Member

@desertaxle desertaxle left a comment

Choose a reason for hiding this comment

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

Thanks @mjpieters!

@desertaxle desertaxle added the development Tech debt, refactors, CI, tests, and other related work. label Dec 8, 2024
Comment on lines +21 to +27
def _current_async_client(
client: Union["PrefectClient", "SyncPrefectClient"],
) -> TypeIs["PrefectClient"]:
from prefect._internal.concurrency.event_loop import get_running_loop

# Only a PrefectClient will have a _loop attribute that is the current loop
return getattr(client, "_loop", None) == get_running_loop()
Copy link
Collaborator

Choose a reason for hiding this comment

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

commenting for visibility

Copy link
Contributor Author

@mjpieters mjpieters Dec 8, 2024

Choose a reason for hiding this comment

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

I may have used the wrong special form here. This type narrowing function returns a TypeIs construct but now that I reread this it should have been a TypeGuard instead.

That's because this function returns False in two different cases:

  1. the client is a SynPrefectClient instance.
  2. the client is a PrefectClient instance, attached to a different loop.

The TypeIs construct acts like an isinstance() check, where the type checker can infer that if False is returned then the client must be a SyncPrefectClient as that's the only type left out of the input Union. But that's not correct for case 2.

A TypeGuard function explicitly doesn't make promises about types when False is returned and so fits this function much better.

It is a technically moot point in how this function is used in this module but an important distinction if you were to use this as an example 🤦.

TLDR:

  • use TypeIs[R] when True means this is of type R and False means this is not of type R
  • use TypeGuard[R] when True means this meets conditions that tell me this is of type R but False means uh, I am not sure what this is and I don't care 🤷.

@zzstoatzz
Copy link
Collaborator

thank you @mjpieters !

@desertaxle desertaxle merged commit 24c7f08 into PrefectHQ:main Dec 8, 2024
40 checks passed
@mjpieters mjpieters deleted the prefect_client_annotations branch December 8, 2024 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development Tech debt, refactors, CI, tests, and other related work.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants