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.concurrency #16441

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mjpieters
Copy link
Contributor

References #16292

@github-actions github-actions bot added the development Tech debt, refactors, CI, tests, and other related work. label Dec 18, 2024
@mjpieters mjpieters force-pushed the typing_pyright_concurrency branch 7 times, most recently from 695d8ff to 32643b2 Compare December 18, 2024 20:43
@mjpieters mjpieters marked this pull request as draft December 18, 2024 20:44
@mjpieters mjpieters force-pushed the typing_pyright_concurrency branch from 32643b2 to 698e585 Compare December 18, 2024 20:48
Copy link

codspeed-hq bot commented Dec 18, 2024

CodSpeed Performance Report

Merging #16441 will not alter performance

Comparing mjpieters:typing_pyright_concurrency (7b725d7) with main (b11b56f)

Summary

✅ 3 untouched benchmarks

@mjpieters mjpieters force-pushed the typing_pyright_concurrency branch 2 times, most recently from 853011a to f99ed16 Compare December 18, 2024 21:17
@mjpieters mjpieters marked this pull request as ready for review December 18, 2024 21:25
Copy link
Collaborator

@zzstoatzz zzstoatzz left a comment

Choose a reason for hiding this comment

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

looking great! holding just for that ruff question and to give @desertaxle a chance to peek at that client context change

src/prefect/concurrency/.ruff.toml Outdated Show resolved Hide resolved
src/prefect/context.py Show resolved Hide resolved
Copy link
Collaborator

@zzstoatzz zzstoatzz left a comment

Choose a reason for hiding this comment

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

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 mjpieters force-pushed the typing_pyright_concurrency branch from f99ed16 to 0003cf9 Compare December 18, 2024 21:53
@mjpieters
Copy link
Contributor Author

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

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 mjpieters force-pushed the typing_pyright_concurrency branch from 0003cf9 to d88e240 Compare December 18, 2024 22:30
@mjpieters
Copy link
Contributor Author

mjpieters commented Dec 18, 2024

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

All the public prefect.concurrency.asyncio and prefect.concurrency.v1.asyncio names are firmly back in place, mea culpa!

@mjpieters mjpieters requested a review from zzstoatzz December 18, 2024 22:32
@mjpieters mjpieters force-pushed the typing_pyright_concurrency branch 6 times, most recently from 128fe58 to 792e706 Compare December 19, 2024 00:25
@mjpieters mjpieters force-pushed the typing_pyright_concurrency branch 23 times, most recently from a204c1c to ace3092 Compare December 24, 2024 17:37
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.

I have two small comments on imports, but otherwise this looks great!

Comment on lines +8 to +12
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
Copy link
Member

Choose a reason for hiding this comment

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

Could these lines be simplified to something like this?

from ._asyncio import (
    AcquireConcurrencySlotTimeoutError,
    ConcurrencySlotAcquisitionError,
    aacquire_concurrency_slots,
    arelease_concurrency_slots,
)

Copy link
Contributor Author

@mjpieters mjpieters Dec 25, 2024

Choose a reason for hiding this comment

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

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.

Comment on lines +9 to +22
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
Copy link
Member

Choose a reason for hiding this comment

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

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 ConcurrencyContext

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See above: you could but then the name is no longer exported.

@mjpieters mjpieters force-pushed the typing_pyright_concurrency branch from ace3092 to ed8177e Compare December 25, 2024 02:31
@mjpieters mjpieters force-pushed the typing_pyright_concurrency branch from ed8177e to 7b725d7 Compare December 25, 2024 09:50
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