MRG: add `Manifest::intersect_manifest` to Rust core #3305

ctb · 2024-08-21T13:40:06Z

This PR implements Manifest::intersect_manifest and Collection::intersect_manifest for the Rust layer, which is needed to support standalone manifests over in sourmash-bio/sourmash_plugin_branchwater#430.

As part of this, the PR implements Eq and Hash traits for Record so that HashSet can be used for efficient intersections.

Related PRs:

WIP: add generic support for any type of sketch collection as query or database sourmash_plugin_branchwater#430

codecov · 2024-08-21T13:52:43Z

Codecov Report

Attention: Patch coverage is 70.83333% with 7 lines in your changes missing coverage. Please review.

Project coverage is 86.49%. Comparing base (26b50f3) to head (aa109f8).
Report is 1 commits behind head on latest.

Files with missing lines	Patch %	Lines
src/core/src/manifest.rs	72.72%	6 Missing ⚠️
src/core/src/collection.rs	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           latest    #3305      +/-   ##
==========================================
- Coverage   86.53%   86.49%   -0.04%     
==========================================
  Files         137      137              
  Lines       16023    16047      +24     
  Branches     2728     2728              
==========================================
+ Hits        13865    13880      +15     
- Misses       1849     1858       +9     
  Partials      309      309

Flag	Coverage Δ
hypothesis-py	`25.40% <ø> (ø)`
python	`92.37% <ø> (ø)`
rust	`62.30% <70.83%> (+<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.

This reverts commit 53bcf02.

This reverts commit d467d9f.

src/core/src/collection.rs

Extracted from #3305 This PR adjusts `RevIndex::open` to propagate a `RocksDBError` when opening a non-RocksDB directory. It also modifies the creation printout in `RevIndex::create` to distinguish index completion from interim output, for slightly easier debugging.

…_unwrap

ctb · 2024-09-16T01:08:56Z

@luizirber @bluegenes ready for review!

…_unwrap

bluegenes · 2024-09-17T16:37:14Z

src/core/src/collection.rs

@@ -215,6 +215,14 @@ impl Collection {
        assert_eq!(sig.signatures.len(), 1);
        Ok(sig)
    }
+
+    pub fn intersect_manifest(&self, mf: &Manifest) -> Self {


would it be simpler (or complementary) to enable intersect of the collections directly i.e. pass a ref to the Collection (other) and then use self.manifest.intersect_manifest(other.manifest)?

I guess it probably doesn't matter as long as we're passing in a reference to either the collection or manifest.

this comes directly from the MultiCollection code use case, where we read a manifest in directly; would be frustrating to have to create a Collection first.

we might be able to coerce a Collection into a Manifest, though, which would make it work more smoothly when you do have a Collection... I'll look!

bluegenes · 2024-09-17T16:41:05Z

src/core/src/collection.rs

+        let manifest = self.manifest.intersect_manifest(mf);
+        Self {
+            manifest,
+            storage: self.storage.clone(),


does just cloning the storage mean that we're carrying around all sigs anyway, but we just don't have access to them via the manifest?

This is probably best since we avoid having to find and copy sigs individually?

I think this is really a question for @luizirber at the moment. The alternative would involve references and lifetime annotations, I think.

does just cloning the storage mean that we're carrying around all sigs anyway, but we just don't have access to them via the manifest?

This is probably best since we avoid having to find and copy sigs individually?

To be clear: I think the answer is yes, esp if it's e.g. a MemStorage.

I am worried about duplicating memory unnecessarily. I don't really know how cloning a storage works in practice. I don't like the idea of cloning (which presumably would double the MemStorage).

Maybe a better approach here would be to have the result of Collection::intersect_manifest take ownership of storage without a clone. I'd need to change the signature of intersect_manifest here, will think on't.

addressed in 9b9e17e and f4e89f4!

MemStorage is the odd one, because it does allocate a lot of new memory, but Storage in general doesn't store much (or any) data, mostly metadata to access the actual data.

ctb · 2024-09-18T13:46:05Z

@luizirber any strong objections to the code here?

ctb · 2024-09-20T16:31:36Z

@luizirber @bluegenes can we merge this? :)

ctb · 2024-09-20T16:31:50Z

(not sure why codecov is acting up... tests do cover that code?!)

bluegenes · 2024-09-21T02:09:13Z

I'm ok with merging 🤷🏼‍♀️

luizirber · 2024-09-21T02:16:45Z

(not sure why codecov is acting up... tests do cover that code?!)

Sometimes cargo-tarpaulin doesn't get everything, I've tried #2993 some time ago, maybe time to revisit

ctb · 2024-09-21T16:29:07Z

🎉

from #2987 (comment): - [x] verify minimum supported rust version (MSRV) for writing release notes (see #2988 for an example); MSRV is checked by CI in `.github/workflows/rust.yml`, `minimum_rust_version` - [x] write release notes using `git log --oneline r0.12.0..latest src/core | cut -d\ -f2- > /tmp/out.txt` - [x] verify that the ChangeLog is up to date: https://github.com/sourmash-bio/sourmash/blob/latest/src/core/CHANGELOG.md - [x] bump version in `src/core/Cargo.toml` # Release notes: ## [0.15.2] - 2024-09-25 MSRV: 1.65 Changes/additions: * add `Manifest::intersect_manifest` to Rust core (#3305) * propagate error from `RocksDB::open` on bad directory (#3306, #3307) Updates: * Bump getset from 0.1.2 to 0.1.3 (#3328) * Bump memmap2 from 0.9.4 to 0.9.5 (#3326) * Bump codspeed-criterion-compat from 2.6.0 to 2.7.2 (#3324) * Bump serde_json from 1.0.127 to 1.0.128 (#3316) * Bump serde from 1.0.209 to 1.0.210 (#3318) * Bump serde from 1.0.208 to 1.0.209 (#3310) * Bump serde_json from 1.0.125 to 1.0.127 (#3309)

make error catchable

6b1d1bb

ctb added 5 commits August 21, 2024 07:03

flag another problematic unwrap

7e73e8f

more

2896c49

more

45b1a8f

upd log message

e8d7999

provide correct error

53bcf02

ctb changed the title ~~EXP: propagate error from opening invalid RocksDB database~~ WIP: RevIndex creation and access minor code updates Aug 21, 2024

ctb added 10 commits August 22, 2024 08:28

switch Manifest from paths to TryFrom

d467d9f

Revert "provide correct error"

5f1eef6

This reverts commit 53bcf02.

Revert "switch Manifest from paths to TryFrom"

eb46ecd

This reverts commit d467d9f.

poor person's picklist?

923af44

add picklist select

315dfff

ok

1fb658a

picklist by ref

a122982

add manifest.is_empty()

39816ab

update revindex indexing message

61416fb

propagate error on bad directory when opening RocksDB

3a7abe9

ctb mentioned this pull request Aug 24, 2024

MRG: propagate error from RocksDB::open on bad directory #3306

Merged

ctb added 3 commits August 24, 2024 06:47

Merge branch 'update_revindex' into remove_unwrap

07e3a09

do we not need len?

fe75c6c

nope, don't need em

5fb20fc

ctb changed the base branch from latest to update_revindex August 24, 2024 14:10

revert for now

2c59010

ctb changed the title ~~WIP: RevIndex creation and access minor code updates~~ EXP: trial code updates for branchwater Aug 24, 2024

luizirber reviewed Aug 24, 2024

View reviewed changes

src/core/src/collection.rs Outdated Show resolved Hide resolved

Base automatically changed from update_revindex to latest August 26, 2024 18:46

Merge branch 'latest' of github.com:sourmash-bio/sourmash into remove…

c8477df

…_unwrap

ctb mentioned this pull request Aug 27, 2024

WIP: add generic support for any type of sketch collection as query or database sourmash-bio/sourmash_plugin_branchwater#430

Open

28 tasks

round out comparison & hashing for Record

32839ae

ctb changed the title ~~EXP: trial code updates for branchwater~~ MRG: add Manifest::intersect_manifest to Rust core Sep 16, 2024

ctb added 8 commits September 15, 2024 18:18

cargo fmt

43ee757

Merge branch 'latest' into remove_unwrap

31aa378

remove identity closure

1594dc4

add in a print for debugging

2465c02

more print

5af5f45

remove prints

b46565d

Merge branch 'latest' of github.com:sourmash-bio/sourmash into remove…

b7a9850

…_unwrap

add a test for Collection::intersect_manifest

9c26752

ctb mentioned this pull request Sep 17, 2024

EXP: debug multisigfile loading #3333

Open

cargo fmt

f2fde5d

bluegenes reviewed Sep 17, 2024

View reviewed changes

ctb added 3 commits September 18, 2024 05:41

modify intersect_manifest signature

f4e89f4

mut Collection for intersect_manifest

9b9e17e

omit unit return type

aa109f8

luizirber approved these changes Sep 21, 2024

View reviewed changes

ctb merged commit ada039a into latest Sep 21, 2024
42 of 46 checks passed

ctb deleted the remove_unwrap branch September 21, 2024 16:29

ctb added a commit to sourmash-bio/sourmash_plugin_branchwater that referenced this pull request Sep 21, 2024

update after merge of sourmash-bio/sourmash#3305

780fbda

ctb mentioned this pull request Sep 25, 2024

MRG: update sourmash core to r0.15.2 #3338

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MRG: add `Manifest::intersect_manifest` to Rust core #3305

MRG: add `Manifest::intersect_manifest` to Rust core #3305

ctb commented Aug 21, 2024 •

edited

Loading

codecov bot commented Aug 21, 2024 •

edited

Loading

ctb commented Sep 16, 2024

bluegenes Sep 17, 2024 •

edited

Loading

ctb Sep 17, 2024

bluegenes Sep 17, 2024

ctb Sep 17, 2024

ctb Sep 17, 2024

ctb Sep 18, 2024

luizirber Sep 21, 2024

ctb commented Sep 18, 2024

ctb commented Sep 20, 2024

ctb commented Sep 20, 2024

bluegenes commented Sep 21, 2024

luizirber commented Sep 21, 2024

ctb commented Sep 21, 2024

MRG: add Manifest::intersect_manifest to Rust core #3305

MRG: add Manifest::intersect_manifest to Rust core #3305

Conversation

ctb commented Aug 21, 2024 • edited Loading

codecov bot commented Aug 21, 2024 • edited Loading

Codecov Report

ctb commented Sep 16, 2024

bluegenes Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

ctb Sep 17, 2024

Choose a reason for hiding this comment

bluegenes Sep 17, 2024

Choose a reason for hiding this comment

ctb Sep 17, 2024

Choose a reason for hiding this comment

ctb Sep 17, 2024

Choose a reason for hiding this comment

ctb Sep 18, 2024

Choose a reason for hiding this comment

luizirber Sep 21, 2024

Choose a reason for hiding this comment

ctb commented Sep 18, 2024

ctb commented Sep 20, 2024

ctb commented Sep 20, 2024

bluegenes commented Sep 21, 2024

luizirber commented Sep 21, 2024

ctb commented Sep 21, 2024

MRG: add `Manifest::intersect_manifest` to Rust core #3305

MRG: add `Manifest::intersect_manifest` to Rust core #3305

ctb commented Aug 21, 2024 •

edited

Loading

codecov bot commented Aug 21, 2024 •

edited

Loading

bluegenes Sep 17, 2024 •

edited

Loading