Skip yanked releases unless specified #10625

albertosottile · 2021-10-30T09:49:04Z

Currently, pip automatically installs yanked releases if it cannot find a regular release compatible with the platform. While this behavior is in line with PEP 592, it could cause some issues to users that do not expect to have inadvertently installed old versions/bugged versions of the requested package (see #10617 for py2exe).

The change proposed in this PR is to suppress the automatic serving of yanked releases by pip for general requests. If the platform cannot be satisfied by any regular release available, pip should throw an error. Conversely, if the user deliberately requests a yanked release (via == or ===), pip should then serve the requested version.

Fix #10617.

pradyunsg · 2021-10-30T09:52:35Z

Relevant section of the standard: https://www.python.org/dev/peps/pep-0592/#installers

albertosottile · 2021-11-05T18:11:43Z

Is there anything else I have to do on my side to proceed with this PR?

pradyunsg · 2021-11-05T18:57:18Z

A test to verify this works would be great!

This change looks about right, other than that!

uranusjr · 2021-11-06T08:51:27Z

Also, please fix the linter error caught by pre-commit

src/pip/_internal/resolution/resolvelib/factory.py

albertosottile · 2021-11-06T15:31:13Z

I fixed the linter error and added a test for the expected error behavior.

tests/functional/test_install.py

uranusjr

I added a "Fix" keyword to the description so the linked issues can be closed automatically.

uranusjr · 2021-11-06T16:43:32Z

Could you also look into creating a test case for #8262? That needs more kinds of releases so we'll need to use create_basic_wheel_for_xxx functions to set the test up. Some tests in test_new_resolver.py can be used as references.

albertosottile · 2021-11-06T18:01:22Z

Could you also look into creating a test case for #8262?

I am not sure this PR fixes #8262, at least not with the expected behavior they proposed in the issue. I implemented a test with

    <a data-yanked="test reason message" href="../../../packages/simple-1.0.tar.gz">simple-1.0.tar.gz</a>
    <a data-yanked="test reason message" href="../../../packages/simple-2.0.tar.gz">simple-2.0.tar.gz</a>
    <a href="../../../packages/simple-3.0rc0.tar.gz">simple-3.0rc0.tar.gz</a>

and the test ran with pip install simple refuses to install the package

-- stderr: --------------------
ERROR: Could not find a version that satisfies the requirement simple (from versions: 1.0, 2.0, 3.0rc0)
ERROR: No matching distribution found for simple

I think this happens because the prerelease is filtered before reaching the part of code we patched, so we get in the all yanked/no specifier case, but I am not 100% sure.

I think that issue needs a separate fix that explicitly bypasses the need of passing --pre to install prerelease packages, even if everything else is yanked.

I wanted to check what happened with pip install simple===3.0rc0 but I was unable to get a pass test due to

ERROR: Could not install packages due to an OSError: [Errno 2] No such file or directory: '/tmp/pytest-of-root/pytest-0/test_all_yanked_but_pre0/data/packages/simple-3.0rc0.tar.gz'

which I do not understand, given my index configuration.

Side note: you mentioned to use create_basic_wheel_for_xxx, however I do not really understand why this is needed for testing #8262. The user wants to have a prerelease package installed automatically, without passing --pre or a specifier, when all the other previous releases are yanked. I think we need a custom index also to test this, as I do not think create_basic_wheel_for_package can yank a release.

Jackenmen · 2021-11-06T18:43:00Z

I think this happens because the prerelease is filtered before reaching the part of code we patched, so we get in the all yanked/no specifier case, but I am not 100% sure.

This seems to check out with what I've noticed when I tried to fix this issue myself:
#8262 (comment)

This certainly does fix a part of #8262, just not the specific case with pre-releases. It's great to see someone working on this! :)

uranusjr · 2021-11-07T03:34:31Z

Nice. In that case I think this is ready, and we should finish fixing #8262 in another PR.

uranusjr · 2021-11-07T07:52:47Z

I just noticed this PR is changing factory.py from 0o644 to 0o755. Could you fix that?

albertosottile · 2021-11-07T10:37:43Z

Fixed, thank you for having spotted that.

Jackenmen · 2021-11-07T10:58:10Z

src/pip/_internal/resolution/resolvelib/factory.py

            # PackageFinder returns earlier versions first, so we reverse.
            for ican in reversed(icans):
-                if not all_yanked and ican.link.is_yanked:
+                if (all_yanked and not is_pinned(specifier)) and ican.link.is_yanked:


Is this condition correct? It would seem that if some of the releases are not yanked, then we would want to skip yanked releases. Currently, this turns into condition:

if (False and ...) and ican.link.is_yanked: ...

which means that the yanked releases won't get ignored.
If I understand this correctly, it should instead be:

Suggested change

if (all_yanked and not is_pinned(specifier)) and ican.link.is_yanked:

if not (all_yanked and is_pinned(specifier)) and ican.link.is_yanked:

Also I've noticed that is_pinned() is called for each ican even though the specifier argument is always the same. It's probably a premature optimization but it seems that this could be calculated just once.

Is this condition correct? It would seem that if some of the releases are not yanked, then we would want to skip yanked releases.

This is actually a good point, I trusted tests/functional/test_install.py::test_ignore_yanked_file for this but it turns out there might be a bug/undocumented feature in the code.

The test is pass, but only because the icans returned by PackageFinder looks like this:

ican=<InstallationCandidate('simple', <Version('3.0')>, <Link file:///tmp/pytest-of-root/pytest-0/test_ignore_yanked_file0/data/packages/simple-3.0.tar.gz (from file:///tmp/pytest-of-root/pytest-0/test_ignore_yanked_file0/data/indexes/yanked/simple/index.html)>)> ican=<InstallationCandidate('simple', <Version('1.0')>, <Link file:///tmp/pytest-of-root/pytest-0/test_ignore_yanked_file0/data/packages/simple-1.0.tar.gz (from file:///tmp/pytest-of-root/pytest-0/test_ignore_yanked_file0/data/indexes/yanked/simple/index.html)>)> ican=<InstallationCandidate('simple', <Version('2.0')>, <Link file:///tmp/pytest-of-root/pytest-0/test_ignore_yanked_file0/data/packages/simple-2.0.tar.gz (from file:///tmp/pytest-of-root/pytest-0/test_ignore_yanked_file0/data/indexes/yanked/simple/index.html)>)>

When reversed later, simple-2.0 becomes the first package, which passes the in-loop condition as it is not yanked and is therefore installed, making the test pass.

Altering the index used by the test and un-yanking simple-3.0 corrects the list order returned by PackageFinder:

ican=<InstallationCandidate('simple', <Version('1.0')>, <Link file:///tmp/pytest-of-root/pytest-0/test_ignore_yanked_file0/data/packages/simple-1.0.tar.gz (from file:///tmp/pytest-of-root/pytest-0/test_ignore_yanked_file0/data/indexes/yanked/simple/index.html)>)> ican=<InstallationCandidate('simple', <Version('2.0')>, <Link file:///tmp/pytest-of-root/pytest-0/test_ignore_yanked_file0/data/packages/simple-2.0.tar.gz (from file:///tmp/pytest-of-root/pytest-0/test_ignore_yanked_file0/data/indexes/yanked/simple/index.html)>)> ican=<InstallationCandidate('simple', <Version('3.0')>, <Link file:///tmp/pytest-of-root/pytest-0/test_ignore_yanked_file0/data/packages/simple-3.0.tar.gz (from file:///tmp/pytest-of-root/pytest-0/test_ignore_yanked_file0/data/indexes/yanked/simple/index.html)>)>

Hence, I am not sure on how/whether to fix this. If we can rely on this returned order, then both the code and the test are valid. Otherwise,

if not (all_yanked and is_pinned(specifier)) and ican.link.is_yanked:

This is fine for me as it passes both tests.

Also I've noticed that is_pinned() is called for each ican even though the specifier argument is always the same. It's probably a premature optimization but it seems that this could be calculated just once.

Good idea, I can adapt as proposed.

@uranusjr Should I implement both the proposed changes?

Let's fix the is_pinned() thing.

The yanked ordering one is more complicated. PackageFinder ordering yanked versions last is a designed feature, defined here:

pip/src/pip/_internal/index/package_finder.py

Lines 473 to 539 in d81c65a

def _sort_key(self, candidate: InstallationCandidate) -> CandidateSortingKey:

"""

Function to pass as the `key` argument to a call to sorted() to sort

InstallationCandidates by preference.

Returns a tuple such that tuples sorting as greater using Python's

default comparison operator are more preferred.

The preference is as follows:

First and foremost, candidates with allowed (matching) hashes are

always preferred over candidates without matching hashes. This is

because e.g. if the only candidate with an allowed hash is yanked,

we still want to use that candidate.

Second, excepting hash considerations, candidates that have been

yanked (in the sense of PEP 592) are always less preferred than

candidates that haven't been yanked. Then:

If not finding wheels, they are sorted by version only.

If finding wheels, then the sort order is by version, then:

1. existing installs

2. wheels ordered via Wheel.support_index_min(self._supported_tags)

3. source archives

If prefer_binary was set, then all wheels are sorted above sources.

Note: it was considered to embed this logic into the Link

comparison operators, but then different sdist links

with the same version, would have to be considered equal

"""

valid_tags = self._supported_tags

support_num = len(valid_tags)

build_tag: BuildTag = ()

binary_preference = 0

link = candidate.link

if link.is_wheel:

# can raise InvalidWheelFilename

wheel = Wheel(link.filename)

try:

pri = -(

wheel.find_most_preferred_tag(

valid_tags, self._wheel_tag_preferences

)

)

except ValueError:

raise UnsupportedWheel(

"{} is not a supported wheel for this platform. It "

"can't be sorted.".format(wheel.filename)

)

if self._prefer_binary:

binary_preference = 1

if wheel.build_tag is not None:

match = re.match(r"^(\d+)(.*)$", wheel.build_tag)

build_tag_groups = match.groups()

build_tag = (int(build_tag_groups[0]), build_tag_groups[1])

else: # sdist

pri = -(support_num)

has_allowed_hash = int(link.is_hash_allowed(self._hashes))

yank_value = -1 * int(link.is_yanked) # -1 for yanked.

return (

has_allowed_hash,

yank_value,

binary_preference,

candidate.version,

pri,

build_tag,

)

So it's OK to rely on this behaviour. The question is whether it's a good idea to do this when we have another solution. The answer to this is usually no, so let's change it. I actually have a feeling we might change the condition again later here specifically due to the prerelease issue (we'd want PackageFinder to return those, but still only allow them if all stable releases are yanked, and I think that's only doable if we rely on ordering). But let's worry about that later.

Agreed, done.

albertosottile · 2021-11-11T18:52:20Z

Is there anything else I could/should do on my side? Sorry if I keep asking, but I do not really know the PR workflow you use in this repository...

pradyunsg · 2021-11-11T20:17:58Z

src/pip/_internal/resolution/resolvelib/factory.py

+            def is_pinned(specifier: SpecifierSet) -> bool:
+                for sp in specifier:
+                    if sp.operator == "===":
+                        return True
+                    if sp.operator != "==":
+                        continue
+                    if sp.version.endswith(".*"):
+                        continue
+                    return True
+                return False
+
+            pinned = is_pinned(specifier)


Not for this PR, but we might want to synchronise this with how the other is_pinned works in the codebase.

I believe there’re two other pinned logic that can be unified:

In provider.py, there’s a pinned = any(op[:2] == "==" for op in operators) that can be tightened up. This one just offers a “hint” to the resolver so the current logic is not very careful with the definition of “pinned”, but since we have a proper implementation now it’s a good idea to reuse it.

There’s an is_pinned property in req_install.py. This one is a stright up bug that can only be triggered by the legacy resolver. But again, it’s a good idea to also fix it if it’s not too much effort. We don’t know if the code path will be needed by future code.

If it isn't put in provider.py this PR I'm happy to submit a PR after this lands.

Currently pinned is considered more important than if a requirement is a backtracking cause in backtracking situations (I have a logical argument and tested evidence for this being faster), so being accurate about it probably reduces the chance of heavy backtracking scenarios.

pradyunsg · 2022-01-24T23:47:50Z

Merging this, on the basis of being already approved by another maintainer!

pradyunsg · 2022-01-24T23:48:01Z

Thanks @albertosottile! ^>^

Skip yanked releases unless specified

6b62c28

albertosottile mentioned this pull request Oct 30, 2021

Python 3.10: pip installs wheel for ancient Python versions #10617

Closed

1 task

uranusjr requested changes Nov 6, 2021

View reviewed changes

src/pip/_internal/resolution/resolvelib/factory.py Outdated Show resolved Hide resolved

albertosottile added 4 commits November 6, 2021 16:22

Add test_error_all_yanked_files_and_no_pin test

99cb1e0

Change specifier detection logic to exclude compatible release clauses

8293826

Minor fix in news file

2f3983e

Lint

51bda96

uranusjr reviewed Nov 6, 2021

View reviewed changes

tests/functional/test_install.py Show resolved Hide resolved

uranusjr approved these changes Nov 6, 2021

View reviewed changes

uranusjr mentioned this pull request Nov 6, 2021

pip does not ignore yanked version of package in unqualified installation when it's the only non-prerelease choice #8262

Open

uranusjr added this to the 22.0 milestone Nov 7, 2021

Fix permissions

d1ba9d5

uranusjr approved these changes Nov 7, 2021

View reviewed changes

Jackenmen reviewed Nov 7, 2021

View reviewed changes

Implement suggested changes

7057423

uranusjr approved these changes Nov 8, 2021

View reviewed changes

pradyunsg reviewed Nov 11, 2021

View reviewed changes

notatallshaw mentioned this pull request Dec 11, 2021

Abort immediately on metadata generation failure instead of backtracking #10722

Merged

4 tasks

pradyunsg merged commit 997c5a7 into pypa:main Jan 24, 2022

notatallshaw mentioned this pull request Feb 1, 2022

[DEPRECATION] Moving away from html5lib to html.parser #10825

Closed

github-actions bot locked as resolved and limited conversation to collaborators Feb 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip yanked releases unless specified #10625

Skip yanked releases unless specified #10625

albertosottile commented Oct 30, 2021 •

edited by uranusjr

Loading

pradyunsg commented Oct 30, 2021

albertosottile commented Nov 5, 2021

pradyunsg commented Nov 5, 2021

uranusjr commented Nov 6, 2021

albertosottile commented Nov 6, 2021

uranusjr left a comment •

edited

Loading

uranusjr commented Nov 6, 2021

albertosottile commented Nov 6, 2021 •

edited

Loading

Jackenmen commented Nov 6, 2021

uranusjr commented Nov 7, 2021 •

edited

Loading

uranusjr commented Nov 7, 2021

albertosottile commented Nov 7, 2021

Jackenmen Nov 7, 2021

albertosottile Nov 7, 2021 •

edited

Loading

uranusjr Nov 7, 2021 •

edited

Loading

albertosottile Nov 7, 2021

albertosottile commented Nov 11, 2021

pradyunsg Nov 11, 2021

uranusjr Nov 15, 2021 •

edited

Loading

notatallshaw Nov 15, 2021

pradyunsg commented Jan 24, 2022

pradyunsg commented Jan 24, 2022

	if (all_yanked and not is_pinned(specifier)) and ican.link.is_yanked:
	if not (all_yanked and is_pinned(specifier)) and ican.link.is_yanked:

	def _sort_key(self, candidate: InstallationCandidate) -> CandidateSortingKey:
	"""
	Function to pass as the `key` argument to a call to sorted() to sort
	InstallationCandidates by preference.

	Returns a tuple such that tuples sorting as greater using Python's
	default comparison operator are more preferred.

	The preference is as follows:

	First and foremost, candidates with allowed (matching) hashes are
	always preferred over candidates without matching hashes. This is
	because e.g. if the only candidate with an allowed hash is yanked,
	we still want to use that candidate.

	Second, excepting hash considerations, candidates that have been
	yanked (in the sense of PEP 592) are always less preferred than
	candidates that haven't been yanked. Then:

	If not finding wheels, they are sorted by version only.
	If finding wheels, then the sort order is by version, then:
	1. existing installs
	2. wheels ordered via Wheel.support_index_min(self._supported_tags)
	3. source archives
	If prefer_binary was set, then all wheels are sorted above sources.

	Note: it was considered to embed this logic into the Link
	comparison operators, but then different sdist links
	with the same version, would have to be considered equal
	"""
	valid_tags = self._supported_tags
	support_num = len(valid_tags)
	build_tag: BuildTag = ()
	binary_preference = 0
	link = candidate.link
	if link.is_wheel:
	# can raise InvalidWheelFilename
	wheel = Wheel(link.filename)
	try:
	pri = -(
	wheel.find_most_preferred_tag(
	valid_tags, self._wheel_tag_preferences
	)
	)
	except ValueError:
	raise UnsupportedWheel(
	"{} is not a supported wheel for this platform. It "
	"can't be sorted.".format(wheel.filename)
	)
	if self._prefer_binary:
	binary_preference = 1
	if wheel.build_tag is not None:
	match = re.match(r"^(\d+)(.*)$", wheel.build_tag)
	build_tag_groups = match.groups()
	build_tag = (int(build_tag_groups[0]), build_tag_groups[1])
	else: # sdist
	pri = -(support_num)
	has_allowed_hash = int(link.is_hash_allowed(self._hashes))
	yank_value = -1 * int(link.is_yanked) # -1 for yanked.
	return (
	has_allowed_hash,
	yank_value,
	binary_preference,
	candidate.version,
	pri,
	build_tag,
	)

Skip yanked releases unless specified #10625

Skip yanked releases unless specified #10625

Conversation

albertosottile commented Oct 30, 2021 • edited by uranusjr Loading

pradyunsg commented Oct 30, 2021

albertosottile commented Nov 5, 2021

pradyunsg commented Nov 5, 2021

uranusjr commented Nov 6, 2021

albertosottile commented Nov 6, 2021

uranusjr left a comment • edited Loading

Choose a reason for hiding this comment

uranusjr commented Nov 6, 2021

albertosottile commented Nov 6, 2021 • edited Loading

Jackenmen commented Nov 6, 2021

uranusjr commented Nov 7, 2021 • edited Loading

uranusjr commented Nov 7, 2021

albertosottile commented Nov 7, 2021

Jackenmen Nov 7, 2021

Choose a reason for hiding this comment

albertosottile Nov 7, 2021 • edited Loading

Choose a reason for hiding this comment

uranusjr Nov 7, 2021 • edited Loading

Choose a reason for hiding this comment

albertosottile Nov 7, 2021

Choose a reason for hiding this comment

albertosottile commented Nov 11, 2021

pradyunsg Nov 11, 2021

Choose a reason for hiding this comment

uranusjr Nov 15, 2021 • edited Loading

Choose a reason for hiding this comment

notatallshaw Nov 15, 2021

Choose a reason for hiding this comment

pradyunsg commented Jan 24, 2022

pradyunsg commented Jan 24, 2022

albertosottile commented Oct 30, 2021 •

edited by uranusjr

Loading

uranusjr left a comment •

edited

Loading

albertosottile commented Nov 6, 2021 •

edited

Loading

uranusjr commented Nov 7, 2021 •

edited

Loading

albertosottile Nov 7, 2021 •

edited

Loading

uranusjr Nov 7, 2021 •

edited

Loading

uranusjr Nov 15, 2021 •

edited

Loading