exclude '.tox', '.nox' from being copied during 'pip install .' #6770

omry · 2019-07-23T02:26:10Z

'pip install .' is very slow in the presence of large directories in the source tree.
Specifial test automation directries (.tox, .nox).
This diff excludes the common culprits from the copy to a temporary directory, speeding up pip install .
significantly in such cases.

For my own repo, this gets the pip install . speed from 1 minute and 30 seconds to 2 seconds.

EDIT:
I changed this PR to address only .tox and .nox due to concerns about not copying scm directories breaking build systems.

Future improvements to this should be in the form of changing the build to build a sdist from the current directory, and then build a wheel using that.
(see comment below from @pfmoore).

…l . to speed installation up

omry · 2019-07-23T02:27:12Z

cc @theacodes

theacodes · 2019-07-23T02:36:58Z

Omg yes please. This will make nox much faster for common use cases and will also make tox faster for others.

dstufft · 2019-07-23T02:38:33Z

We did this before and had to revert it because it broke people's setup.py files, some setup.py files make assumptions about what's on disk in certain phases of development and we cant just start YOLO excluding files.

theacodes · 2019-07-23T02:40:47Z

Legacy is fun. :) Could we put this behavior behind a flag? Would that be horribly offensive?

…

On Mon, Jul 22, 2019 at 7:38 PM Donald Stufft ***@***.***> wrote: We did this before and had to revert it because it broke people's setup.py files, some setup.py files make assumptions about what's on disk in certain phases of development and we cant just start YOLO excluding files. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6770?email_source=notifications&email_token=AAB5I4YKMIUFWYFNJ5D4RGDQAZVLPA5CNFSM4IF7FWF2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2RXULA#issuecomment-514030124>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAB5I432RKXMLIL7B5PXP43QAZVLPANCNFSM4IF7FWFQ> .

omry · 2019-07-23T02:50:24Z

@dstufft, what about a .pipignore solution?

ideally we should just install what's included in the source dist. I guess if someone wants the whole git repo in their source dist its their right and they should opt in to that.
(hopefully that would make them rethink their wicked ways).

That kind of patch is a bit beyond me though, I never even looked at the code for pip before.

omry · 2019-07-23T02:59:31Z

@dstufft, also I would personally be more than happy with just .tox and .nox.
if someone relies on those they should be shot dead.

pradyunsg · 2019-07-23T03:46:08Z

For running the Linters locally: tox -e lint-py3

https://pip.pypa.io/en/stable/development/getting-started/#running-linters

pfmoore · 2019-07-23T07:13:30Z

The ideal solution for this that we're looking at is to build a sdist from the current directory, and then build a wheel using that - this would avoid copying anything but the essential files needed for a build.

omry · 2019-07-23T08:27:42Z

that would be the ideal solution.
until we get that, can we agree that excluding .tox and .nox is reasonable and is unlikely to backfire?

pradyunsg · 2019-07-23T09:11:12Z

can we agree that excluding .tox and .nox is reasonable and is unlikely to backfire?

I'm okay with this -- but if someone comes around to asking for more, I'm gonna point them at #2195 and tell them that is how we have to solve this.

I have 0 interest in managing a blocklist that has to be kept up to date.

RonnyPfannschmidt · 2019-07-23T09:46:01Z

this may potentially break setuptool_scm worse than pip usually does

pradyunsg · 2019-07-23T09:54:27Z

this may potentially break setuptool_scm worse than pip usually does

VCS directories would be continue to be copied.

That's not the current state of the PR but that's definitely what we're going to do, if we do this.

pfmoore · 2019-07-23T09:55:12Z

@RonnyPfannschmidt The original proposal not to copy VCS directories (which definitely would break setuptools_scm) or the revised one just to skip .tox and .nox?

RonnyPfannschmidt · 2019-07-23T18:42:01Z

thanks for the call out, i missed the update about the scope reduction

pradyunsg · 2019-07-23T18:56:28Z

what about a .pipignore solution?

Well... We had #4900 and even there, I think it was clear that going dir -> sdist -> installed route is a lot better.

omry · 2019-07-23T18:59:41Z

Well... We had #4900 and even there, I think it was clear that going dir -> sdist -> installed route is a lot better.

I agree that it makes more sense to use the sdist.
.pipignore would just be replicating logic that is needed to build the sdist anyway, seems more productive to not force users to define the sdist in two places.

omry · 2019-07-24T01:22:34Z

Okay, with the scope reductions I think we can move forward with this?

pradyunsg

This needs tests and a skim through the documentation to put a sentence about the skipped directories in the right place.

The tests should probably also verify that VCS directories continue to get copied.

news/6770.bugfix

omry · 2019-07-25T02:47:44Z

can you point me to a test that does something similar?
install from a directory and with a place to manipulate the input directory and verify that the temp directory does not contain .tox and .nox?

src/pip/_internal/download.py

omry · 2019-08-02T03:47:47Z

it does not seem like shutil.copytree supports having an ignore glob that only effects the top level.
@chrahunt, do you have any suggestions on how to implement it without reimplementing shutil.copytree from scratch?

chrahunt · 2019-08-02T05:02:38Z

The ignore function receives the "current" directory being iterated over and the list of contents. If the current directory is not equal to the top-level source directory then the list of contents can be returned without any changes. E.g.

def ignore(d, names):
    if d != src:
        return names
    return [name for name in names if name not in ['.nox', '.tox']]

chrahunt

Just a few minor comments.

tests/unit/test_download.py

src/pip/_internal/download.py

chrahunt · 2019-08-03T02:59:26Z

Could we also put a note about this new behavior somewhere in the docs? I would add it to or make a new section after pip_install/#build-system-interface.

omry

fixing in a diff shortly.

omry · 2019-08-03T06:13:56Z

about the doc update, I am not sure where this should go. it seems that explaining this will require explaining that pip copies the entire directory to a temp location prior to installing, which is already quiet a handful to dump on the user in the usage manual.

pfmoore · 2019-08-03T08:33:42Z

about the doc update, I am not sure where this should go

I'd be inclined to put it in the docs on the build system interface, as @cjerdonek suggested. It could be an additional paragraph (or subsection) explaining how a local source is prepared for building, explaining that it's copied to a temporary directory (excluding .tox and .nox subdirectories 😄) and then the build system is invoked on that temporary location.

I don't think it needs to be an in-depth discussion, just hit the points I mention above and that should do (if anyone feels like going into greater detail, they can always submit a follow-up PR).

omry · 2019-08-03T17:32:21Z

After looking at the docs, I feel that the proper place for this is the pip_install reference page.

chrahunt

2 minor items, otherwise LGTM.

chrahunt · 2019-08-03T17:51:10Z

tests/unit/test_download.py

+def test_unpack_file_url_excludes_expected_dirs(tmpdir, exclude_dir):
+    src_dir = tmpdir / 'src'
+    dst_dir = tmpdir / 'dst'
+    src_included_file = Path.joinpath(src_dir, 'file.txt')


Sorry, my previous comment was not very good - I meant using the joinpath method on src_dir which is now a Path, like src_dir.joinpath('file.txt'). Likewise with joinpath and touch below.

chrahunt · 2019-08-03T17:53:29Z

docs/html/reference/pip_install.rst

+
+$ pip install path/to/SomeProject
+
+During the installation, pip will copy the entire project directory to a temporary location and install from there.


I would say regular installation just so people don't get the idea that it may apply to editable installs.

chrahunt

LGTM.

omry · 2019-08-05T19:52:23Z

@pradyunsg, I feel like addressed your change request.
can you accept and land?

pfmoore · 2019-08-05T20:02:31Z

@pradyunsg This LGTM, but I don't want to merge with your review request outstanding. Can you confirm it's been dealt with (I think it has) and I'll merge (or you can)?

@omry Thanks for your contribution :-)

pradyunsg

Happy to let someone else click the merge button -- I really need to sleep.

pfmoore · 2019-08-05T22:29:39Z

Done. Thanks again @omry!

omry · 2019-08-05T22:47:48Z

Thanks!
Any estimate on when this will become available to end users?

pradyunsg · 2019-08-06T04:16:40Z

In October. We have a 3 month release cadence.

theacodes · 2019-08-06T04:24:56Z

Thanks for making this happen, everyone. We'll have some happy tox and Nox users in October. 💜

…

On Mon, Aug 5, 2019, 9:16 PM Pradyun Gedam ***@***.***> wrote: In October. We have a 3 month release cadence. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6770?email_source=notifications&email_token=AAB5I42R2SMIO7CC4PVDCQLQDD3LLA5CNFSM4IF7FWF2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3TYXRA#issuecomment-518491076>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAB5I4Z3FVV5XWBESMGHBZDQDD3LLANCNFSM4IF7FWFQ> .

omry added 2 commits July 22, 2019 19:17

