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

BUG: finish fix for speeding up "pip install ." #3219

Closed
wants to merge 3 commits into from

Conversation

rgommers
Copy link

@rgommers rgommers commented Nov 1, 2015

This is a follow-up to gh-2535, which added the code to copy via
(sdist + unpack) instead of shutil.copytree, but forgot to actually
call that function.

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

I've tested that this works (on scipy), but since gh-2535 refers to some issues with pbr / OpenStack and the previous PR didn't change pip's behavior the pbr test should be redone.

Review on Reviewable

@rgommers
Copy link
Author

rgommers commented Nov 1, 2015

OK this isn't a complete fix. The TravisCI build fails on executing setup.py sdist on a wheel (?).

There's also an issue with pip install .:

# just showing that the starting point is clean
$ echo $PYTHONPATH
$ git clean -xdf
$ pip uninstall pywt
Cannot uninstall requirement pywt, not installed
$ pip uninstall PyWavelets
Cannot uninstall requirement PyWavelets, not installed

$ pip install . --user
Processing /home/rgommers/Code/tmp/pywt
  Running setup.py sdist for /home/rgommers/Code/tmp/pywt
  Unpacking sdist /tmp/pip-NDK_2H-build/PyWavelets-0.4.0.dev0+da1c6b4.tar.gz into /tmp/pip-NDK_2H-build
  Requirement already satisfied (use --upgrade to upgrade): PyWavelets==0.4.0

What's happening here apparently is that setup.py sdist is generating a PyWavelets.egg-info and this is then found and recognized as an installed version. Hence installation fails.

The original PR probably never worked.....

@rgommers
Copy link
Author

rgommers commented Nov 2, 2015

Can anyone tell me what the best way is to run the relevant subset of tests here? I ran tox -e py34, but it takes forever (still not done after maybe 1.5 hours).

@dstufft
Copy link
Member

dstufft commented Nov 3, 2015

You can pass a filename to it like tox -e py34 tests/test_foo.py and it'll only run Those tests. You can also use pytest -k option to run s single test.

Sent from my iPhone

On Nov 2, 2015, at 5:56 PM, Ralf Gommers [email protected] wrote:

Can anyone tell me what the best way is to run the relevant subset of tests here? I ran tox -e py34, but it takes forever (still not done after maybe 1.5 hours).


Reply to this email directly or view it on GitHub.

@rgommers
Copy link
Author

rgommers commented Nov 3, 2015

Thanks @dstufft . I'll only run the pip install tests then and leave the rest to TravisCI.

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

rgommers commented Nov 8, 2015

I'll finish this up (if needed) after the discussion on what pip install <local_dir> should do has come to a conclusion.

@rgommers
Copy link
Author

rgommers commented Nov 8, 2015

To clarify, this is what @dstufft has in mind currently (needs this PR):
@dstufft final situation current proposal

And this is what I'd prefer (doesn't need this PR):
pip_install_paths_proposal

Discussion on mailing list: http://article.gmane.org/gmane.comp.python.distutils.devel/24687

@pfmoore
Copy link
Member

pfmoore commented Nov 8, 2015

FWIW, I agree with @dstufft. Your diagram doesn't include a way to get from directory to sdist - which while it may not be needed for the limited case of installing from a local directory, will be needed elsewhere (for example to release from a local directory).

@rgommers
Copy link
Author

rgommers commented Nov 8, 2015

FWIW, I agree with @dstufft. Your diagram doesn't include a way to get from directory to sdist - which while it may not be needed for the limited case of installing from a local directory, will be needed elsewhere (for example to release from a local directory).

It depends. There's no sdist command in Robert's build-PEP. There's also no pip sdist, while there is pip wheel. The flit author seems to disagree, he doesn't want to implement flit sdist. So it certainly seems optional.

@rgommers
Copy link
Author

rgommers commented Nov 8, 2015

Or is there a plan to add pip sdist?

@dstufft
Copy link
Member

dstufft commented Nov 8, 2015

Well the build PEP hasn't been accepted yet :)

There isn't a pip sdist yet mostly because right now pip blurs the lines between sdist and directory (treating it more like an unpacked sdist) which causes some of the problems I listed on distutils-sig. My goal is aspirational, this is the direction I want to see us move in because I think it will reduce the confusion of everyone involved when these two actions end up with the same result:

$ python setup.py sdist
$ pip install ./dist/foobar-1.0.tar.gz

and

$ pip install .

Instead of being subtly different. In that vein, if we go this direction (and I'm not some dictator of course, so it's not set in stone) then we would likely end up adding a pip sdist command (and we might add it either way).

The flit author may not want to create a sdist, but I don't really think they are going to be able to do that in the larger ecosystem. Downstream redistributors are not going to accept wheels (even pure Python ones) as the source of their packages.

@rgommers
Copy link
Author

rgommers commented Nov 8, 2015

