ipfs / kubo

An IPFS implementation in Go
https://docs.ipfs.tech/how-to/command-line-quick-start/
Other
15.82k stars 2.96k forks source link

Pinning semantics slightly confusing. #590

Closed whyrusleeping closed 7 years ago

whyrusleeping commented 9 years ago

while working with pinning to test out the gc command, i ran into a few issues that revolved around understanding the pinning system, for example:

ipfs add afile                # adds and pins the file recursively
ipfs pin ls                     # wont show the recently added file
ipfs pin ls -type=all     #will show the file
ipfs pin rm <afile hash> # will say it unpinned the file
ipfs pin ls -type=all     # still shows the file
ipfs pin rm -r <afile hash> # actually unpins the file

I dont think its immediately apparent what the right thing to do to accomplish a given task is, the UX here needs some thought.

whyrusleeping commented 9 years ago

my thoughts, are that ipfs pin rm should remove both direct and recursive pins, but not indirect, and that ipfs pin ls should list direct and recursive pins, and say which they are to the side, like:

QmdHeAhfS5Ehv direct
QmelkjlSgsfgr recursive
QmAleFdFgRtmv direct
jbenet commented 9 years ago

my thoughts, are that ipfs pin rm should remove both direct and recursive pins, but not indirect

Agreed on the "but not indirect", but consider this user-misleading case:

# suppose h1 points to h2

# i mean to pin h2 and its siblings, which are in h1.
ipfs pin add -r <h1>   # pins h2 indirectly

# some time later, i want h1 itself.
ipfs pin add <h1>      # i forgot i already had pinned h1 recursively
ipfs pin rm <h1>       # oh nvm i dont want it anymore
# h2 now unpinned silently. oops.

What about something like this:

# if h1 is already pinned recursively:
> ipfs pin add <h1>
# idempotent no-op, h1 is already pinned.
> ipfs pin rm <h1>       # oh nvm i dont want it anymore
error: <h1> is pinned recursively, to remove recursive pins, use:
  ipfs pin rm -r <h1>

So -- unless im not considering something which is highly probable -- maybe effectively we can say that the user must use ipfs pin rm -r <path> to remove a recursive pin, and that attempting to remove a recursive pin without -r is always an error. (kind of like rm <dir> without -r is always an error)

that ipfs pin ls should list direct and recursive pins, and say which they are to the side,

Great, i like this. (and looking at the original description we discussed it then too: https://github.com/jbenet/go-ipfs/issues/172 )

whyrusleeping commented 9 years ago

Okay, so if h1 is pinned both direct and recursive, ipfs pin rm h1 would spit out an error, and ipfs pin rm -r h1 would remove both pins?

jbenet commented 9 years ago

im not certain this is good UX yet, but it addresses the confusion you outlined above. We've two pick between to models:

  1. direct and recursive pins are separate, and managed entirely separately.
  2. direct pins are superceeded by recursive pins

it may be worth evaluating whether -r should really be the default, and direct pins should be the deviation (ipfs pin add foo is equivalent to ipfs pin add -r foo, and ipfs pin add --direct foo is non-recursive). not set either way here.

whyrusleeping commented 9 years ago

I think im leaning towards recursive being the default

jbenet commented 9 years ago

it's probably what is meant most often. we shouldnt remove the -r option though, as we may have to switch this default in the future.

whyrusleeping commented 9 years ago

okay, do you want this on its own PR? ive been digging into it on my gc command branch since i need it for proper testing

jbenet commented 9 years ago

ideally own pr, for independent testing

jbenet commented 9 years ago

what i often do is rebase those commits to the beginning of my branch, spin them out as a separate branch and keep working (see bootstrap-fix in relation to addr-explosion)

whyrusleeping commented 9 years ago

The more i tinker with this, the more i think that you shouldnt be able to have something both Directly and Recursively pinned.

Example:

ifps pin add h1 #h1 pinned directly
ipfs pin add -r h1 #h1 now pinned recursively (ONLY) and subnodes pinned indirectly
ipfs pin rm h1 #h1 and children no longer pinned.
jbenet commented 9 years ago

This leads to the case described here: https://github.com/jbenet/go-ipfs/issues/590#issuecomment-70387682

mildred commented 9 years ago

What about this use case:

# suppose h1 points to h2

# i mean to pin h2 and its siblings, which are in h1.
ipfs pin add -r <h1>   # pins h2 indirectly

# some time later, i want h2 itself.
ipfs pin add <h2>      # i forgot i already had pinned h1 recursively
ipfs pin rm <h2>       # oh nvm i dont want it anymore

It should remove the direct pin on h2 but leave the indirect pin on h2 (because h1 is pinned recursively). h2 should stay around because of h1.

Now this one:

# suppose h1 points to h2 and h2 points to h3

# i mean to pin h2 and its siblings, which are in h1.
ipfs pin add -r <h1>   # pins h2 indirectly

# some time later, i want h2 itself (and its children).
ipfs pin add -r <h2>      # i forgot i already had pinned h1 recursively

# now knowing that h1 points to h2 (I only see the name of the link in my filesystem)
# I want to get rid of h1
ipfs pin rm -r <h1>

h2 should stay around because it was pinned recursively, even if the indirect pin inherited from h1 was removed. h3 should also stay around because its indirect pin inherited from h1 is removed, but its indirect pin inherited from h2 is still there.

whyrusleeping commented 9 years ago

It might be worth (eventually) having a flag for rm that allows demotion from a recursive pin to a direct pin.

anarcat commented 9 years ago

it doesn't help that ipfs pin --help doesn't actually explain what indirect, direct and whatnot means.

jbenet commented 9 years ago

@anarcat pinning:


It would be really helpful to get someone outside the core dev team to help us with the help docs. we need to strike a good balance between our biased way of thinking and how a newcomer would approach the concepts. who can help us with this?

anarcat commented 9 years ago

i threw a bunch of issues in - how can i help? :)

