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

Implement PEP 685 extra normalization in resolver #12002

Merged
merged 10 commits into from
Sep 13, 2023

Conversation

uranusjr
Copy link
Member

All extras from user input or dependant package metadata are properly normalized for comparison and resolution. This ensures requests for extras from a dependant can always correctly find the normalized extra in the dependency, even if the requested extra name is not normalized.

Note that this still relies on the declaration of extra names in the dependency's package metadata to be properly normalized when the package is built, since correct comparison between an extra name's normalized and non-normalized forms requires change to the metadata parsing logic, which is only available in packaging 22.0 and up, which pip does not use at the moment.

Close #11649 (but not #11445!)

All extras from user input or dependant package metadata are properly
normalized for comparison and resolution. This ensures requests for
extras from a dependant can always correctly find the normalized extra
in the dependency, even if the requested extra name is not normalized.

Note that this still relies on the declaration of extra names in the
dependency's package metadata to be properly normalized when the package
is built, since correct comparison between an extra name's normalized
and non-normalized forms requires change to the metadata parsing logic,
which is only available in packaging 22.0 and up, which pip does not use
at the moment.
@pradyunsg
Copy link
Member

Yea, this fails to handle packages that don't pull in PEP 685 style extra names. :/

@uranusjr
Copy link
Member Author

uranusjr commented May 8, 2023

Note to self it’d be a good idea to also provide a better error message for this.

@uranusjr
Copy link
Member Author

Alright, addressed the test failure.

In fact I re-implemented this almost entirely, so now the resolver actually keeps track of non-normalized (requested) extras alongside with the normalized form. The normalized form is used in the resolver, except when a candidate inspects distribution metadata to look for dependencies—both the normalized and non-normalized forms are used for look up.

This means that if a package is built without following PEP 685, the user (and the package’s dependants) can still access the extra using the non-normalized form. And once we upgrade to newer packaging with proper PEP 685 support, the non-normalized extras would simply return some duplicated dependencies that are de-duped anyway.

The only failing scenario now would be if a distribution is built without normalizing extras, and is requested with a normalized extra. This would not have worked prior to PEP 685 anyway, and will continue to fail until we upgrade packaging.

When an unnormalized extra is requested, try to look up dependencies
with both its raw and normalized forms, to maintain compatibility when
an extra is both specified and requested in a non-standard form.
@uranusjr uranusjr force-pushed the extra-normalization branch from 76335ad to d64190c Compare May 11, 2023 07:10
@uranusjr uranusjr requested a review from pradyunsg May 11, 2023 07:10
Since this function now always take normalized names, additional
normalization is no longer needed.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@pypa-bot
Copy link

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will eligible for code review and hopefully merging!

@pypa-bot pypa-bot added needs rebase or merge PR has conflicts with current master and removed needs rebase or merge PR has conflicts with current master labels Sep 12, 2023
@uranusjr
Copy link
Member Author

Gosh this setuptools thing is so annoying. I think I’ll try to manually build a package instead 😐

@pradyunsg
Copy link
Member

Gosh this setuptools thing is so annoying. I think I’ll try to manually build a package instead 😐

Should be feasible with create_basic_wheel_for_package and foo; extra == 'bar' style dependency declarations.

This removes extra normalization when metadata is loaded into the data
structures, so we can obtain the raw values later in the process during
resolution.

The change in match_markers is needed because this is relied on by the
legacy resolver. Since we removed eager normalization, we need to do
that when the extras are used instead to maintain compatibility.
@uranusjr uranusjr force-pushed the extra-normalization branch 2 times, most recently from c525928 to 15a3ba5 Compare September 13, 2023 08:45
The importlib.metadata and pkg_resources backends unfortunately
normalize extras differently, and we don't really want to continue using
the latter's logic (being partially lossy while still not compliant to
standards), so we add a new abstraction for the purpose.
@RonnyPfannschmidt
Copy link
Contributor

i have a side channel question for this one wrt implementing certain helpers on top of this for normalizing extras of specific versions to a canonical extra (to support default extras)

how hard would it be to expand the resolver to turn extras from a intermediate for to something like "canonical" extras when the solver picks a specific version

(in my wip draft for the pep intend to turn extras like mypkg[foo, -bar] into a canonical form like mypkg[-, foo, ])

if its not unreasonably hard to implement/put on top, i'd go forward with making my draft publishable

@uranusjr
Copy link
Member Author

Shouldn’t be too hard. The point of this is actually to delay marker normalisation as much as possible, and not at all once packaging implements that directly into marker evaluation. The real problem would actually be in pkg_resources inherited from setuptools, which eagerly normalises stuff and necessitates most of the workaround in here. Setuptools is where you’ll get the most issues with or without this PR.

@uranusjr uranusjr merged commit 0827d76 into pypa:main Sep 13, 2023
24 checks passed
@uranusjr uranusjr deleted the extra-normalization branch September 13, 2023 13:01
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dependency resolution conflicts with hyphen in extras
4 participants