Skip to content
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

MRG: panic when FSStorage::load_sig encounters more than one Signature in a JSON record #3333

Open
wants to merge 50 commits into
base: latest
Choose a base branch
from

Conversation

ctb
Copy link
Contributor

@ctb ctb commented Sep 17, 2024

This PR was originally about debugging sourmash-bio/sourmash_plugin_branchwater#445, but that's going to require more work to fix properly. For now, I would like to nominate it for merge because sourmash fails silently in this situation, and that's Bad.

In brief, the main thing this PR does is panic with an unimplemented! when FSStorage::load_sig encounters more than one Signature in a JSON record.

This PR also adds a bit of documentation to InnerStorage, per the bottom of this comment.


The problem at hand: when loading a SigStore/Signature from a Storage, sourmash only loads the first one and ignores any others.

fn load_sig(&self, path: &str) -> Result<SigStore> {
let raw = self.load(path)?;
let sig = Signature::from_reader(&mut &raw[..])?
// TODO: select the right sig?
.swap_remove(0);

This results from the concept of a Signature as containing one or more sketches; the history of this is described here, and it leads to some interesting silliness in the Python layer.

The contrapositive is that, in Rust, a single Signature can include multiple sketches, e.g. with different ksizes. So this works fine for the wort case where we have a single .sig file with k=21, k=31, k51.

Note that the Python layer (and hence the entire sourmash CLI) fully supports multiple Signatures in JSON: this is well tested and well covered behavior. The branchwater plugin runs into it because it is using the Rust layer and the API is not fully fleshed out there.


Copy link

codecov bot commented Sep 17, 2024

Codecov Report

Attention: Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.

Project coverage is 86.47%. Comparing base (6ae9cd3) to head (abc01c1).

Files with missing lines Patch % Lines
src/core/src/storage/mod.rs 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           latest    #3333      +/-   ##
==========================================
- Coverage   86.47%   86.47%   -0.01%     
==========================================
  Files         137      137              
  Lines       16086    16090       +4     
  Branches     2219     2219              
==========================================
+ Hits        13911    13914       +3     
- Misses       1868     1869       +1     
  Partials      307      307              
Flag Coverage Δ
hypothesis-py 25.43% <ø> (ø)
python 92.40% <ø> (ø)
rust 62.27% <85.71%> (+0.01%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ctb ctb changed the title EXP: debug multisigfile loading WIP: explicitly note that loading more than one Signature from a .sig file is unimplemented currently Oct 27, 2024
@ctb ctb changed the title WIP: explicitly note that loading more than one Signature from a .sig file is unimplemented currently WIP: panic when FSStorage::load_sig encounters more than one Signature in a JSON record Oct 27, 2024
@ctb ctb added the rust label Oct 27, 2024
@ctb
Copy link
Contributor Author

ctb commented Oct 27, 2024

@luizirber @bluegenes thoughts welcome!

@ctb ctb changed the title WIP: panic when FSStorage::load_sig encounters more than one Signature in a JSON record MRG: panic when FSStorage::load_sig encounters more than one Signature in a JSON record Oct 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant