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

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

Merged
merged 14 commits into from Aug 5, 2019

Conversation

@omry
Copy link
Contributor

omry commented Jul 23, 2019

'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).

omry added 2 commits Jul 23, 2019
@omry

This comment has been minimized.

Copy link
Contributor Author

omry commented Jul 23, 2019

@theacodes

This comment has been minimized.

Copy link
Member

theacodes commented Jul 23, 2019

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

@dstufft

This comment has been minimized.

Copy link
Member

dstufft commented Jul 23, 2019

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

This comment has been minimized.

Copy link
Member

theacodes commented Jul 23, 2019

@omry

This comment has been minimized.

Copy link
Contributor Author

omry commented Jul 23, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

omry commented Jul 23, 2019

@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

This comment has been minimized.

Copy link
Member

pradyunsg commented Jul 23, 2019

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

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

@pfmoore

This comment has been minimized.

Copy link
Member

pfmoore commented Jul 23, 2019

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

This comment has been minimized.

Copy link
Contributor Author

omry commented Jul 23, 2019

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

This comment has been minimized.

Copy link
Member

pradyunsg commented Jul 23, 2019

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

This comment has been minimized.

Copy link
Contributor

RonnyPfannschmidt commented Jul 23, 2019

this may potentially break setuptool_scm worse than pip usually does

@pradyunsg

This comment has been minimized.

Copy link
Member

pradyunsg commented Jul 23, 2019

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

This comment has been minimized.

Copy link
Member

pfmoore commented Jul 23, 2019

@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?

@omry 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
@RonnyPfannschmidt

This comment has been minimized.

Copy link
Contributor

RonnyPfannschmidt commented Jul 23, 2019

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

@pradyunsg

This comment has been minimized.

Copy link
Member

pradyunsg commented Jul 23, 2019

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

This comment has been minimized.

Copy link
Contributor Author

omry commented Jul 23, 2019

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

This comment has been minimized.

Copy link
Contributor Author

omry commented Jul 24, 2019

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

omry added 2 commits Jul 24, 2019
Copy link
Member

pradyunsg left a comment

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 Outdated Show resolved Hide resolved
@omry

This comment has been minimized.

Copy link
Contributor Author

omry commented Jul 25, 2019

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 Outdated Show resolved Hide resolved
src/pip/_internal/download.py Outdated Show resolved Hide resolved
@omry

This comment has been minimized.

Copy link
Contributor Author

omry commented Aug 2, 2019

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

This comment has been minimized.

Copy link
Member

chrahunt commented Aug 2, 2019

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']]
omry added 2 commits Aug 3, 2019
Copy link
Member

chrahunt left a comment

Just a few minor comments.

tests/unit/test_download.py Outdated Show resolved Hide resolved
tests/unit/test_download.py Outdated Show resolved Hide resolved
tests/unit/test_download.py Outdated Show resolved Hide resolved
tests/unit/test_download.py Outdated Show resolved Hide resolved
tests/unit/test_download.py Outdated Show resolved Hide resolved
src/pip/_internal/download.py Outdated Show resolved Hide resolved
src/pip/_internal/download.py Outdated Show resolved Hide resolved
src/pip/_internal/download.py Show resolved Hide resolved
@chrahunt

This comment has been minimized.

Copy link
Member

chrahunt commented Aug 3, 2019

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.

Copy link
Contributor Author

omry left a comment

fixing in a diff shortly.

@omry

This comment has been minimized.

Copy link
Contributor Author

omry commented Aug 3, 2019

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

This comment has been minimized.

Copy link
Member

pfmoore commented Aug 3, 2019

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

This comment has been minimized.

Copy link
Contributor Author

omry commented Aug 3, 2019

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

Copy link
Member

chrahunt left a comment

2 minor items, otherwise LGTM.

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')

This comment has been minimized.

Copy link
@chrahunt

chrahunt Aug 3, 2019

Member

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.


$ pip install path/to/SomeProject

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

This comment has been minimized.

Copy link
@chrahunt

chrahunt Aug 3, 2019

Member

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

Copy link
Member

chrahunt left a comment

LGTM.

@omry

This comment has been minimized.

Copy link
Contributor Author

omry commented Aug 5, 2019

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

@pfmoore

This comment has been minimized.

Copy link
Member

pfmoore commented Aug 5, 2019

@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 :-)

Copy link
Member

pradyunsg left a comment

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

@pfmoore pfmoore merged commit e4c32b9 into pypa:master Aug 5, 2019
22 checks passed
22 checks passed
Linux Build #20190803.5 succeeded
Details
Linux (Package) Package succeeded
Details
Linux (Test Primary Python27) Test Primary Python27 succeeded
Details
Linux (Test Primary Python36) Test Primary Python36 succeeded
Details
Linux (Test Secondary Python35) Test Secondary Python35 succeeded
Details
Linux (Test Secondary Python37) Test Secondary Python37 succeeded
Details
Windows Build #20190803.5 succeeded
Details
Windows (Package) Package succeeded
Details
Windows (Test Primary Python27-x86) Test Primary Python27-x86 succeeded
Details
Windows (Test Primary Python37-x64) Test Primary Python37-x64 succeeded
Details
Windows (Test Secondary Python35-x86) Test Secondary Python35-x86 succeeded
Details
Windows (Test Secondary Python36-x86) Test Secondary Python36-x86 succeeded
Details
Windows (Test Secondary Python37-x86) Test Secondary Python37-x86 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
macOS Build #20190803.5 succeeded
Details
macOS (Package) Package succeeded
Details
macOS (Test Primary Python27) Test Primary Python27 succeeded
Details
macOS (Test Primary Python36) Test Primary Python36 succeeded
Details
macOS (Test Secondary Python35) Test Secondary Python35 succeeded
Details
macOS (Test Secondary Python37) Test Secondary Python37 succeeded
Details
news-file/pr News files updated and/or change is trivial.
Details
@pfmoore

This comment has been minimized.

Copy link
Member

pfmoore commented Aug 5, 2019

Done. Thanks again @omry!

@omry

This comment has been minimized.

Copy link
Contributor Author

omry commented Aug 5, 2019

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

@pradyunsg

This comment has been minimized.

Copy link
Member

pradyunsg commented Aug 6, 2019

In October. We have a 3 month release cadence.

@theacodes

This comment has been minimized.

Copy link
Member

theacodes commented Aug 6, 2019

@lock lock bot added the S: auto-locked label Sep 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Sep 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
7 participants
You can’t perform that action at this time.