paylogic / pip-accel

pip-accel: Accelerator for pip, the Python package manager
https://pypi.python.org/pypi/pip-accel
MIT License
308 stars 35 forks source link

Adding max-retries to configuration and fixing issues with uninstall. #48

Closed ryanjoneil closed 9 years ago

ryanjoneil commented 9 years ago

This fixes and issue we are observing where upgrades of packages do not happen cleanly. The reason is that pip-accel uninstall does not remove package contents.

If you pip-accel install scikit-learn==0.14.1 and then pip uninstall scikit-learn, there will still be an sklearn directory with Python code in it. Upgrading to scikit-learn==0.15.2 afterward will give a broken build.

$ python -c 'from sklearn.metrics.pairwise import manhattan_distances'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/usr/local/lib/python2.7/dist-packages/sklearn/metrics/__init__.py", line 6, in <module>
    from .metrics import (accuracy_score,
  File "/usr/local/lib/python2.7/dist-packages/sklearn/metrics/metrics.py", line 31, in <module>
    from ..preprocessing import LabelBinarizer, label_binarize
  File "/usr/local/lib/python2.7/dist-packages/sklearn/preprocessing/__init__.py", line 6, in <module>
    from .data import Binarizer
  File "/usr/local/lib/python2.7/dist-packages/sklearn/preprocessing/data.py", line 25, in <module>
    from ..utils.sparsefuncs import inplace_column_scale
ImportError: cannot import name inplace_column_scale

The reason pip-accel can't uninstall binary packages is that it is building them using python setup.py bdist or python setup.py bdist_dumb. These commands do not create an installed-files.txt, which is what pip uses to track which files belong to a package.

This pull request fixes the issue by doing the following:

It also adds a max-retries option to the pip-accel configuration, allowing the user to limit the number of potentially costly attempts to build a package. This is important when building a lot of versions of different packages in batch, some of which may no longer be available.

coveralls commented 9 years ago

Coverage Status

Coverage decreased (-0.11%) to 87.12% when pulling b47792e957a55ba4793271a27748227347b2a32e on yhat:master into 808e3113c55e8f8aa336b10cd6a3f14c4d4a1329 on paylogic:master.

xolox commented 9 years ago

This pull request contained a bug report and several unrelated changes, one of which suggested an implementation that fixed the reported bug.

  1. I merged the part of this pull request that makes the maximum number of retries a configuration option because it makes sense and required little effort to apply (it's just that no one requested it until now). This was included in pip-accel 0.24 (e2f51e105c9b7d13fe1a784361f57786009222e1).
  2. I'm also confident that the bug you reported is now fixed, but I didn't use the implementation you suggested. Support for clean upgrades and pip uninstall was achieved with the following changes combined:
    • pip-accel 0.25 (0bf3c2202d23e36bd7eb2fa2d25bbaea2ef4fd3c) implemented initial support for tracking of installed files. The commit message explains in more detail why I didn't use the implementation you suggested in this pull request.
    • pip-accel 0.29 (6902027816b3ff81cc2f13b82db3f9d768b2ed31, published today) improves support for tracking of installed files by evaluating all setup.py scripts using setuptools (regardless of whether the setup.py script imports distutils or setuptools).
  3. I didn't do anything with 60a60c9a5b9632c9b74988b9c5dfb99493f974a5. If you want me to change this please open a separate issue describing your problem and in what environment you encounter the problem. For what it's worth, I've never seen a colon being problematic in a UNIX file system, and looking through the Docker issue tracker I can only find bugs in the Docker command line application docker treating colons awkwardly.

The bug reported in this pull request is now fixed so I'm going to close this pull request. In the future if you want to contribute, please consider using separate feature branches and pull requests for unrelated issues. It makes it much easier for me to discern related vs. unrelated changes and issues.

Thanks for the feedback!