Add an option to delete the cache when download completes.

[DailyDreaming]

Adds functionality to address: HumanCellAtlas/data-store#2064

In the future, we might consider setting the default to True for this, but in the meantime, this should be a useful switch for the needs of the dss.

Will add a test if it makes sense to add this.

[codecov-io]

Codecov Report

❗ No coverage uploaded for pull request base (master@a496e08). Click here to learn what that means.
The diff coverage is 88.23%.

@@            Coverage Diff            @@
##             master     #394   +/-   ##
=========================================
  Coverage          ?   85.36%           
=========================================
  Files             ?       39           
  Lines             ?     1866           
  Branches          ?        0           
=========================================
  Hits              ?     1593           
  Misses            ?      273           
  Partials          ?        0

Impacted Files	Coverage Δ
hca/dss/__init__.py	89.86% <88.23%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a496e08...bd24130. Read the comment docs.

[hannes-ucsc]

I will probably oppose ever making this default to True. My review comments add language that highlights the benefit of the file store and the implications of removing it.

I suggest that we decide if we want to use the term "cache" or "file store". I actually prefer "cache" but you would have to eliminate all mention of "file store" from both the code and the documentation. I think sticking with "file store" is easier. Whatever we do, we cannot afford to use inconsistent language.

[DailyDreaming]

@hannes-ucsc All comments should be addressed. I switched filestore to cache as well, since that is my preference as well.

[jessebrennan]

@DailyDreaming are you still making changes or is this good to review?

[jessebrennan]

I know there's been mixed opinions about this, but I'm starting to feel more strongly that we should use the name filestore instead of cache. Here's my reasoning:

For a cache, you typically expect that it can be cleared / deleted and the only impact will be upon performance in the future. In the case of our filestore, deleting it can have dire consequences for some kinds of downloads.

Specifically, consider what would happen if the download_manifest command is run with the delete_cache option set and layout=none. The files would be downloaded to the filestore, the manifest would be updated with paths to the files, then all of the files would be deleted and the paths would be broken. After hours of downloading, the user ends up with a bunch of broken paths. This is definitely not ever desirable behavior. Using the name cache gives users the false impression that it can be safely deleted, or that running with these options could even make sense.

The example above has also made me question adding the delete_cache feature. I think (at the very least) the name should be changed to delete_filestore, and if the feature is maintained, it should turn off the writing of the updated manifest or even be removed completely from the manifest download. Also, if it is kept, it should come with a stronger warning, explaining that bundles downloaded with this option will use double the disk space for redundant files, etc.

Maybe we should with @hannes-ucsc, all talk about with together in person.

[hannes-ucsc]

I hadn't thought about the case with --layout=none, that @jessebrennan brought up. --layout=none directly contradicts --delete-cache because using both would render a rewritten manifest with paths to non-existent files.

We could of course revert the renaming, ensure that --delete-filestore can't be used in conjunction with --layout=none by making them mutually exclusive.

I have a third option: we enforce the specification of a download directory instead of polluting the current working directory. If the user is forced to chose the name of a directory for the download, the user will then be more likely to regard that entire directory as an artifact of the download and therefore delete that directory instead of just the files in it. Deleting the directory would remove the hardlinks created for the selected layout, the rewritten manifest and the filestore.

[jessebrennan]

@hannes-ucsc I think your third option is a good idea and perhaps we should make a separate ticket about making the download directory a required argument. It's worth pointing out however, that the use case @DailyDreaming was originally trying to solve is not addressed by that change.