i this case, wouldn't a change in the help suffice?

jbenet commented 9 years ago

@anarcat yeah in this case a simple change to the help would... help. check it out here: https://github.com/jbenet/go-ipfs/blob/master/core/commands/pin.go

the commands are pretty ugly go code, but it has all the help text in one place

anarcat commented 9 years ago

re:

It would be really helpful to get someone outside the core dev team to help us with the help docs. we need to strike a good balance between our biased way of thinking and how a newcomer would approach the concepts. who can help us with this?

@jbenet i wonder if the current approach of having "everything in --help" will really scale in the long term. maybe having a set of docs on (say) readthedocs.org or opening up access to the main website to trusted contributors could help a lot. i also like the way git works: everything is in manpages - but this approach was somewhat discarded so far (#275). maybe a separate issue for docs refactoring should be opened?

jbenet commented 9 years ago

readthedocs.org

I want to have:

to the main website to trusted contributors

It's here: https://github.com/protocol/websites/tree/master/ipn.io/src/ipfs.io

maybe a separate issue for docs refactoring should be opened?

Yeah, i've been reluctant because i cant lead this effort atm, but happy to guide

zramsay commented 8 years ago

this issue tripped me up...i'll look into the codebase over the next week. Noob perspective should help too

jbenet commented 8 years ago

@zramsay yes please!

zramsay commented 8 years ago

So it took me awhile to pin a file successfully - because I didn't realize that add would pin recursively. Why is that by default ...what if a user wants to add a file without pinning it? (seems like that could often be the case).

For the API, if there's an error, the unmarshalled response.Body fits nicely in the Error struct. Otherwise, it's decoded into a []byte. It works fine, but leads to messy code, especially w/ an abstraction. e.g. I have a func PostAPICallToIPFS() that parses & deals with errors, but has to return the (success) response to the func that called it then parse it. It'd be nice to have a standard struct for all http responses; whether they are succesful or error.

jbenet commented 8 years ago

So it took me awhile to pin a file successfully

Curious if you saw the pinning example in http://ipfs.io/docs/examples ?

Why is that by default ...what if a user wants to add a file without pinning it? (seems like that could often be the case).

The expectation at the moment is for the common case of a user who knows almost nothing about how ipfs works under the hood-- that if "user adds X to ipfs" then "user expects X to be in ipfs later".

Maybe a --pin=false should be added?

For the API, if there's an error, the unmarshalled response.Body fits nicely in the Error struct. Otherwise, it's decoded into a []byte. It works fine, but leads to messy code, especially w/ an abstraction. e.g. I have a func PostAPICallToIPFS() that parses & deals with errors, but has to return the (success) response to the func that called it then parse it. It'd be nice to have a standard struct for all http responses; whether they are succesful or error.

Yeah... do you have a concrete code example we can look at together to fix the right UX?

zramsay commented 8 years ago

Curious if you saw the pinning example in http://ipfs.io/docs/examples?

Yeah when I finally came across it, everything made sense. Probably should've also spent more time playing with the cli beforehand.

The expectation at the moment is for the common case of a user who knows almost nothing about how ipfs works under the hood-- that if "user adds X to ipfs" then "user expects X to be in ipfs later".

Gotcha.

Maybe a --pin=false should be added?

That's a perfect task for me to get more familiar with the codebase.

do you have a concrete code example we can look at together to fix the right UX?

Yup func is called at line 80.

jbenet commented 8 years ago

That's a perfect task for me to get more familiar with the codebase.

great! :+1: (cc @whyrusleeping because this will need the lock stuff un the 0.4.0 branch)

whyrusleeping commented 8 years ago

nothing should change with respect to the locking stuff. we already lock around the add command, extra flags shouldnt change anything

jbenet commented 8 years ago

@whyrusleeping it may be added to the object command.

jbenet commented 8 years ago

btw, @zramsay have you seen https://github.com/whyrusleeping/ipfs-shell ? we're creating an API wrapper slowly. see it used here: https://github.com/whyrusleeping/pinbot

jbenet commented 8 years ago

@whyrusleeping (and @wking) may be time to hammer our ipfs-shell. shall we do it in that repo? @whyrusleeping want to move whyrusleeping/ipfs-shell to ipfs/go-ipfs-shell ?

zramsay commented 8 years ago

that wrapper is exactly what we need!

whyrusleeping commented 8 years ago

i could move it to ipfs/ipfs-shell

whyrusleeping commented 8 years ago

https://github.com/ipfs/ipfs-shell <- moved

whyrusleeping commented 8 years ago

@zramsay if you need any help with the ipfs-shell package, just let me know, and/or stop by irc :)

