Use wheelfile-based pkg_resources.Distribution for metadata #7539

chrahunt · 2020-01-02T03:04:55Z

Use wheel-file-based pkg_resources.Distribution for metadata

Adapts the zipfile-compatible wheel metadata functions from #7538 to back a pkg_resources.Distribution. This allows us to remove wheel file unpacking everywhere except right before installation.

Progresses #6030 and implements simplification described in #7483 (comment).

pradyunsg · 2020-01-02T10:01:49Z

@-me when this is ready for review. :)

sbidoul

Very nice. Code LGTM.

As this should improve performance with large wheels, this could be mentioned in a news file?

One can probably also remove the unpack_file in the preparer if link.is_wheel (in a followup)?

pip/src/pip/_internal/operations/prepare.py

Lines 162 to 164 in 08f61a9

    
           # unpack the archive to the build dir location. even when only 
        
           # downloading archives, they have to be unpacked to parse dependencies 
        
           unpack_file(from_path, location, content_type)

src/pip/_internal/utils/wheel.py

tests/lib/__init__.py

src/pip/_internal/utils/wheel.py

chrahunt · 2020-01-03T21:58:20Z

As this should improve performance with large wheels, this could be mentioned in a news file?

It should for pip wheel. I would mention it after removing the now-unnecessary unpack_file you mentioned, since otherwise the "if"s and "except"s in the news entry here would make it too wordy.

pkg_resources.Distribution classes delegate to the IMetadataProvider implementation provided at construction. This is the one we'll use for adapting a ZipFile to pkg_resources.DistInfoDistribution.

pradyunsg

LGTM.

Feel free to merge if you want to -- these comments aren't blockers.

There's another spot where we do unconditional unpacking, as @sbidoul has helpfully pointed out too. I would've preferred to change that too, in this PR since we're already removing one unnecessary location where we're unpacking -- but doing it in a follow up wouldn't hurt either. :)

Thanks for breaking this up into smaller PRs that were independently reviewed. :)

src/pip/_internal/wheel_builder.py

src/pip/_internal/distributions/wheel.py

src/pip/_internal/utils/pkg_resources.py

tests/functional/test_install_wheel.py

chrahunt · 2020-01-05T10:39:18Z

There's another spot where we do unconditional unpacking, as @sbidoul has helpfully pointed out too. I would've preferred to change that too, in this PR since we're already removing one unnecessary location where we're unpacking -- but doing it in a follow up wouldn't hurt either. :)

I would have, but I wanted to move all unpacking out of the individual "unpack_*" functions in operations.prepare (and rename them) before that, to avoid making that code worse. :)

sbidoul

Tested again, works fine. 👍

tests/functional/test_install_wheel.py

We now extract all metadata files from the wheel directly into memory and make them available to the wrapping pkg_resources.Distribution via the DictMetadata introduced earlier.

Actual installation has been using the wheel file directly for some time. The last piece that required an unpacked wheel was metadata. Now that it uses the wheel file directly, we can remove the unpacking after build.

chrahunt · 2020-01-05T17:24:23Z

Thank you both for reviewing.

chrahunt added type: refactor Refactoring code skip news Does not need a NEWS file entry (eg: trivial changes) labels Jan 2, 2020

chrahunt changed the title ~~Refactor/get dist from zip~~ Use wheelfile-based pkg_resources.Distribution for metadata Jan 2, 2020

chrahunt force-pushed the refactor/get-dist-from-zip branch from 4a1ef42 to bfffb32 Compare January 2, 2020 13:11

chrahunt marked this pull request as ready for review January 2, 2020 13:45

chrahunt requested a review from pradyunsg January 2, 2020 13:46

sbidoul approved these changes Jan 3, 2020

View reviewed changes

src/pip/_internal/utils/wheel.py Outdated Show resolved Hide resolved

tests/lib/__init__.py Outdated Show resolved Hide resolved

sbidoul reviewed Jan 3, 2020

View reviewed changes

src/pip/_internal/utils/wheel.py Outdated Show resolved Hide resolved

This was referenced Jan 3, 2020