exclude '.tox', '.nox', '.git', '.hg', '.bzr', '.svn' from pip instal…

3626530

…l . to speed installation up

news

82e89a9

lint

f285407

lint take 2 (not sure how to test locally)

e833052

pradyunsg added state: needs discussion This needs some more discussion type: enhancement Improvements to functionality labels Jul 23, 2019

fixed lint, excludes only .tox and .nox

67ce442

omry changed the title ~~exclude '.tox', '.nox', '.git', '.hg', '.bzr', '.svn' from 'pip install .'~~ exclude '.tox', '.nox' from being copied during 'pip install .' Jul 23, 2019

updated news

29a8b6e

omry added 2 commits July 23, 2019 18:23

renamed news file to match PR

459b4dc

simplifies new language

6b1aa6f

pradyunsg requested changes Jul 25, 2019

View reviewed changes

news/6770.bugfix Outdated Show resolved Hide resolved

chrahunt mentioned this pull request Jul 31, 2019

Ignore errors copying socket files for source installs. #6802

Closed

chrahunt suggested changes Jul 31, 2019

View reviewed changes

src/pip/_internal/download.py Outdated Show resolved Hide resolved

src/pip/_internal/download.py Outdated Show resolved Hide resolved

minor updates

d4171f2

omry added 2 commits August 2, 2019 18:17

unit test

77c1504

fixed on python 2.7

e67f066

chrahunt suggested changes Aug 3, 2019

View reviewed changes

omry commented Aug 3, 2019

View reviewed changes

responding to comments

8e87980

updated docs

ec47c86

chrahunt suggested changes Aug 3, 2019

View reviewed changes

responded to comments

6dd5727

chrahunt approved these changes Aug 3, 2019

View reviewed changes

pradyunsg approved these changes Aug 5, 2019

View reviewed changes

pfmoore merged commit e4c32b9 into pypa:master Aug 5, 2019

chrahunt mentioned this pull request Aug 25, 2019

Large project directories take a long time to install. #6916

Closed

pradyunsg mentioned this pull request Aug 25, 2019

pip install of a directory is super slow #2195

Closed

lock bot added the auto-locked Outdated issues that have been locked by automation label Sep 5, 2019

lock bot locked as resolved and limited conversation to collaborators Sep 5, 2019


		$ pip install path/to/SomeProject

		During the installation, pip will copy the entire project directory to a temporary location and install from there.

exclude '.tox', '.nox' from being copied during 'pip install .' #6770

exclude '.tox', '.nox' from being copied during 'pip install .' #6770

Conversation

omry commented Jul 23, 2019 • edited Loading

omry commented Jul 23, 2019

theacodes commented Jul 23, 2019

dstufft commented Jul 23, 2019

theacodes commented Jul 23, 2019 via email

omry commented Jul 23, 2019

omry commented Jul 23, 2019

pradyunsg commented Jul 23, 2019

pfmoore commented Jul 23, 2019

omry commented Jul 23, 2019

pradyunsg commented Jul 23, 2019

RonnyPfannschmidt commented Jul 23, 2019

pradyunsg commented Jul 23, 2019

pfmoore commented Jul 23, 2019

RonnyPfannschmidt commented Jul 23, 2019

pradyunsg commented Jul 23, 2019

omry commented Jul 23, 2019

omry commented Jul 24, 2019

pradyunsg left a comment

Choose a reason for hiding this comment

omry commented Jul 25, 2019

omry commented Aug 2, 2019

chrahunt commented Aug 2, 2019

chrahunt left a comment

Choose a reason for hiding this comment

chrahunt commented Aug 3, 2019

omry left a comment

Choose a reason for hiding this comment

omry commented Aug 3, 2019

pfmoore commented Aug 3, 2019 • edited Loading

omry commented Aug 3, 2019

chrahunt left a comment

Choose a reason for hiding this comment

chrahunt Aug 3, 2019

Choose a reason for hiding this comment

chrahunt Aug 3, 2019

Choose a reason for hiding this comment

chrahunt left a comment

Choose a reason for hiding this comment

omry commented Aug 5, 2019

pfmoore commented Aug 5, 2019

pradyunsg left a comment

Choose a reason for hiding this comment

pfmoore commented Aug 5, 2019

omry commented Aug 5, 2019

pradyunsg commented Aug 6, 2019

theacodes commented Aug 6, 2019 via email

omry commented Jul 23, 2019 •

edited

Loading

pfmoore commented Aug 3, 2019 •

edited

Loading