Add cache command #3968

tdsmith · 2016-09-14T22:59:49Z

Adds an option that prints the configured location of the cache directory and
exits. This is useful if users want to know where the cache is so that they can
delete stale or poisoned wheels.

This change is

dstufft

I don't like the use of a flag for this. I think it'd be better off being a command of some sort. I'm not sure what exactly that command would look like though. Off the top of my head I can think of two different ways of going with this.

We make a dedicated pip cache command that allows people to interact with the cache to do things like see the location, clear the cache, etc.
We make a dedicated pip config command that allows people to set and read configuration values, so that they can see what the cache dir, and other settings would be and also modify them via a command.

tdsmith · 2016-09-15T21:41:25Z

I kind of like the idea of pip cache list, pip cache erase, pip cache location...

dstufft · 2016-09-15T22:20:56Z

Yea that could be real good. Wouldn't need to start off with everything either. I think pip cache erase and pip cache location could be two real good ones. Or maybe pip cache erase and pip cache info which shows things like location and sizes and such.

dstufft · 2016-09-15T22:25:23Z

Related issues: #2819, #3734, #2851

pfmoore · 2016-09-15T22:27:20Z

I kind of like the idea of pip cache list, pip cache erase, pip cache location

+1 from me, too. That sounds like the right way to expose this type of functionality.

tdsmith · 2016-09-16T08:34:53Z

Added a cache location command.

I see this reinvents part of #3734; any feelings about what to do there?

pfmoore · 2016-09-16T09:31:30Z

There's a PEP8 issue, and I've just restarted the python 3.6 nightly run, as the failures there seem unrelated. You should probably also have a docs page for the new command. But otherwise it's looking OK to me.

Regarding #3734 let's see what @xavfernandez thinks. I'm inclined to think that this PR is a nice quick win, and the extra functionality from #3734 could be added later if needed - but if #3734 is close to merging, it might be easier to get the whole lot at once.

dstufft

This is looking a lot better! Two small comments and one major one.

dstufft · 2016-09-16T15:59:52Z

pip/commands/cache.py