Hmm, then that's a pretty fundamental difference. I'd like to just teach package authors to properly test their sdists in their CI (it's not that hard), and have

$ python setup.py bdist_wheel
$ pip install ./dist/foobar-1.0.whl

be the same as pip install ..

The internal complexity for pip is similar for both proposals, and the advantage of better testing for sdist to me is very minor compared to interfering with build system caching and slower installs due to extra sdist generation and copying. But I guess we're simply disagreeing on that trade-off....

@dstufft
Copy link
Member

dstufft commented Nov 8, 2015

Right, and I'm sure the reason we're disagreeing on that trade-off is you're used to projects that take a significant amount of time to build and I'm used to random folks getting confused because of the subtle differences between installing from an sdist created from a directory and installing directly from a directory :)

At the end of the day, the answer will probably come from whatever PEP ends up getting accepted for the build system abstraction and what capabilities that mandates a build system have. If it mandates that it has an sdist step and the process is directory -> sdist -> wheel -> install, then pip will follow that. If it mandates that sdists are really just a special form of a local directory and that you can go either from an existing local directory OR a sdist into a wheel, then pip will do that. If it's my preferred way, then that obviously kills the ability to let the build system do caching between multiple invocations of pip install .. If it's your preferred way then we'll have to figure out if pip wants (or needs) to still copy a local directory to a temporary build directory, or whether or not doing the build in place is fine.

@rgommers
Copy link
Author

rgommers commented Nov 9, 2015

@dstufft agreed, makes sense to let this depend on the outcome of the build PEP.

@njsmith
Copy link
Member

njsmith commented Jan 17, 2016

For the record, I think at this point the only reason numpy and scipy haven't already disabled setup.py install is pip's insistence on breaking incremental builds.

@rgommers
Copy link
Author

For the record, I think at this point the only reason numpy and scipy haven't already disabled setup.py install is pip's insistence on breaking incremental builds.

@njsmith not quite, gh-536 also still needs fixing.

@rgommers
Copy link
Author

After some recent packaging work I was reminded again that python setup.py sdist has absolutely horrible behavior if you use setuptools (plain distutils is much better), so as soon as this PR would be finalized it will give lots of issues. Yet another reasons why I'd rather not see this finished and merged.

Unfortunately it looks like the build PEP didn't explicitly resolve this ....

@dstufft
Copy link
Member

dstufft commented Jan 29, 2016

Can you clarify what that behavior is?

@rgommers
Copy link
Author

Can you clarify what that behavior is?

Here's a few issues: numpy/numpy#7127 (comment).

IIRC the inclusion of vcs history got fixed at some point, but not sure if that was for all vcs's or only for svn/git/hg.

I haven't checked yet if egg_info is regenerated at build/install time (I assume so), but running it under setup.py sdist is weird.

Also, ignoring the way distutils handles MANIFEST.in, which is clear and understandable, and instead going off and determining for the user what should and should be included leads to hard to predict issues. That's probably a main reason why all the scientific projects I checked explicitly import sdist from distutils.commands even though they use setuptools for most other things.

@dstufft dstufft closed this May 18, 2016
@dstufft
Copy link
Member

dstufft commented May 18, 2016

Accidentally closed this, reopening. Sorry!

@dstufft dstufft reopened this May 18, 2016
@BrownTruck
Copy link
Contributor

Hello!

As part of an effort to ease the contribution process and adopt a more standard workflow pip has switched to doing development on the master branch. However, this Pull Request was made against the develop branch so it will need to be resubmitted against master. Unfortunately, this pull request does not cleanly merge against the current master branch.

If you do nothing, this Pull Request will be automatically closed by @BrownTruck since it cannot be merged.

If this pull request is still valid, please rebase it against master (or merge master into it) and resubmit it against the master branch, closing and referencing the original Pull Request.

If you choose to rebase/merge and resubmit this Pull Request, here is an example message that you can copy and paste:

This is a follow-up to gh-2535, which added the code to copy via
(sdist + unpack) instead of shutil.copytree, but forgot to actually
call that function.

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

I've tested that this works (on scipy), but since gh-2535 refers to some issues with pbr / OpenStack and the previous PR didn't change pip's behavior the pbr test should be redone.

---

*This was migrated from pypa/pip#3219 to reparent it to the ``master`` branch. Please see original pull request for any previous discussion.*

@BrownTruck BrownTruck added asked to reparent auto-bitrotten PRs that died because they weren't updated labels May 19, 2016
@BrownTruck
Copy link
Contributor

This Pull Request was closed because it cannot be automatically reparented to the master branch and it appears to have bit rotted.

Please feel free to re-open it or re-submit it if it is still valid and you have rebased it onto master or merged master into it.

@BrownTruck BrownTruck closed this May 26, 2016
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 3, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-bitrotten PRs that died because they weren't updated auto-locked Outdated issues that have been locked by automation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants