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

Make wheel compatibility tag preferences more important than the build tag #9575

Merged
merged 7 commits into from
Apr 2, 2021

Conversation

hexagonrecursion
Copy link
Contributor

Fixes #9565

Example

If linux_x86_64 is preferred over linux_i386 the old behavior was to prefer

simple-1.0-2-py3-abi3-linux_i386.whl over
simple-1.0-1-py3-abi3-linux_x86_64.whl

Now pip prefers

simple-1.0-1-py3-abi3-linux_x86_64.whl over
simple-1.0-2-py3-abi3-linux_i386.whl

news/9565.bugfix.rst Outdated Show resolved Hide resolved
@pfmoore
Copy link
Member

pfmoore commented Feb 9, 2021

There's also a few lint issues to be fixed.

@pradyunsg
Copy link
Member

It might be worthwhile to think about / investigate if there's folks doing things in a weird way here (and decide if we need a deprecation cycle here).

I don't have any opinions on this, but just flagging this because it feels like a case where we'd create nuanced failures for someone.

Copy link
Member

@uranusjr uranusjr left a comment

Choose a reason for hiding this comment

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

At least one more test (or fixture) is needed to ensure 1.0-1-py3-abi3-linux_x86_64.whl would be prioritised over 1.0-2-py3-any-none.whl. This is arguably the more meaningful test to this change; the current test would pass without flipping the sort logic, if I understand correctly.

@uranusjr
Copy link
Member

uranusjr commented Feb 9, 2021

I seem to recall there was a research on PyPI wheels and basically nobody is using build tags right now. If that’s correct, anybody doing weird things are doing them in private indexes.

If past experience of how those people react to similar behavioural changes, I’m pessimistic how effective a deprecation cycle would be.

tests/unit/test_finder.py Outdated Show resolved Hide resolved
@hexagonrecursion
Copy link
Contributor Author

It might be worthwhile to think about / investigate if there's folks doing things in a weird way here (and decide if we need a deprecation cycle here).

Should I reclassify this a breaking instead of bugfix?

tests/unit/test_finder.py Show resolved Hide resolved
@hexagonrecursion
Copy link
Contributor Author

the current test would pass without flipping the sort logic, if I understand correctly.

I believe this is incorrect. I ran the test without the code change and got:

___ TestCandidateEvaluator.test_build_tag_is_less_important_than_other_tags ____

self = <tests.unit.test_finder.TestCandidateEvaluator object at 0x729ce6dbd130>

    def test_build_tag_is_less_important_than_other_tags(self):
        links = [
            InstallationCandidate(
                "simple",
                "1.0",
                Link('simple-1.0-1-py3-abi3-linux_x86_64.whl'),
            ),
            InstallationCandidate(
                "simple",
                '1.0',
                Link('simple-1.0-2-py3-abi3-linux_i386.whl'),
            ),
            InstallationCandidate(
                "simple",
                '1.0',
                Link('simple-1.0.tar.gz'),
            ),
        ]
        valid_tags = [
            Tag('py3', 'abi3', 'linux_x86_64'),
            Tag('py3', 'abi3', 'linux_i386'),
        ]
        evaluator = CandidateEvaluator(
            'my-project', supported_tags=valid_tags, specifier = SpecifierSet(),
        )
        sort_key = evaluator._sort_key
        results = sorted(links, key=sort_key, reverse=True)
        results2 = sorted(reversed(links), key=sort_key, reverse=True)
    
>       assert links == results == results2, results2
E       AssertionError: [<InstallationCandidate('simple', <Version('1.0')>, <Link simple-1.0-2-py3-abi3-linux_i386.whl>)>, <InstallationCandid...mple-1.0-1-py3-abi3-linux_x86_64.whl>)>, <InstallationCandidate('simple', <Version('1.0')>, <Link simple-1.0.tar.gz>)>]
E       assert [<Installatio...1.0.tar.gz>)>] == [<Installatio...1.0.tar.gz>)>]
E         At index 0 diff: <InstallationCandidate('simple', <Version('1.0')>, <Link simple-1.0-1-py3-abi3-linux_x86_64.whl>)> != <InstallationCandidate('simple', <Version('1.0')>, <Link simple-1.0-2-py3-abi3-linux_i386.whl>)>
E         Use -v to get the full diff

tests/unit/test_finder.py:310: AssertionError

@hexagonrecursion
Copy link
Contributor Author

At least one more test (or fixture) is needed to ensure 1.0-1-py3-abi3-linux_x86_64.whl would be prioritised over 1.0-2-py3-any-none.whl.

done

@pradyunsg
Copy link
Member

I'm pretty sure PyPI disallows wheels containing build tags, so this shouldn't affect PyPI-only users.

@hexagonrecursion
Copy link
Contributor Author

If past experience of how those people react to similar behavioural changes, I’m pessimistic how effective a deprecation cycle would be.

Bias alert: you hear complaints from people who ignored the deprecation until you broke their code but you never hear anything from people who headed your warning and fixed things on their end in time.

@BrownTruck
Copy link
Contributor

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 be eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Feb 20, 2021
@hexagonrecursion hexagonrecursion marked this pull request as draft February 21, 2021 11:36
@pradyunsg
Copy link
Member

@hexagonrecursion Could you update this PR?

@pradyunsg pradyunsg added the S: awaiting response Waiting for a response/more information label Apr 2, 2021
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Apr 2, 2021
@hexagonrecursion
Copy link
Contributor Author

@hexagonrecursion Could you update this PR?

Incorporated PR feedback and rebased

@hexagonrecursion hexagonrecursion marked this pull request as ready for review April 2, 2021 12:19
@hexagonrecursion
Copy link
Contributor Author

@pradyunsg you have upvoted my comment:

If past experience of how those people react to similar behavioural changes, I’m pessimistic how effective a deprecation cycle would be.

Bias alert: you hear complaints from people who ignored the deprecation until you broke their code but you never hear anything from people who headed your warning and fixed things on their end in time.

Should I change the news entry from bugfix to breaking ?

@pradyunsg
Copy link
Member

Nah, bugfix is fine here.

@uranusjr uranusjr merged commit d08f968 into pypa:main Apr 2, 2021
@hexagonrecursion hexagonrecursion deleted the test-sort-order branch April 2, 2021 12:40
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
S: awaiting response Waiting for a response/more information
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sort wheel files such that the build number is used only as a tie-breaker
6 participants