jbenet commented 8 years ago

Can you make it "go-ipfs-shell"? There will be other languages. These are basically bindings.

SCBuergel commented 8 years ago

Just some outsiders perspective on the pin/add issue: Without having seen your discussion here, I expected that ipfs add only adds stuff to ipfs but does not pin it. Otherwise, what is the usecase for ipfs pin add in the first place (if a file is anyway always added upon the initial ipfs add)?

mildred commented 8 years ago

@SCBuergel

ipfs pin add is useful if you want to bookmark a resource and participate in sharing it from your node. So there is a use case for it.

If you don't pin the object you just freshly added, what's to keep these objects from vanishing a few seconds later? Nobody is interested in these object as nobody knows their hash. As the objects are not pinned, they are only stored temporarily, and risk being thown out of your temporary cache.

SCBuergel commented 8 years ago

Hi @mildred, thanks for your explanation. But there's some confusion on my side now: I did ipfs add <file> and then ipfs pin add <hash> but got the following error message:

Error: pin: <hash> already pinned recursively

Therefore, I assume that ipfs add <file> already did the pinning (note that I did not use the file or hash anywhere else before) or how do I interpret this error message?

mildred commented 8 years ago

Therefore, I assume that ipfs add already did the pinning

I'd expect it to

jbenet commented 8 years ago

@SCBuergel we made a call early on that matched an admittedly small-sample, but strong indicator that:

we just haven't added the --pin flag :/. we should do that.

SCBuergel commented 8 years ago

Thanks, @jbenet. That default behavior was not initially clear to me.

daviddias commented 8 years ago

If we move to ipfs add to ipfs files add and ipfs object to ipfs dag (https://github.com/ipfs/go-ipfs/issues/257#issuecomment-168351470), it would be great (and way simpler to understand) to have 'dag pinning' under ipfs dag and let the ipfs files API do the pinning automatically for its files. This will help making the 'dag' vs 'files' distinction even better https://github.com/ipfs/go-ipfs/issues/575

whyrusleeping commented 7 years ago

Closing this, initial concerns have been addressed

IvanTurgenev commented 5 years ago

ipfs pin ls, still doesnt show my recently added folders

Stebalien commented 5 years ago

(~@ivan386~ sorry) @IvanTurgenev ipfs pin ls should list all pins and their children. Please open a new issue (and include how you're adding files to IPFS).