Move parse_wheel and supporting functions to utils.wheel #7550

Merged

Use valid wheel for functional download tests #7551

Merged

chrahunt force-pushed the refactor/get-dist-from-zip branch 2 times, most recently from d9af6ac to cd9a678 Compare January 5, 2020 01:45

chrahunt added 3 commits January 4, 2020 20:49

Add DictMetadata for adapting zip data to Distribution

20706eb

pkg_resources.Distribution classes delegate to the IMetadataProvider implementation provided at construction. This is the one we'll use for adapting a ZipFile to pkg_resources.DistInfoDistribution.

Parameterize wheel file path for metadata extraction

ae34781

Extract reading wheel file from wheel_metadata into separate function

ed65393

chrahunt force-pushed the refactor/get-dist-from-zip branch from cd9a678 to 64f887f Compare January 5, 2020 01:50

pradyunsg approved these changes Jan 5, 2020

View reviewed changes

src/pip/_internal/wheel_builder.py Show resolved Hide resolved

src/pip/_internal/distributions/wheel.py Show resolved Hide resolved

src/pip/_internal/utils/pkg_resources.py Show resolved Hide resolved

tests/functional/test_install_wheel.py Outdated Show resolved Hide resolved

pradyunsg reviewed Jan 5, 2020

View reviewed changes

tests/functional/test_install_wheel.py Show resolved Hide resolved

sbidoul reviewed Jan 5, 2020

View reviewed changes

tests/functional/test_install_wheel.py Outdated Show resolved Hide resolved

chrahunt force-pushed the refactor/get-dist-from-zip branch from 64f887f to 561ef79 Compare January 5, 2020 15:50

chrahunt added 2 commits January 5, 2020 10:57

Use pkg_resources.Distribution derived from wheel directly

33043ba

We now extract all metadata files from the wheel directly into memory and make them available to the wrapping pkg_resources.Distribution via the DictMetadata introduced earlier.

Don't unpack wheel files after building for install

a94fb53

Actual installation has been using the wheel file directly for some time. The last piece that required an unpacked wheel was metadata. Now that it uses the wheel file directly, we can remove the unpacking after build.

chrahunt force-pushed the refactor/get-dist-from-zip branch from 561ef79 to a94fb53 Compare January 5, 2020 15:57

sbidoul approved these changes Jan 5, 2020

View reviewed changes

chrahunt merged commit b7ed044 into pypa:master Jan 5, 2020

chrahunt deleted the refactor/get-dist-from-zip branch January 5, 2020 17:24

lock bot added the auto-locked Outdated issues that have been locked by automation label Feb 4, 2020

lock bot locked as resolved and limited conversation to collaborators Feb 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use wheelfile-based pkg_resources.Distribution for metadata #7539

Use wheelfile-based pkg_resources.Distribution for metadata #7539

chrahunt commented Jan 2, 2020 •

edited

Loading

pradyunsg commented Jan 2, 2020

sbidoul left a comment

chrahunt commented Jan 3, 2020 •

edited

Loading

pradyunsg left a comment

chrahunt commented Jan 5, 2020 •

edited

Loading

sbidoul left a comment

chrahunt commented Jan 5, 2020

	# unpack the archive to the build dir location. even when only
	# downloading archives, they have to be unpacked to parse dependencies
	unpack_file(from_path, location, content_type)

Use wheelfile-based pkg_resources.Distribution for metadata #7539

Use wheelfile-based pkg_resources.Distribution for metadata #7539

Conversation

chrahunt commented Jan 2, 2020 • edited Loading

pradyunsg commented Jan 2, 2020

sbidoul left a comment

Choose a reason for hiding this comment

chrahunt commented Jan 3, 2020 • edited Loading

pradyunsg left a comment

Choose a reason for hiding this comment

chrahunt commented Jan 5, 2020 • edited Loading

sbidoul left a comment

Choose a reason for hiding this comment

chrahunt commented Jan 5, 2020

chrahunt commented Jan 2, 2020 •

edited

Loading

chrahunt commented Jan 3, 2020 •

edited

Loading

chrahunt commented Jan 5, 2020 •

edited

Loading