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

Prefer wheels over source distribution #6547

Merged

Conversation

dimbleby
Copy link
Contributor

@dimbleby dimbleby commented Sep 17, 2022

#5385 against recent code, and with a couple of minor improvements (but full credit @gpfv)

In particular I have

  • accounted for the possibility that a single wheel can declare support for multiple pyversion / abi / platform (the .split(".") stuff)
  • rearranged the test script so that it tests both cases independent of the platform on which the tests happen to be executing

Per #5385 this gives a meaningful performance improvement in real world cases eg on my laptop the locking of dependencies per https://github.com/lincolnloop/python-package-manager-shootout comes down from about 120s to about 60s.

It will also sidestep at least some of the many "I am trying to install a package whose pep517 build from source is broken" issues.

@dimbleby dimbleby force-pushed the prefer-platform-specific-wheels branch 2 times, most recently from 4fc297f to c3c11a7 Compare September 17, 2022 13:49
@dimbleby
Copy link
Contributor Author

dimbleby commented Sep 17, 2022

It probably ought to be safe in this code to take the first wheel always, come to think of it, whether compatible with the current platform or not. poetry is extracting metadata that ought to be the same regardless of which wheel it reads it from. Indeed it relies on that being true, else the solution that it builds is not a cross-platform solution after all.

That takes locking the shootout down from ~60s to ~45s for me without obviously breaking anything.

Views on whether that's a good idea?

@neersighted
Copy link
Member

I don't see any issue extracting metadata from the first wheel -- that is how PyPI works, and if metadata is different on one platform to another that is on the packagers.

@neersighted neersighted added this to the 1.3 milestone Sep 17, 2022
@neersighted neersighted added area/installer Related to the dependency installer kind/enhancement Not a bug or feature, but improves usability or performance impact/changelog Requires a changelog entry labels Sep 17, 2022
@dimbleby dimbleby force-pushed the prefer-platform-specific-wheels branch from c3c11a7 to 5194029 Compare September 17, 2022 15:44
@dimbleby
Copy link
Contributor Author

I don't see any issue extracting metadata from the first wheel -- that is how PyPI works, and if metadata is different on one platform to another that is on the packagers.

I mostly agree.

The really principled change to this code would just be always to look at literally any of the wheels, but doing that broke some interesting unit test. I've expanded a comment explaining why we still look for py2 and py3 universal wheels.

It is plausible that this could all be removed: recent isort has dropped python2 altogether, even 4.3.21 published a single py2.py3 wheel, if you're feeling lucky about the state of the python world then this whole code branch could just look at wheels[0] and be done.

@dimbleby dimbleby changed the title Prefer compatible wheels over source distribution Prefer wheels over source distribution Sep 17, 2022
@dimbleby dimbleby force-pushed the prefer-platform-specific-wheels branch from 5194029 to abe852b Compare September 17, 2022 15:47
@neersighted
Copy link
Member

Ultimately we are going to have to make a similar decision about how we gather metadata (or evolve the solver) for PyPI as well, as the metadata that is available is intended to be per-distfile in the long term per PEP 658.

I don't see the harm in preferring pure Python and then platform wheels for now (as opposed to the current 'universal wheel, py-version wheel, platform wheel') as ultimately it's a very minor change. Simply taking wheels[0] might be something we do in the long run, but seems too high-risk at this current time.

Copy link
Member

@neersighted neersighted left a comment

Choose a reason for hiding this comment

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

It increases the scope of this change a bit, but while you're in here would you be willing to simplify this logic to prefer a Python-only wheel (e.g. universal or py3), platform_specific_wheels[0], and then sdist?

@dimbleby
Copy link
Contributor Author

I don't understand what you're asking for that's different from what the code already does

@neersighted
Copy link
Member

neersighted commented Sep 18, 2022

Right now the logic prioritizes a universal wheel (py2.py3), then a py2 wheel (dead code), then a py3 wheel. It could be significantly simplified to instead look for any Python 3 compatible pure Python wheel ((py2\.)?py3, though that regex is a simplficiation) wheel as the first step, removing a large swathe of wheel-selection code.

@dimbleby
Copy link
Contributor Author

the bit of the code that's complicated is the bit that deals with "there's both a py2 and py3 wheel" eg per isort 4.3.4. Dropping that breaks the testcase that verifies that it works.

In what sense is py2 wheels dead code? Did poetry drop support for managing projects that want to be python2 compatible?

I suspect we're probably at cross purposes here, but I still don't understand what you're after. Feel free to push to this MR, but perhaps it's better handled separately.

@neersighted
Copy link
Member

Poetry has indeed dropped support for managing projects using Python 2.7 (https://python-poetry.org/blog/announcing-poetry-1.2.0/#dropping-support-for-managing-python-27-projects) -- though not all relevant code was removed in time for the 1.2 release.

We were forced to make this change due to our dependencies (some of which need to run inside the target environment) dropping Python 2.7 support, but given it's EOL as of nearly 24 months ago (after a years-long deprecation), I think it was overdue in Poetry anyway.

@dimbleby
Copy link
Contributor Author

There's probably a fair bit more to do than just tidy up this code if that's to be taken seriously.

There are still quite a few places in poetry-core that default to ~2.7 || >=3.4 for instance, and many of the test scripts in both projects have a similar python version constraint.

I think I'd prefer to let this MR stand as it is, I don't feel in the mood to chase down the inevitable unit test failures if I remove the py2 code.

@neersighted
Copy link
Member

Fair enough -- we'll leave that as an exercise for the next person (and indeed, the last PR to attempt to excise Python 2 support stalled due to the extensive entanglement in the test suite).

Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/installer Related to the dependency installer impact/changelog Requires a changelog entry kind/enhancement Not a bug or feature, but improves usability or performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants