[MRG] Fix SBT duplicates bug

[ctb]

For Zip SBTs, signatures with identical md5sums were being saved correctly as different signatures, but the updated signature filenames resulting from duplicate md5sums were not being communicated back to the tree properly. This meant that the names of a number of signatures (but not their content) were being lost, triggering the behavior seen over in #1511 (comment) as well as a number of apparent duplicate signatures per #1171.

Fixes #1171.

TODO:

• add basic tests that trigger the problem!
• add tests where we see what happens if the SBT already exists ;)

[codecov]

Codecov Report

Merging #1568 (a52e1a7) into latest (01de852) will increase coverage by 0.17%.
The diff coverage is 92.42%.

@@            Coverage Diff             @@
##           latest    #1568      +/-   ##
==========================================
+ Coverage   80.96%   81.14%   +0.17%     
==========================================
  Files         102      102              
  Lines       10299    10322      +23     
  Branches     1165     1169       +4     
==========================================
+ Hits         8339     8376      +37     
+ Misses       1751     1741      -10     
+ Partials      209      205       -4     

Flag	Coverage Δ
python	89.27% <92.42%> (+0.24%)	⬆️
rust	66.59% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/sourmash/sbt_storage.py	89.90% <91.83%> (+7.00%)	⬆️
src/sourmash/sbt.py	81.04% <94.11%> (+0.25%)	⬆️
src/sourmash/sourmash_args.py	94.24% <0.00%> (+0.02%)	⬆️
src/sourmash/lca/lca_utils.py	87.90% <0.00%> (+0.09%)	⬆️
src/sourmash/utils.py	78.94% <0.00%> (+1.75%)	⬆️

---

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 01de852...a52e1a7. Read the comment docs.

[ctb]

The tests are failing because of the need to support --append for zip SBTs; the revised code is saving the index as .sbt.json_0. I am thinking of adding a new function to SBT storage that is save_exact, which will replace the contents of the file if needed without generating a new filename.

[ctb]

OK, @luizirber, I think this is ready for a look.

[ctb]

@luizirber there's one failing test under python 3.9 on linux - ipfsstorage. I'm unclear on whether it's something I broke, tho.

[luizirber]

@luizirber there's one failing test under python 3.9 on linux - ipfsstorage. I'm unclear on whether it's something I broke, tho.

It's something that was broken: list_sbts is not implemented for IPFSStorage (but wasn't tested/checked/needed before, because IPFS-backed SBTs usually are a single JSON anyway)
https://github.com/dib-lab/sourmash/pull/1568/checks?check_run_id=2748963142#step:12:135

(actually implementing it is interesting because it leads to using an IPFS address as we use .zip files nowadays, hmm...)

[ctb]

@luizirber there's one failing test under python 3.9 on linux - ipfsstorage. I'm unclear on whether it's something I broke, tho.

It's something that was broken: list_sbts is not implemented for IPFSStorage (but wasn't tested/checked/needed before, because IPFS-backed SBTs usually are a single JSON anyway)
https://github.com/dib-lab/sourmash/pull/1568/checks?check_run_id=2748963142#step:12:135

(actually implementing it is interesting because it leads to using an IPFS address as we use .zip files nowadays, hmm...)

OK. Any thoughts on what to do about it, or why it's acting up now? I can dig into it later, just wondering if you know off the top of your head.

[ctb]

yay w00t! fixed!

[ctb]

(ready for review and merge @luizirber)

[luizirber]

OK. Any thoughts on what to do about it, or why it's acting up now? I can dig into it later, just wondering if you know off the top of your head.

Trying to think a bit conceptually about what is happening when saving/loading:

• a location is always required. It can be a JSON file (with the SBT description, for FSStorage and IPFSStorage) or a Zip file containing the SBT JSON (for ZipStorage).
• storage is not required, but can be passed to replace the storage defined in the location. For example, we can use an SBT with a FSStorage in location, but actually use a ZipStorage to load/save data.

Part of the confusion in using this API is that we mostly depend on the extension of the location to decide what to do. I'll try to come up with a table later to map behaviors...

[ctb]

OK. Any thoughts on what to do about it, or why it's acting up now? I can dig into it later, just wondering if you know off the top of your head.

Trying to think a bit conceptually about what is happening when saving/loading:

* a `location` is always required. It can be a JSON file (with the SBT description, for `FSStorage` and `IPFSStorage`) or a Zip file containing the SBT JSON (for `ZipStorage`).

* `storage` is not required, but can be passed to **replace** the storage defined in the `location`. For example, we can use an SBT with a `FSStorage` in `location`, but actually use a `ZipStorage` to load/save data.

Part of the confusion in using this API is that we mostly depend on the extension of the location to decide what to do. I'll try to come up with a table later to map behaviors...

Right - and I have a much better understanding of the code's behavior now 😄 , which is nice!