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
Merged

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

merged 14 commits into from
Aug 5, 2019

Conversation

omry
Copy link
Contributor

@omry 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
Copy link
Contributor Author

omry commented Jul 23, 2019

cc @theacodes

@theacodes
Copy link
Member

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

@dstufft
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
Copy link
Member

theacodes commented Jul 23, 2019 via email

@omry
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
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
Copy link
Member

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

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

@pfmoore
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
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 pradyunsg added state: needs discussion This needs some more discussion type: enhancement Improvements to functionality labels Jul 23, 2019
@pradyunsg
Copy link
Member

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
Copy link
Contributor

this may potentially break setuptool_scm worse than pip usually does

@pradyunsg
Copy link
Member

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
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
Copy link
Contributor

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

@pradyunsg
Copy link
Member

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
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
Copy link
Contributor Author

omry commented Jul 24, 2019

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

Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

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

Copy link
Member

@chrahunt chrahunt left a comment

Choose a reason for hiding this comment

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

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
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 omry left a comment

Choose a reason for hiding this comment

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

fixing in a diff shortly.

@omry
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
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
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 chrahunt left a comment

Choose a reason for hiding this comment

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

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')
Copy link
Member

Choose a reason for hiding this comment

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

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.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@chrahunt chrahunt left a comment

Choose a reason for hiding this comment

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

LGTM.

@omry
Copy link
Contributor Author

omry commented Aug 5, 2019

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

@pfmoore
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 pradyunsg left a comment

Choose a reason for hiding this comment

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

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
@pfmoore
Copy link
Member

pfmoore commented Aug 5, 2019

Done. Thanks again @omry!

@omry
Copy link
Contributor Author

omry commented Aug 5, 2019

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

@pradyunsg
Copy link
Member

In October. We have a 3 month release cadence.

@theacodes
Copy link
Member

theacodes commented Aug 6, 2019 via email

@lock lock bot added the auto-locked Outdated issues that have been locked by automation 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.
Labels
auto-locked Outdated issues that have been locked by automation state: needs discussion This needs some more discussion type: enhancement Improvements to functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants