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: update to (prospective) sourmash r0.18.0 #548

Merged
merged 9 commits into from
Dec 21, 2024
Merged

MRG: update to (prospective) sourmash r0.18.0 #548

merged 9 commits into from
Dec 21, 2024

Conversation

ctb
Copy link
Collaborator

@ctb ctb commented Dec 18, 2024

This PR updates to sourmash core latest, which will be r0.18.0.

The biggest set of changes in the plugin relate to sourmash-bio/sourmash#3431, which made it possible to detect bad (and good) zip files based on content, and handle the error; in particular, this means that we can continue past errors from reading a file misnamed as a sig zip file, and try reading it in other ways. So a bunch of tests (e.g. test_fastgather.py::test_bad_against_3) no longer apply!

sourmash r0.18.0 also includes sourmash-bio/sourmash#3434, which changes the behavior of Signature::name() to return an Option if name is not set, rather than defaulting to either filename() or md5sum(), in that order. This caused a bug (see #538 (comment), split out into #550) where manifests would improperly disagree about whether two Records matched.

Fixes #550

TODO:

@ctb ctb changed the title WIP: update to (prospective) sourmash r0.18.0 MRG: update to (prospective) sourmash r0.18.0 Dec 21, 2024
@ctb
Copy link
Collaborator Author

ctb commented Dec 21, 2024

Ready for review & merge @bluegenes! But I will merge this w/o a review as needed in the interest of getting #531 into a release ;).

Copy link
Contributor

@bluegenes bluegenes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm

@bluegenes
Copy link
Contributor

We still have a test for improper zip file that can't be read, right?

@ctb
Copy link
Collaborator Author

ctb commented Dec 21, 2024

Uhh... actually, good question. Probably. What kind(s) of things are you thinking about? I could see -

  • zip file that is corrupted
  • zip file that doesn't have a manifest

other things?

@bluegenes
Copy link
Contributor

Yep -- I was mostly thinking about zipfiles that get interrupted while writing and thus can't be read - I guess this falls into your "corrupted" category. The error is now propagated from sourmash, so I'm guessing we just exit and say we can't load the sigs.

Just wanted to make sure we still have a test covering this.

@ctb ctb merged commit 992bf48 into main Dec 21, 2024
3 checks passed
@ctb ctb deleted the update_smash branch December 21, 2024 19:02
@ctb ctb mentioned this pull request Dec 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

intersect_manifest fails to match identical Records because of Signature::name() behavior
2 participants