MRG: panic when FSStorage::load_sig encounters more than one Signature in a JSON record
#3333
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ctb
changed the title
Signature from a .sig file is unimplemented currently
ctb
changed the title
Signature from a .sig file is unimplemented currentlyFSStorage::load_sig encounters more than one Signature in a JSON record
|
@luizirber @bluegenes thoughts welcome! |
ctb
changed the title
FSStorage::load_sig encounters more than one Signature in a JSON recordFSStorage::load_sig encounters more than one Signature in a JSON record
|
@luizirber bump! |
bluegenes
approved these changes
Nov 1, 2024
There was a problem hiding this comment.
This seems like a good plan to me. @luizirber, did you have any thoughts?
luizirber
approved these changes
Nov 4, 2024
|
🙏 |
ctb
added a commit
that referenced
this pull request
Nov 5, 2024
## [0.17.0] - 2024-11-05 Changes/additions: * standardize on u32 for scaled, and introduce `ScaledType` (#3364) * panic when `FSStorage::load_sig` encounters more than one `Signature` in a JSON record (#3333) Updates: * Bump needletail from 0.5.1 to 0.6.0 (#3376) * Bump histogram from 0.11.0 to 0.11.1 (#3377) * Bump serde from 1.0.210 to 1.0.214 (#3368) * Bump serde_json from 1.0.128 to 1.0.132 (#3358) * Fix clippy lints from 1.83 beta (#3357)
This was referenced Nov 6, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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!whenFSStorage::load_sigencounters more than oneSignaturein 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/Signaturefrom aStorage, sourmash only loads the first one and ignores any others.sourmash/src/core/src/storage/mod.rs
Lines 34 to 38 in 26b50f3
This results from the concept of a
Signatureas 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
Signaturecan include multiple sketches, e.g. with different ksizes. So this works fine for the wort case where we have a single.sigfile 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.