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

Feature/pipignore #4900

Closed
wants to merge 3 commits into from
Closed

Conversation

andre-merzky
Copy link

This addresses #2195.

/request-review

```
(ve)  thinkie  merzky  ~/projects/pip  [master *] $ rm -rf data/
(ve)  thinkie  merzky  ~/projects/pip  [master *] $ unset PIP_IGNORE
(ve)  thinkie  merzky  ~/projects/pip  [master *] $ rm -f .pipignore
(ve)  thinkie  merzky  ~/projects/pip  [master *] $ time pip install --no-deps --upgrade .
Looking in indexes: https://pypi.python.org/simple, https://packagecloud.io/github/git-lfs/pypi/simple
Processing /home/merzky/projects/pip
Installing collected packages: pip
  Found existing installation: pip 10.0.0.dev0
    Uninstalling pip-10.0.0.dev0:
      Successfully uninstalled pip-10.0.0.dev0
  Running setup.py install for pip ... done
Successfully installed pip-10.0.0.dev0
r:0m3.421s  u:0m2.828s  s:0m0.604s
(ve)  thinkie  merzky  ~/projects/pip  [master *] $ mkdir data
(ve)  thinkie  merzky  ~/projects/pip  [master *] $ dd if=/dev/urandom of=data/test.dat count=$((2048*1024))
2097152+0 records in
2097152+0 records out
1073741824 bytes (1.1 GB, 1.0 GiB) copied, 15.17 s, 70.8 MB/s
(ve)  thinkie  merzky  ~/projects/pip  [master *] $ time pip install --no-deps --upgrade .
Looking in indexes: https://pypi.python.org/simple, https://packagecloud.io/github/git-lfs/pypi/simple
Processing /home/merzky/projects/pip
Installing collected packages: pip
  Found existing installation: pip 10.0.0.dev0
    Uninstalling pip-10.0.0.dev0:
      Successfully uninstalled pip-10.0.0.dev0
  Running setup.py install for pip ... done
Successfully installed pip-10.0.0.dev0
r:0m41.687s  u:0m3.420s  s:0m3.320s
(ve)  thinkie  merzky  ~/projects/pip  [master *] $ export PIP_IGNORE=.pipignore
(ve)  thinkie  merzky  ~/projects/pip  [master *] $ cat > .pipignore
  # and this

data

.pipignore # because, why not

(ve)  thinkie  merzky  ~/projects/pip  [master *] $ time pip install --no-deps --upgrade .
Looking in indexes: https://pypi.python.org/simple, https://packagecloud.io/github/git-lfs/pypi/simple
Processing /home/merzky/projects/pip
Installing collected packages: pip
  Found existing installation: pip 10.0.0.dev0
    Uninstalling pip-10.0.0.dev0:
      Successfully uninstalled pip-10.0.0.dev0
  Running setup.py install for pip ... done
Successfully installed pip-10.0.0.dev0
r:0m3.495s  u:0m2.780s  s:0m0.664s

```
@pradyunsg
Copy link
Member

Nice round number! 🎉

This PR would need tests, documentation and maybe a flag to disable the .pipignore behaviour. I imagine it'll be the same format as .gitignore? Is there some way to verify/ensure that?


I personally think #4764 is a better way to address #2195 than this.

@pfmoore
Copy link
Member

pfmoore commented Dec 1, 2017

Aargh. @pradyunsg Agreed #4764 is a better resolution. I'd mistakenly recalled that as being linked with the PEP 517 stuff, which it isn't.

@andre-merzky Apologies for missing this, I may have caused you to waste some time here. Perhaps you could test #4764 and confirm it would resolve your issue?

@pradyunsg
Copy link
Member

To be clear, I am not sure if #4764 does or does not resolve this but I think that general approach of building a wheel in-place might be what we want to do here.

@andre-merzky
Copy link
Author

@pfmoore : no worries, I worked on this patch before you prompted me for it (realized that just complaining is no good... *blush*) If #4764 resolves this issue and gets merged, I'm a happy camper. I'll give it a spin with our modules... I am in no position to argue what approach is better, since I am not familiar with the pip code base etc., at all.

@pradyunsg: I am not sure a disabling-flag would make sense, at this is disabled by default, ie. nothing happens if the env var is not set. Or are you asking for a flag to disable the code path completely?

@andre-merzky
Copy link
Author

I might be missing something, but #4764 seems not to resolve the issue for me:

thinkie  merzky  ~/projects/pypa.pip  [master] $ git checkout pr/4764
[...]

thinkie  merzky  ~/projects/pypa.pip  130  [pr/4764] $ git h | head -n 1
*   9b6f1d78                                                7 hours ago             Pradyun Gedam                   (HEAD -> pr/4764, origin/pr/4764) Merge branch 'master' into cache/ephem-wheel-cache

