-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Do wheel installs from VCS/directories using ephemeral caching #4764
Changes from all commits
c624090
a32ebea
fe5e9cc
30109c3
ee67867
46c2c54
9fdcfaf
9109322
ad242d1
a51843a
73440ed
5610636
1c03c1b
ad50ac9
d045cc2
01d226b
d5e3082
1997456
328f607
8ef8400
cb56bb3
5f88252
ebf06ce
901f529
b96618b
165b10b
16cda30
1c6a450
3ec643c
d4e2d7e
e36bb6d
a39f36a
01d97e7
cd14240
9b6f1d7
4c14739
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
Installing from a local directory or a VCS URL now builds a wheel to install, | ||
rather than running ``setup.py install``. Wheels from these sources are not | ||
cached. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ | |
from pip._internal import index | ||
from pip._internal.compat import expanduser | ||
from pip._internal.download import path_to_url | ||
from pip._internal.utils.temp_dir import TempDirectory | ||
from pip._internal.wheel import InvalidWheelFilename, Wheel | ||
|
||
logger = logging.getLogger(__name__) | ||
|
@@ -102,13 +103,18 @@ def _link_for_candidate(self, link, candidate): | |
|
||
return index.Link(path_to_url(path)) | ||
|
||
def cleanup(self): | ||
pass | ||
|
||
class WheelCache(Cache): | ||
|
||
class SimpleWheelCache(Cache): | ||
"""A cache of wheels for future installs. | ||
""" | ||
|
||
def __init__(self, cache_dir, format_control): | ||
super(WheelCache, self).__init__(cache_dir, format_control, {"binary"}) | ||
super(SimpleWheelCache, self).__init__( | ||
cache_dir, format_control, {"binary"} | ||
) | ||
|
||
def get_path_for_link(self, link): | ||
"""Return a directory to store cached wheels for link | ||
|
@@ -127,8 +133,7 @@ def get_path_for_link(self, link): | |
""" | ||
parts = self._get_cache_path_parts(link) | ||
|
||
# Inside of the base location for cached wheels, expand our parts and | ||
# join them all together. | ||
# Store wheels within the root cache_dir | ||
return os.path.join(self.cache_dir, "wheels", *parts) | ||
|
||
def get(self, link, package_name): | ||
|
@@ -148,3 +153,50 @@ def get(self, link, package_name): | |
return link | ||
|
||
return self._link_for_candidate(link, min(candidates)[1]) | ||
|
||
|
||
class EphemWheelCache(SimpleWheelCache): | ||
"""A SimpleWheelCache that creates it's own temporary cache directory | ||
""" | ||
|
||
def __init__(self, format_control): | ||
self._temp_dir = TempDirectory(kind="ephem-wheel-cache") | ||
self._temp_dir.create() | ||
|
||
super(EphemWheelCache, self).__init__( | ||
self._temp_dir.path, format_control | ||
) | ||
|
||
def cleanup(self): | ||
self._temp_dir.cleanup() | ||
|
||
|
||
class WheelCache(Cache): | ||
"""Wraps EphemWheelCache and SimpleWheelCache into a single Cache | ||
|
||
This Cache allows for gracefully degradation, using the ephem wheel cache | ||
when a certain link is not found in the simple wheel cache first. | ||
""" | ||
|
||
def __init__(self, cache_dir, format_control): | ||
super(WheelCache, self).__init__( | ||
cache_dir, format_control, {'binary'} | ||
) | ||
self._wheel_cache = SimpleWheelCache(cache_dir, format_control) | ||
self._ephem_cache = EphemWheelCache(format_control) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be worth building the ephemeral wheel cache lazily? The majority of installs won't be installing from git or a local directory, so why waste time creating and deleting a temporary directory that we never use? This may be a premature optimisation, of course. Feel free to take the view that we can do this later if it proves to be a problem. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. $ python -c "
import time
import pip._internal.utils.temp_dir as tmp
t = tmp.TempDirectory()
print(time.time())
t.create()
t.cleanup()
print(time.time())"
1507128804.431749
1507128804.43445 At about 0.003 seconds, I don't think it's a major issue. Even if it's 10x as costly, I'm tempted to say it's not worth it to optimise. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, agreed. Thanks for testing this. I was concerned that Windows might be slower (not an uncommon problem...) but I ran your test script on my machine, and it's fine. I'm convinced - leave it as it is. |
||
|
||
def get_path_for_link(self, link): | ||
return self._wheel_cache.get_path_for_link(link) | ||
|
||
def get_ephem_path_for_link(self, link): | ||
return self._ephem_cache.get_path_for_link(link) | ||
|
||
def get(self, link, package_name): | ||
retval = self._wheel_cache.get(link, package_name) | ||
if retval is link: | ||
retval = self._ephem_cache.get(link, package_name) | ||
return retval | ||
|
||
def cleanup(self): | ||
self._wheel_cache.cleanup() | ||
self._ephem_cache.cleanup() |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -89,5 +89,8 @@ def run(self, options, args): | |
exclude_editable=options.exclude_editable, | ||
) | ||
|
||
for line in freeze(**freeze_kwargs): | ||
sys.stdout.write(line + '\n') | ||
try: | ||
for line in freeze(**freeze_kwargs): | ||
sys.stdout.write(line + '\n') | ||
finally: | ||
wheel_cache.cleanup() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Style nit. I'm not very keen on the fact that the try...finally is separated from the creation of the cache. Clearly there's no way the intermediate lines could fail, so in practice it's not a problem, but it looks odd (I went and looked at the source code to verify that the cleanup was in the right place). This is of course one place where caches being context managers would make things clearer - so I'm OK with leaving this for tidying up when we look at doing that (and I agree with your comment that making that change should be a separate PR). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should note that @xoviat had proposed in #4714 to make
Cache
s into context managers, to avoid using thiscleanup()
pattern.I'm still confused about how I feel about that. FWIW, we can do that in a later PR. I have some other ideas too but I'd prefer to experiment with that after this PR is done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rhettinger What do you think about using context managers for cleanup? You're the person that I got this from.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be clear, I definitely think using a context manager is a good idea. But I agree with @pradyunsg that it's something we should defer until a followup PR. Let's get the functionality landed then address the cleanup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with that; I was just wondering what others thought in principle.