+
+
+class CacheCommand(Command):
+    """\


What's the trailing slash here for?

Better auto-dedenting behavior; IIRC if the first line of the block quote doesn't start with whitespace the rest of the docstring won't be dedented.

dstufft · 2016-09-16T16:00:58Z

pip/commands/cache.py

+                "ERROR: Please provide one of these subcommands: %s" %
+                ", ".join(self.actions))
+            return ERROR
+        method = self.__getattribute__("action_%s" % args[0])


This would be better written as method = getattr(self, "action_%s" % args[0]).

dstufft · 2016-09-16T16:10:37Z

pip/commands/cache.py

+        return method(options, args[1:])
+
+    def action_location(self, options, args):
+        logger.info(os.path.join(options.cache_dir, "wheels"))


We probably want this to be a bit more structured yea? I imagine something that outputs something more like:

Cache Locations: HTTP = /Users/dstufft/Library/Caches/pip/http Wheels = /Users/dstufft/Library/Caches/pip/wheels

Or something like that? Maybe It'd be better to switch pip cache locations for pip cache info and have somehting like:

HTTP Cache: Location = /Users/dstufft/Library/Caches/pip/http Size = 284M Items = 1045 Wheel Cache Location = /Users/dstufft/Library/Caches/pip/wheels Size = 95M Items = 391

I guess the key point here is that we currently have at least two caches, might add more in the future and arbitrarily picking one of them to return as the location feels like the wrong thing to do so we should at least support something that returns them all. Adding more information like Size and number of items might be interesting too to give people an idea of the scope of their caches.

I'm hoping this is the output from cache info. :) I'd like $(pip cache location) to be possible. Refraining from adding /wheels is probably wise.

What are you hoping to do with $(pip cache location)?

I ask because I'm trying to think of a use case where you don't need to be able to explicitly ask for http versus wheel cache OR bake in an assumption about the names of those caches relative to the overall cache directory that isn't rm -rf $(pip cache location) or du -h -d 0 $(pip cache location).

I see your point; I think it follows that any verb that operates on a cache will have to understand which cache it wants to operate on, right?

Is it worth exposing anything about the http cache? It's less mysterious than the wheel cache; I haven't seen people having trouble with it. Should cache just deal with the wheel cache?

From the name I'd expect to be able to deal with any of pip's caches with pip cache. Maybe we could add a --type [wheel|http] with the default being wheel. That's explicit, and clearly documents that you get wheel by default, but maybe it's a bit over-engineered?

Yea I definitely think everything should encompass all of the caches we have. The reason I was asking what your use case was for that command is trying to determine if it makes sense to have pip cache location return the top level cache directory, or if it made sense to have flags to return specific cache directories or if it made sense to just omit the command all together and roll whatever specific functionality you were looking at using that command for into it's own command (e.g. if you were looking to do rm -rf $(pip cache location) then just add pip cache clear).

However, I think the way to do it, assuming we can't just roll whatever functionality up, is maybe by default have it return the top level cache directory, and then add flags for --wheel and --http (or --type [wheel|http]) that let you scope that down to a specific type instead of the entire thing. Could easily apply this pattern to other commands like pip cache info (only show info for wheel or http) and pip cache clear (only clear one type).

Does it make sense to make this an option on the cache command or on each subcommand? Similarly, I guess, does a pip cache list command make sense for the http cache?

tdsmith · 2016-09-17T20:17:31Z

I added an implementation of an info command; let me know if you'd rather handle that in a separate PR.

Output looks like:

(tmp-f5f9d55100f33573) tim@rocketman:pip (show-cache-dir)$ pip cache --type=all info
HTTP cache info:
   Location: /Users/tim/Library/Caches/pip/http
   Number of files: 1155
   Size: 677.3 MiB

Wheel cache info:
   Location: /Users/tim/Library/Caches/pip/wheels
   Number of files: 281
   Size: 55.6 MiB

tdsmith · 2016-09-17T20:35:44Z

pip/utils/filesystem.py

+    return result
+
+
+def human_size(n_bytes):


Oops, duplicates pip.utils.format_size; should use that instead.

Ivoz · 2016-09-19T01:05:28Z

A) info and location are practically duplications of eachother. I understand the argument would be "I want the non-hassle of being able to use $(pip cache location) without having to filter cache info." Firstly, if functionality of pip cache clear is indeed intended to be implemented, that's mostly a non-argument in the first place; secondly, if the need to clear the cache is so often that this slight convenience becomes a real time-saver, then I'd suggest there would be other behaviour in pip towards how to manage its cache that needs to be fixed first (so it's not causing the user such problems in the first place), not to make it easy for someone to delete on the command-line in one-liners. Therefore I think only the command equivalent to pip cache info is necessary to be implemented here.

B) I'd suggest that Files: <number> is abundantly clear as to the meaning of the information, so Number of... seems verbose to me.

C) If given my first point, then I'd suggest that rather than implementing a whole command this could fit neatly under the existing informational command in pip, pip show. E.G, pip show --cache. If we had clearly decided there needs to be other commands for cache functionality, other than one to show the cache and one to delete it, such that grouping them under a sub command would definitely be necessary I'd concede this point.

I make most of these comments, under a vest of trying to keep feature-creep out of pip's existing UI. Not that I want to see anything added ever, but always very solid reasons when done so.

tdsmith · 2016-09-19T01:22:28Z

wrt pip cache location, the other use I can think of is commands of the form find $(pip cache location)....

Sort of in that vein, I think pip cache list (list cached artifacts) and pip cache clear should exist; pip cache rm seems plausible.

dstufft · 2016-09-19T12:27:46Z

One thing to be careful of is anything that bakes in an assumption about how our caches are laid out internally.

For example, right now the wheel cache you can kind of figure out what's cached just by doing an exhaustive search of all of the filenames. However that is not currently true for the HTTP cache where filenames are sha256 (IIRC?) hashes of the URLs they are caching. For these, it's not currently possible to see what items are currently cached, only to ask "is this particular URL cached?". While adding a dedicated location command doesn't on it's own tell people that hey you can depend on the structure of our cache, it feels like it's paving a cowpath towards people doing just that (outside of cases like rm -rf $(pip cache location)).

Given that, I think that it probably makes sense to not implement pip cache location unless we can come up with a use case that isn't going to mandate baking in assumptions about the "schema" of our caches themselves and instead bake that logic into pip itself where we can update it as/when we change the cache schema.

I think that pip cache [info|clear|rm] seem reasonable to me (and if we do that, we probably don't need --type flags I think? It seems OK to just have all of those three commands operate on everything. I'm less sure about pip cache list, spewing hundreds or thousands of lines of text onto the screen doesn't seem as useful to me as the other commands. (To be clear, I don't think you need to implement all of the above in this PR, but if you want to you can!).

Does that all sound reasonable?

pfmoore · 2016-09-19T13:26:20Z

On reflection, I also think we should strongly enforce YAGNI here. ISTM that we've ended up at a point where we're implementing a feature without exactly remembering why we wanted it. As @dstufft said, the internals of the cache are undocumented and private, so we should be very clear on intended use cases before implementing features, as otherwise there's a risk that we open up the internals in a way that means people end up relying on them. Sorry if this feels like backing out of what seemed like an almost-complete PR, but sometimes it takes getting to this sort of stage before the implications are really clear.

For example, re-reading the the original use case, it was "This is useful if users want to know where the cache is so that they can delete stale or poisoned wheels." Is the best way to clear out invalid wheels from the cache to let people locate the cache and go hunting for the relevant wheels themselves? If there's a need to delete wheels would an explicit command not be better? Something like pip cache clear-wheel <requirement>? Then we can keep the cache structure private.

There's also a reasonable use case for "blow away the whole cache" - something like pip cache clear. Maybe we could allow people to choose HTTP, wheel or both, but I don't know why anyone would need to specify to that level.

But what other use case is there for "tell me where the cache is"? Or indeed for any other cache related commands?

dstufft · 2016-09-20T11:40:03Z

Looking good!

I'm still on the fence about the cache list command. I grok that its goal is to enable figuring out what arguments need passed to cache rm, but I wonder if it doesn't just make sense to just make the argument to cache rm be a project name and be done with it. That feels like a simpler UI for end users to me (at the cost of maybe wider than needed cache removal) especially if we ensure that pip cache rm <thing that isn't cache> works successfully (making rm essentially "ensure this isn't cached"). This also seems like it'll be less error prone as end users don't need to worry about interpreting wheel filenames to figure out which wheel they need to cache.

I think it makes sense for the list and rm commands to not operate on the HTTP cache since that should (ideally) be entirely transparent to end users whereas (as you identified) the wheel cache is a bit different as there is a chance of it going stale if the state of the machine has changed.

I kind of don't like the --type flag though. Would it be terrible if we just made info show both cache types always, and just made list and rm only operate on the wheel cache?

RonnyPfannschmidt · 2016-09-20T12:39:56Z

what about using cache discard instead of cache rm as name (similar to the difference between set.remove and set.discard)

tdsmith · 2016-09-21T01:50:22Z

I wonder if it doesn't just make sense to just make the argument to cache rm be a project name and be done with it

This might be a bridge too far, but what if it's treated like a glob if it contains a metacharacter or ends in .whl and otherwise it's treated like a project name?

I kind of don't like the --type flag though. Would it be terrible if we just made info show both cache types always, and just made list and rm only operate on the wheel cache?

I wouldn't be mad. Thoughts about purge?

dstufft · 2016-09-21T02:01:23Z

I wonder if it doesn't just make sense to just make the argument to cache rm be a project name and be done with it

This might be a bridge too far, but what if it's treated like a glob if it contains a metacharacter or ends in .whl and otherwise it's treated like a project name?

My thinking about restricting it to just project names also means it's not tied to wheel caching either. If we rejigger the HTTP cache to tag cached responses with a project name, we can make it just work with that. If we add caching of VCS repositories, we can make it just work with that as well. By keeping it higher level we eliminate some amounts of tying it to a specific implementation. Of course the flipside of that is with your proposed change, that is still somewhat true, just that glob characters/.whl would restrict it to just deleting stuff from the wheel cache in my hypothetical above.

Overall, it feels somewhat magical to do that and I'm trying to think of a use case where it is vitally important to just delete a specific wheel from the cache and no others. Worst case scenario it seems like it'd just trigger the slow path again (once) but also probably save some space by cleaning up old wheels for versions you're no longer installing?

I kind of don't like the --type flag though. Would it be terrible if we just made info show both cache types always, and just made list and rm only operate on the wheel cache?

I wouldn't be mad. Thoughts about purge?

I'd make it act like info, always operate on both caches. If someone comes along and really wants to only purge one or the other we can look at adding it back then.

As an aside, I don't really like the discard verb that @RonnyPfannschmidt suggested (idk why, just feels harder) but maybe it'd make sense to spell out the entire word "remove", so it'd be pip cache remove <whatever>.

tdsmith · 2016-09-21T04:03:31Z

If you want to extend this to cover arbitrary cache types I'm wondering if it makes sense to retain --type since the different caches aren't necessarily related. I like it for purge too since the reasons you'd want to delete the http cache (it got too big) are different from the reasons you'd want to delete the wheel cache (something was stale).

I almost wonder if --type should be mandatory instead of defaulting to wheel, in that case!?

remove sounds fine!

xavfernandez · 2016-09-22T14:51:15Z

Regarding #3734 let's see what @xavfernandez thinks. I'm inclined to think that this PR is a nice quick win, and the extra functionality from #3734 could be added later if needed - but if #3734 is close to merging, it might be easier to get the whole lot at once.

Hello, I was in holidays :)
Regarding #3734, it was on hold since it did not seem to attract any enthusiasm from anyone and I wouldn't want to add unused commands to pip. So I'm glad the idea has gained traction but disappointed that it was reimplemented from scratch (but I guess that's the fate of pip with its 60+ open PRs).

Note that https://github.com/pypa/pip/pull/3734/files#diff-2695f32c4432acd141c3dbe7e7e3a6b0R803 was also added to track the origin of the wheel cache.

BrownTruck · 2017-05-14T05:00:11Z

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will eligible for code review and hopefully merging!

pradyunsg · 2017-05-16T04:12:25Z

@tdsmith @xavfernandez Is one of you going to look into this in the near future?

xavfernandez · 2017-05-31T21:42:12Z

@pradyunsg Well, seeing your comment, I rebased #3734 which still works fine but which contained less functionnality than @tdsmith's PR.

I won't have much time to work on it though.

pradyunsg · 2017-06-17T09:33:47Z

@tdsmith May I take this forward?

pradyunsg · 2017-08-21T10:09:52Z

Closing in favour of #4685.

dstufft requested changes Sep 15, 2016

View reviewed changes

tdsmith force-pushed the show-cache-dir branch from 9358a45 to 89778fa Compare September 16, 2016 08:29

tdsmith changed the title ~~Add --print-cache-dir option~~ Add cache command Sep 16, 2016

tdsmith force-pushed the show-cache-dir branch from 89778fa to a586fe4 Compare September 16, 2016 15:51

dstufft requested changes Sep 16, 2016

View reviewed changes

tdsmith commented Sep 17, 2016

View reviewed changes

dstufft closed this Sep 18, 2016

dstufft reopened this Sep 18, 2016

tdsmith added 7 commits September 18, 2016 11:04

Implement pip cache command.

f477a6d

Add test for cache location command

bcf058d

Document cache command

40411df

Use getattr()

ce3891d

Allow the type of cache to be specified

3d1aefe

Test that cache types are constrained

75b383a

Implement cache info command

7dd3422

tdsmith force-pushed the show-cache-dir branch from 0247b30 to 7dd3422 Compare September 18, 2016 18:04

tdsmith added 2 commits September 18, 2016 13:41

don't duplicate format_size

ae6a0ed

Test cache info

baea58f

RonnyPfannschmidt mentioned this pull request Sep 19, 2016

Implement rwt as pip run #3979

Closed

tdsmith added 3 commits September 20, 2016 00:03

Implement pip cache purge

65e992e

Doc tweak

a90dfea

Nit

2906389

tdsmith added 3 commits September 20, 2016 19:44

Allow pip cache rm to accept a package name

73e09fa

pip cache list should return SUCCESS

c25cdda

Factor out filename matching logic

05a953a

tdsmith force-pushed the show-cache-dir branch from cd53d04 to 05a953a Compare September 21, 2016 02:44

Rename rm to remove

9315cb6

dstufft mentioned this pull request Dec 5, 2016

Print cache_dir value in help. #4159

Closed

fix test

a11f61a

tdsmith force-pushed the show-cache-dir branch from 269a505 to a11f61a Compare December 5, 2016 21:18

BrownTruck added the needs rebase or merge PR has conflicts with current master label May 14, 2017

pradyunsg mentioned this pull request Aug 21, 2017

Add a command for managing pip's cache #4685

Closed

pradyunsg closed this Aug 21, 2017

pradyunsg mentioned this pull request Nov 2, 2017

How to clear pip cache safely? #4830

Closed

duckinator mentioned this pull request Apr 8, 2019

Add 'pip cache' command #6391

Merged

10 tasks

lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 2, 2019

lock bot locked as resolved and limited conversation to collaborators Jun 2, 2019

pradyunsg removed the needs rebase or merge PR has conflicts with current master label Apr 2, 2021

Add cache command #3968

Add cache command #3968

Conversation

tdsmith commented Sep 14, 2016 • edited by dstufft Loading

dstufft left a comment

Choose a reason for hiding this comment

tdsmith commented Sep 15, 2016

dstufft commented Sep 15, 2016

dstufft commented Sep 15, 2016

pfmoore commented Sep 15, 2016

tdsmith commented Sep 16, 2016

pfmoore commented Sep 16, 2016

dstufft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dstufft Sep 16, 2016 • edited Loading

Choose a reason for hiding this comment

tdsmith Sep 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdsmith commented Sep 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivoz commented Sep 19, 2016 • edited Loading

tdsmith commented Sep 19, 2016

dstufft commented Sep 19, 2016

pfmoore commented Sep 19, 2016

dstufft commented Sep 20, 2016

RonnyPfannschmidt commented Sep 20, 2016

tdsmith commented Sep 21, 2016

dstufft commented Sep 21, 2016

tdsmith commented Sep 21, 2016

xavfernandez commented Sep 22, 2016

BrownTruck commented May 14, 2017

pradyunsg commented May 16, 2017

xavfernandez commented May 31, 2017

pradyunsg commented Jun 17, 2017

pradyunsg commented Aug 21, 2017

tdsmith commented Sep 14, 2016 •

edited by dstufft

Loading

dstufft Sep 16, 2016 •

edited

Loading

tdsmith Sep 16, 2016 •

edited

Loading

Ivoz commented Sep 19, 2016 •

edited

Loading