thinkie  merzky  ~/projects/pypa.pip  [pr/4764] $ virtualenv ve
[...]

thinkie  merzky  ~/projects/pypa.pip  1  [pr/4764] $ source ve/bin/activate

(ve)  thinkie  merzky  ~/projects/pypa.pip  [pr/4764] $ pip -V
pip 9.0.1 from /home/merzky/projects/pypa.pip/ve/local/lib/python2.7/site-packages (python 2.7)

(ve)  thinkie  merzky  ~/projects/pypa.pip  [pr/4764] $ time pip install --no-deps --upgrade .
[...]
Successfully installed pip-10.0.0.dev0
r:0m5.062s  u:0m3.452s  s:0m0.980s

(ve)  thinkie  merzky  ~/projects/pypa.pip  [pr/4764] $ pip -V
pip 10.0.0.dev0 from /home/merzky/projects/pypa.pip/ve/local/lib/python2.7/site-packages/pip (python 2.7)

(ve)  thinkie  merzky  ~/projects/pypa.pip  [pr/4764] $ time pip install --no-deps --upgrade .
[...]
Successfully installed pip-10.0.0.dev0
r:0m8.556s  u:0m5.376s  s:0m1.468s

(ve)  thinkie  merzky  ~/projects/pypa.pip  [pr/4764] $ mv ../pip/data/ . ; du -sh data/
1.1G    data/

(ve)  thinkie  merzky  ~/projects/pypa.pip  [pr/4764] $ time pip install --no-deps --upgrade .
[...]
Successfully installed pip-10.0.0.dev0
r:0m31.282s  u:0m5.968s  s:0m3.856s

Any suggestion what I should be looking at?

@andre-merzky
Copy link
Author

ping - any suggestion on where I should start looking? Thanks!

@pradyunsg
Copy link
Member

#4764 seems not to resolve the issue for me

Aha. Turns out I tripped on the same wire twice. @pfmoore and I have had this exact same discussion on #4764 and @pfmoore had pointed out that this is not the case because we're still copying to the temporary build directory.

I am honestly not sure what the approach should be. The things that come to mind are this idea and allowing/doing in-place builds for directories (which avoid copying)... I'm genuinely not sure which one I dislike less than the other. :P

@pradyunsg
Copy link
Member

Any suggestion what I should be looking at?

As I mentioned, I'm not sure how to move forward here. There's no obvious path so, I'd like some inputs from other @pypa/pip-committers.

If it comes to that, I currently think that in-place builds are sorta fine for this.

@pradyunsg pradyunsg added state: needs discussion This needs some more discussion type: feature request Request for a new feature labels Dec 8, 2017
@pfmoore
Copy link
Member

pfmoore commented Dec 9, 2017

This is fixed by PEP 517, essentially by letting the backend choose whether to do in-place builds or builds from sdist. The discussion on this point was pretty extensive, but the final conclusion was fundamentally that it's not the frontend's place to decide how the wheel gets built, but we do expect backends to handle it correctly (i.e., without issues like #2195). The immediate problem here is in pip's code that essentially implements a setuptools backend, and that code will be either completely ripped out or significantly rewritten as part of implementing PEP 517.

As for the pipignore feature, while it's pretty simple, I'm not sure it'll have any meaning in a post-PEP 517 world, so my impression is that it'll end up being something that gets added and then taken away, probably all before we get a pip release. If the feature could be described (and implemented) in a way that made sense in the context of PEP 517, I'd be happier with it. As it stands, assuming this PR gets proper documentation and tests, I'd see it as something we should put on hold with the intention of rejecting it when PEP 517 is implemented, or merging it as a workaround if we decide to do a release without PEP 517 included.

I'm not sure whether @andre-merzky would be keen on putting extra work into a PR that's likely not to be implemented, and I fully understand that. The uncertainty about PEP 517 and the next release puts us in a less than ideal place at the moment, which I can only apologise for...

My personal feeling is that people have lived with #2195 for long enough now that waiting a little longer for PEP 517 should be fine.

@andre-merzky
Copy link
Author

I'm not sure whether @andre-merzky would be keen on putting extra work into a PR that's likely not to be implemented, and I fully understand that. The uncertainty about PEP 517 and the next release puts us in a less than ideal place at the moment, which I can only apologise for...

Let me turn this around: if there happens to emerge real interest in that approach, I would not mind picking this up and add the remaining pieces - just ping.

@pfmoore
Copy link
Member

pfmoore commented Dec 11, 2017

Brilliant - thanks for your understanding, and assistance with this.

@pradyunsg
Copy link
Member

@andre-merzky I'll close this PR, since even if we decide to go this way, we'll likely need a new PR anyway. :)

@pradyunsg pradyunsg closed this May 10, 2018
@lock
Copy link

lock bot commented Jun 2, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 2, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 2, 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: feature request Request for a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants