Skip to content
This repository has been archived by the owner on Mar 19, 2024. It is now read-only.

Add --no-use-wheel to default pip install arguments #53

Closed
wants to merge 3 commits into from
Closed

Add --no-use-wheel to default pip install arguments #53

wants to merge 3 commits into from

Conversation

willkg
Copy link
Collaborator

@willkg willkg commented Oct 6, 2014

pip 1.4.0 through 1.5.6 (and possibly later versions which haven't come
out, yet) support wheels, but don't upgrade them correctly. So instead
of dealing with virtual environments that can grow cruft, it's better to
just not use wheels for now.

This adds --no-use-wheel to the default pip install arguments to
prevent pip from using wheels.

Fixes #50

@willkg
Copy link
Collaborator Author

willkg commented Oct 6, 2014

@erikrose I tried to do the minimum changes I could get away with that worked. In the issue, I suggested we leave it unbounded until pip releases a version that works. I bounded it in this patch--I'm kind of on the fence about which way is best. Thoughts?

@mythmon
Copy link
Contributor

mythmon commented Oct 6, 2014

What happens if a user says peep install --use-wheels -r requirements.txt? What happens if a user specified --no-use-wheels?

@willkg
Copy link
Collaborator Author

willkg commented Oct 6, 2014

If someone specifies --no-use-wheel, then we end up with two in the arguments, but pip works fine with that. I could add a check for it already existing.

I don't see a --use-wheels argument. Where are you seeing that?

@mythmon
Copy link
Contributor

mythmon commented Oct 6, 2014

Oh. I made an assumption without actually checking the docs. There is no option, and I'm just being silly.

I have no more objections. +1 from me.

pip 1.4.0 through 1.5.6 (and possibly later versions which haven't come
out, yet) support wheels, but don't upgrade them correctly. So instead
of dealing with virtual environments that can grow cruft, it's better to
just not use wheels for now.

This adds ``--no-use-wheel`` to the default pip install arguments to
prevent pip from using wheels.

Fixes #50
@willkg
Copy link
Collaborator Author

willkg commented Oct 6, 2014

^^^ That checks for the argument first before applying.

@willkg
Copy link
Collaborator Author

willkg commented Oct 6, 2014

Bah. pip 1.4 has a --use-wheel argument, but 1.5 does not. I'll tweak the code to handle that.

@willkg
Copy link
Collaborator Author

willkg commented Oct 6, 2014

^^^ That handles things correctly with the two different arguments in the two different versions of pip. Yay.

if pip.__version__ >= '1.5' and pip.__version__ <= '1.5.6' and '--no-use-wheel' not in initial_args:
initial_args.insert(1, '--no-use-wheel')

print('>>> ', initial_args)
Copy link
Contributor

Choose a reason for hiding this comment

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

Debug line?

@willkg
Copy link
Collaborator Author

willkg commented Oct 6, 2014

^^^ Bah. Nixed the debug statement.

@erikrose
Copy link
Owner

erikrose commented Oct 7, 2014

A horrible thought: what if someone wants to use wheels for speed (imagine some package where compiling takes forever) but always starts from a blank virtualenv? Then we'd be taking away perfectly good functionality, from their viewpoint. What's more, this is a very reasonable scenario, since there's no facility yet for removing packages from a venv that are no longer in the requirements file—if you don't want an ever-growing venv full of cruft, you have to start fresh.

I wonder if we could take into account whether the package is already installed before freaking out about the wheel option. (Search for the "req.satisfied_by" check.)

if pip.__version__ >= '1.4' and pip.__version__ < '1.5' and '--use-wheel' in initial_args:
initial_args.remove('--use-wheel')

if pip.__version__ >= '1.5' and pip.__version__ <= '1.5.6' and '--no-use-wheel' not in initial_args:
Copy link
Owner

Choose a reason for hiding this comment

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

Can we really get away with all these stringwise comparisons? If we ever get to 1.5.10, I could imagine trouble. I bet there's a tuple-ized rendition of the pip version number floating around somewhere.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

According to the pip bug, there will be no 1.5.7 and the next version will be 1.6.

Copy link
Owner

Choose a reason for hiding this comment

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

But '1.10' < '1.4'.

@willkg
Copy link
Collaborator Author

willkg commented Oct 7, 2014

I'm kind of hitting the end of my "what if..." handling abilities. Right now SUMO updates are busted in regards to upgrades, so there's some urgency in fixing this. @mythmon is loathe to edit his peep locally (I'm not--I already fixed this for fjord (though I need to tweak that fix)), so I think it needs to be fixed upstream for SUMO's purposes.

peep has no tests. I know you worked on that last night, but I doubt we'll have a sufficient test suite that I'm comfortable about doing complex solutions to this. Thus I'm trying real hard to keep this as simple as possible and that probably means we can't satisfy everyone. Hopefully this is a stopgap until pip 1.6 comes out and then people can do whatever it is they want to do again. If they don't like that, they can edit their version of peep which is made easier because the code in question is in one place and pretty clearly documented.

So because of time and complexity constraints (plus I'm not wildly interested in spending a lot more time on this) I think I have these options:

  1. If you consider that use case to be really important, then I'll abandon this effort, users can have wheel upgrade problems, and I'll just edit my local copy.
  2. If you consider this use case to be really important and have a simple fix in mind that you want to try your hand at, then we can land this and you can fix it further.
  3. If you don't consider this important then we land this as is because it's pretty simple and it's tested.

@erikrose
Copy link
Owner

erikrose commented Oct 7, 2014

A couple of discrete thoughts:

  • I can understand how you're at the end of your rope; Junk leftover when installing a new version after installing github tarball. #50 was a pretty long thread.
  • It makes me nervous that you're both reusing venvs from deployment to deployment, since you have no way of removing no-longer-needed libs without getting on the server and mucking around.
  • I am willing to merge this for Mozilla's sake; there aren't thousands upon thousands of people using peep yet, and we can respond to the wheel use case iff people complain. But I would like to fix the version comparison first and also the mutation of the initial_args arg in run_pip first. I can't promise I will have time to do that myself today.

#
# This prevents pip from using wheels for those versions.
if pip.__version__ >= '1.4' and pip.__version__ < '1.5' and '--use-wheel' in initial_args:
initial_args.remove('--use-wheel')
Copy link
Owner

Choose a reason for hiding this comment

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

Let's construct a new list rather than mutating the arg. Mutating could lead to some very tricky bugs down the road.

@willkg
Copy link
Collaborator Author

willkg commented Oct 7, 2014

I've got bug https://bugzilla.mozilla.org/show_bug.cgi?id=1074900 covering removing things from the virtualenv. I'm planning to fix that, too, so I'm not worried about that use case.

Generating a new virtualenv every deploy is pretty intense unless someone figures out a good caching solution.

@willkg
Copy link
Collaborator Author

willkg commented Oct 7, 2014

Also, if we're expecting people to be deleting virtualenvs and creating new ones, then we don't need this and we should just end it here.

@erikrose
Copy link
Owner

erikrose commented Oct 7, 2014

The typical --download-cache should serve. You still hit the index, but you don't redownload everything all the time.

As for deleting venvs, that's a best practice IMO, but individuals can make that tradeoff. peep is a repeatable installer, not a full-on deployment tool. That's more where I was targeting Shiva.

@willkg
Copy link
Collaborator Author

willkg commented Oct 8, 2014

Given that, I vote we switch gears back to option 1 in comment #50 (comment) in #50. That seems to be the best option given the various use cases discussed here and it's the simplest solution, too.

I'll close this PR out and do one for a docs change.

@willkg willkg closed this Oct 8, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Junk leftover when installing a new version after installing github tarball.
3 participants