[typing] prefect.concurrency #16441
[typing] prefect.concurrency #16441
Conversation
mjpieters
requested review from
cicdw,
desertaxle and
zzstoatzz
as code owners
github-actions
bot
added
the
development
mjpieters
force-pushed
the
typing_pyright_concurrency
branch
7 times, most recently
from
695d8ff to
32643b2
Compare
mjpieters
force-pushed
the
typing_pyright_concurrency
branch
from
32643b2 to
698e585
Compare
CodSpeed Performance ReportMerging #16441 will not alter performanceComparing Summary
|
mjpieters
force-pushed
the
typing_pyright_concurrency
branch
2 times, most recently
from
853011a to
f99ed16
Compare
There was a problem hiding this comment.
looking great! holding just for that ruff question and to give @desertaxle a chance to peek at that client context change
There was a problem hiding this comment.
just realizing that prefect.concurrency.asyncio -> prefect.concurrency._asyncio etc would be a breaking change from an imports perspective, which I think we'd have to avoid
mjpieters
force-pushed
the
typing_pyright_concurrency
branch
from
f99ed16 to
0003cf9
Compare
Darn. I'll put the public names from that module back then; until the client flow test failed I wasn't even aware there were other import locations outside of the concurrency package. |
mjpieters
force-pushed
the
typing_pyright_concurrency
branch
from
0003cf9 to
d88e240
Compare
All the public |
mjpieters
force-pushed
the
typing_pyright_concurrency
branch
6 times, most recently
from
128fe58 to
792e706
Compare
mjpieters
force-pushed
the
typing_pyright_concurrency
branch
23 times, most recently
from
a204c1c to
ace3092
Compare
| from ._asyncio import ( | ||
| AcquireConcurrencySlotTimeoutError as AcquireConcurrencySlotTimeoutError, | ||
| ) | ||
| from .services import ConcurrencySlotAcquisitionService | ||
|
|
||
|
|
||
| class ConcurrencySlotAcquisitionError(Exception): | ||
| """Raised when an unhandlable occurs while acquiring concurrency slots.""" | ||
|
|
||
|
|
||
| class AcquireConcurrencySlotTimeoutError(TimeoutError): | ||
| """Raised when acquiring a concurrency slot times out.""" | ||
| from ._asyncio import ConcurrencySlotAcquisitionError as ConcurrencySlotAcquisitionError | ||
| from ._asyncio import aacquire_concurrency_slots, arelease_concurrency_slots |
There was a problem hiding this comment.
Could these lines be simplified to something like this?
from ._asyncio import (
AcquireConcurrencySlotTimeoutError,
ConcurrencySlotAcquisitionError,
aacquire_concurrency_slots,
arelease_concurrency_slots,
)There was a problem hiding this comment.
They could, but the 'import name as name' pattern explicitly marks the name as an export again. This is one way you can tell linters and type checkers an imported name is not unused and public.
The other way is to name the imported symbol in __all__, but that's more tedious as you have to name everything.
Without re-exporting these names they would remain private and lingers fish them as inaccessible if you try to import them from this location.
See the Pyright documentation on typed libraries:
Each module exposes a set of symbols. Some of these symbols are considered “private” — implementation details that are not part of the library’s interface. Type checkers like pyright use the following rules to determine which symbols are visible outside of the package.
- [...]
- Imported symbols are considered private by default. If they use the “import A as A” (a redundant module alias), “from X import A as A” (a redundant symbol alias), or “from . import A” forms, symbol “A” is not private unless the name begins with an underscore.
| from prefect.concurrency.v1._asyncio import ( | ||
| acquire_concurrency_slots, | ||
| release_concurrency_slots, | ||
| ) | ||
| from .services import ConcurrencySlotAcquisitionService | ||
|
|
||
|
|
||
| class ConcurrencySlotAcquisitionError(Exception): | ||
| """Raised when an unhandlable occurs while acquiring concurrency slots.""" | ||
|
|
||
| from prefect.concurrency.v1._events import ( | ||
| emit_concurrency_acquisition_events, | ||
| emit_concurrency_release_events, | ||
| ) | ||
| from prefect.concurrency.v1.context import ConcurrencyContext | ||
|
|
||
| class AcquireConcurrencySlotTimeoutError(TimeoutError): | ||
| """Raised when acquiring a concurrency slot times out.""" | ||
| from ._asyncio import ( | ||
| AcquireConcurrencySlotTimeoutError as AcquireConcurrencySlotTimeoutError, | ||
| ) | ||
| from ._asyncio import ConcurrencySlotAcquisitionError as ConcurrencySlotAcquisitionError |
There was a problem hiding this comment.
Could you use relative imports here to match prefect.concurrency.asyncio?
from ._asyncio import (
AcquireConcurrencySlotTimeoutError,
ConcurrencySlotAcquisitionError,
acquire_concurrency_slots,
release_concurrency_slots,
)
from ._events import (
emit_concurrency_acquisition_events,
emit_concurrency_release_events,
)
from prefect.concurrency.v1.context import ConcurrencyContextThere was a problem hiding this comment.
See above: you could but then the name is no longer exported.
mjpieters
force-pushed
the
typing_pyright_concurrency
branch
from
ace3092 to
ed8177e
Compare
References #16292