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

Refactor type hints #754

Closed
wants to merge 1 commit into from
Closed

Refactor type hints #754

wants to merge 1 commit into from

Conversation

ofek
Copy link
Sponsor Contributor

@ofek ofek commented Dec 28, 2023

This PR uses from __future__ import annotations to allow for:

  • Lazily computed types (no more strings)
  • Direct subscripting of built-in types e.g. Dict[str, List[str]] -> dict[str, list[str]]
  • Removal of Optional in favor of | None e.g. str | None
  • Removal of Union in favor of | e.g. str | int

Notes:

  • The VersionComparisonMethod type alias in src/packaging/version.py was unused so I removed it
  • All types are now stored in a dedicated file src/packaging/_types.py but type definitions must use the old-style type classes because it is ambiguous that they are type aliases for use in annotations rather than assignments and typing.TypeAlias was introduced only very recently in 3.10
  • After this is merged I plan to do one or two more PRs that will drastically reduce the cost of importing packaging modules

@ofek ofek force-pushed the import-time branch 3 times, most recently from f60d535 to d75c57e Compare December 28, 2023 16:27
@pradyunsg
Copy link
Member

After this is merged I plan to do one or two more PRs that will drastically reduce the cost of importing packaging modules

Could you please mention what these changes / PRs would be doing, and how they'd reduce the import overhead?

@ofek
Copy link
Sponsor Contributor Author

ofek commented Dec 28, 2023

The majority of the improvement will be lazily loading imports such as subprocess and logging, and I may defer regular expression compilation in a second PR however I don't see that being a huge improvement.

@ofek
Copy link
Sponsor Contributor Author

ofek commented Dec 29, 2023

ofek#1

❯ docker run --rm python:3.13-rc bash -c "python -m pip install -qqq 'packaging @ git+https://github.com/pypa/packaging@main' && python -c 'from packaging.requirements import Requirement' && sleep 2 && for i in {1..10}; do python -m timeit -n 1 -r 1 'from packaging.requirements import Requirement'; done"
1 loop, best of 1: 32.3 msec per loop
1 loop, best of 1: 32.1 msec per loop
1 loop, best of 1: 32.2 msec per loop
1 loop, best of 1: 31.5 msec per loop
1 loop, best of 1: 31.3 msec per loop
1 loop, best of 1: 31.5 msec per loop
1 loop, best of 1: 31.5 msec per loop
1 loop, best of 1: 31.4 msec per loop
1 loop, best of 1: 31.3 msec per loop
1 loop, best of 1: 32.1 msec per loop

❯ docker run --rm python:3.13-rc bash -c "python -m pip install -qqq 'packaging @ git+https://github.com/ofek/packaging@import-time-1' && python -c 'from packaging.requirements import Requirement' && sleep 2 && for i in {1..10}; do python -m timeit -n 1 -r 1 'from packaging.requirements import Requirement'; done"
1 loop, best of 1: 16.5 msec per loop
1 loop, best of 1: 16.9 msec per loop
1 loop, best of 1: 17.1 msec per loop
1 loop, best of 1: 16.9 msec per loop
1 loop, best of 1: 17 msec per loop
1 loop, best of 1: 16.8 msec per loop
1 loop, best of 1: 16.9 msec per loop
1 loop, best of 1: 16.9 msec per loop
1 loop, best of 1: 16.9 msec per loop
1 loop, best of 1: 16.9 msec per loop

@ofek
Copy link
Sponsor Contributor Author

ofek commented Dec 29, 2023

This PR is ready for review!

@zanieb
Copy link

zanieb commented Dec 29, 2023

Why are these changes needed for ofek#1?

@ofek
Copy link
Sponsor Contributor Author

ofek commented Dec 29, 2023

I thought I would do a refactor first since I saw a performance benefit to separating type definitions into a separate module. The benefit turned out to be nonexistent because the annotations import has an equivalent cost (it will go away eventually) but I figured this is useful anyway since there is no benefit to defining runtime types.

I think the changes here are useful as the typing people are encouraging the more built-in approaches and heavily discouraging (I can't find where at the moment) the use of Optional and Union.

@ofek
Copy link
Sponsor Contributor Author

ofek commented Dec 29, 2023

# MarkerAtom = Union[MarkerItem, List["MarkerAtom"]]
# MarkerList = List[Union["MarkerList", MarkerAtom, str]]
# mypy does not support recursive type definition
# https://github.com/python/mypy/issues/731
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is valid anymore. The issue is closed in the mypy repo, maybe you could check? WOuld be nice to avoid hauling around the tech debt.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

I thought the same thing and tried but encountered errors.

@pradyunsg
Copy link
Member

I came here to merge this and realised that the codebase has evolved meaningfully since this PR was filed, so filed an updated #785.

@ofek
Copy link
Sponsor Contributor Author

ofek commented Mar 10, 2024

After that is merged I will do the lazy import performance improvement in a separate PR!

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.

None yet

4 participants