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

Speed up unpack_file_url #2535

Merged
merged 1 commit into from Mar 16, 2015

Conversation

Projects
None yet
7 participants
@msabramo
Contributor

msabramo commented Mar 13, 2015

E.g.: pip install /path/to/dir

by building an sdist and then unpacking it instead of doing shutil.copytree. shutil.copytree may copy files that aren't included in the sdist, which means that:

  1. If those files are large, a pip install could take much longer than it should.
  2. Folks that are using python setup.py install to test their package are not fully testing it, because it may copy over files that wouldn't be there if they built and sdist and installed that.

So this method building an sdist is both more accurate and faster.

Fixes #2195

Cc: @tomprince, @pfmoore, @dstufft, @fungi, @rbtcollins

Speed up unpack_file_url
E.g.: `pip install /path/to/dir`

by building an sdist and then unpacking it instead of doing
`shutil.copytree`. `shutil.copytree` may copy files that aren't included
in the sdist, which means that:

1. If those files are large, a `pip install` could take much longer than
it should.

2. Folks that are using `python setup.py install` to test their package
are not fully testing it, because it may copy over files that wouldn't
be there if they built and sdist and installed that.

So this method building an sdist is both more accurate and faster.

Fixes #2195
@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore Mar 13, 2015

Member

👍

Member

pfmoore commented Mar 13, 2015

👍

@msabramo

This comment has been minimized.

Show comment
Hide comment
@msabramo

msabramo Mar 13, 2015

Contributor

This one should probably require a few thumbs up before merging because my first attempt at this messed pbr and had to be reverted.

So I'd love for OpenStack folks to give this a try and weigh in.

Contributor

msabramo commented Mar 13, 2015

This one should probably require a few thumbs up before merging because my first attempt at this messed pbr and had to be reverted.

So I'd love for OpenStack folks to give this a try and weigh in.

@msabramo

This comment has been minimized.

Show comment
Hide comment
@msabramo

msabramo Mar 13, 2015

Contributor

This could replace #2533

Contributor

msabramo commented Mar 13, 2015

This could replace #2533

@emonty

This comment has been minimized.

Show comment
Hide comment
@emonty

emonty Mar 14, 2015

Contributor

@msabramo thanks! I'll run it through some pbr paces this weekend.

Contributor

emonty commented Mar 14, 2015

@msabramo thanks! I'll run it through some pbr paces this weekend.

@msabramo

This comment has been minimized.

Show comment
Hide comment
@msabramo

msabramo Mar 14, 2015

Contributor

Thanks, @emonty! I hope this change works better for pbr than my earlier one. It would be really nice to make pip install /path/to/dist work a lot faster (and be a more realistic test of what will happen when you build an sdist and publish it).

Contributor

msabramo commented Mar 14, 2015

Thanks, @emonty! I hope this change works better for pbr than my earlier one. It would be really nice to make pip install /path/to/dist work a lot faster (and be a more realistic test of what will happen when you build an sdist and publish it).

@dstufft

This comment has been minimized.

Show comment
Hide comment
@dstufft

dstufft Mar 16, 2015

Member

Talked to @emonty on IRC and it appears like this works for pbr now.

Member

dstufft commented Mar 16, 2015

Talked to @emonty on IRC and it appears like this works for pbr now.

dstufft added a commit that referenced this pull request Mar 16, 2015

@dstufft dstufft merged commit d01e7d6 into pypa:develop Mar 16, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@erikrose

This comment has been minimized.

Show comment
Hide comment
@erikrose

erikrose Sep 11, 2015

Contributor

It looks like you forgot to commit the bit that actually calls your new function. Do you still have that lying around anywhere?

Contributor

erikrose commented on 4e6d935 Sep 11, 2015

It looks like you forgot to commit the bit that actually calls your new function. Do you still have that lying around anywhere?

@Ivoz

This comment has been minimized.

Show comment
Hide comment
@Ivoz

Ivoz Sep 12, 2015

Member

For posterity, atm, it looks like it was supposed to be

     if os.path.isdir(link_path):
-        if os.path.isdir(location):
-            rmtree(location)
-        shutil.copytree(link_path, location, symlinks=True)
+        _copy_dist_from_dir(link_path, location)

at Line 696

That said, when a function called unpack_file_url is using subprocess calls to execute setup.pys things get semantically confusing for me. Seems like things could use a little re-(naming/factoring). Also might have to be careful this isn't on a codepath which allows setuptools to not be installed (for wheels, for instance).

Member

Ivoz commented Sep 12, 2015

For posterity, atm, it looks like it was supposed to be

     if os.path.isdir(link_path):
-        if os.path.isdir(location):
-            rmtree(location)
-        shutil.copytree(link_path, location, symlinks=True)
+        _copy_dist_from_dir(link_path, location)

at Line 696

That said, when a function called unpack_file_url is using subprocess calls to execute setup.pys things get semantically confusing for me. Seems like things could use a little re-(naming/factoring). Also might have to be careful this isn't on a codepath which allows setuptools to not be installed (for wheels, for instance).

rgommers added a commit to rgommers/pip that referenced this pull request Nov 1, 2015

BUG: finish fix for speeding up "pip install .".
This is a follow-up to pypagh-2535, which added the code to copy via
(sdist + unpack) instead of shutil.copytree, but forgot to actually
call that function.

Fixes pypagh-2195 (slow pip install of a dir).

rgommers added a commit to rgommers/pip that referenced this pull request Nov 3, 2015

BUG: finish fix for speeding up "pip install .".
This is a follow-up to pypagh-2535, which added the code to copy via
(sdist + unpack) instead of shutil.copytree, but forgot to actually
call that function.

Fixes pypagh-2195 (slow pip install of a dir).

warner added a commit to warner/pip that referenced this pull request Apr 13, 2016

Copy setup.py into place too.
This works around a problem with the new sdist-based "pip install .":

* when creating the sdist, we don't run a literal "setup.py sdist"
* instead, sys.argv[0] is a complicated shim that injects
  setuptools even into distutils-based projects
* as a result, distutils.command.sdist.add_defaults() doesn't realize
  that "setup.py" is the name of its setup script (it gets confused
  because sys.argv[0] is not a real file).
* so add_defaults() doesn't include setup.py in the generated
  tarball. (projects could add "include setup.py" to their MANIFEST.in,
  but this is not common practice because usually it's automatic)
* so the unpacked sdist (from which pip will make a wheel) lacks the
  critical setup.py

This copies the setup.py from source tree to unpacked target tree.

The patch also removes a performance comment that was obsoleted by
switching to _copy_dist_from_dir().

refs pypa#2195, pypa#2535, pypa#3176
@henrykironde

This comment has been minimized.

Show comment
Hide comment
@henrykironde

henrykironde Jan 11, 2017

In case it helps: I compared pip and python setup.py install. I found that when I included data in the directory structure, pip would try to copy all this data. This would take over 5 minutes since my project can create lots of data when "testing under development". python setup.py install would not include the files since I defined the folders and files to include(or packages). Am wondering if pip does not clearly detect the required files in any corrupted directory structure.

henrykironde commented Jan 11, 2017

In case it helps: I compared pip and python setup.py install. I found that when I included data in the directory structure, pip would try to copy all this data. This would take over 5 minutes since my project can create lots of data when "testing under development". python setup.py install would not include the files since I defined the folders and files to include(or packages). Am wondering if pip does not clearly detect the required files in any corrupted directory structure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment