Stable sample identity across search outputs by jspaezp · Pull Request #147 · TalusBio/timsbuktoolkit

jspaezp · 2026-09-18T16:51:11Z

Summary

Derive stable sample IDs from parent URI text plus display stem. Hash only URI text (fixed FNV-1a 64-bit); never read/hash raw contents.
Reject two inputs with the same derived ID within one invocation, before staging, prediction or search, including --overwrite. Rerunning the same command with --overwrite still replaces prior outputs normally. Same stems in different parents work in batch or independent invocations.
Reuse resolved identity for artifact probes, output directories, cleanup and upload; retain sample_id/sample_name in run/performance reports and Parquet metadata, including empty and raw-score files.
Keep identity fields private. Reports/writers accept a location-constructed SampleIdentity, not arbitrary ID/name strings; no unchecked deserialization.

Compatibility

Per-sample directories change from <stem>/ to <16-hex-prefix>-<stem>/. Legacy directories are not migrated. Parquet columns/format version remain unchanged: sample identity is file-level metadata. Consumers combining files must retain that metadata. Local paths become absolute without resolving symlinks; no moved-file/content equivalence is promised. See docs/development.md for pinned decomposition and fixtures.

No library provenance, generated-decoy, scientific scoring, reducer, or FDR changes.

CI vocabulary refresh

Regenerated PSI-MS origin-type snapshot from 4.1.261 to 4.2.2. The generated diff is only the version string: all ten origin terms and their decoy/theoretical-m/z memberships are identical. Compared full release term stanzas excluding new subset: tags: no existing terms removed, only MS:1003384 (semantic-version regex, unused here) changed; 1,934 terms added outside the generated subtree. mzSpecLib reader's RT/mobility, origin and decoy term semantics remain unchanged.

Sources: 4.1.261, 4.2.2.

Additional checks: uv run python scripts/gen_psims_origin_type.py --check; cargo test -p timsquery serde::mzspeclib_io — all 29 reader tests passed.

Verification

cargo test -p timsseek_cli -p timsseek --lib --bins — 306 passed, 1 existing ignored test
cargo test -p timsseek_cli --test sample_identity — CLI rejection with/without overwrite
cargo test -p timsseek --doc sample_identity — forging private fields fails compilation
cargo clippy -p timsseek -p timsseek_cli --all-targets -- -D warnings
task fmt; git diff --check

Tests pin hash vectors, independent/reordered runs, local/remote paths, storage suffixes, duplicate/invalid inputs, separate artifact destinations, cleanup isolation, JSON fields and empty/nonempty Parquet metadata in both modes. No live cloud upload or full instrument-data search was performed.

github-actions · 2026-09-18T16:58:12Z

Apex-finder bench

cargo run -p apex_sim --release --example bench -- 1000 2

Sensitivity + timing across canonical scenarios

=== summary (n=1000, tol=±2 cycles) ===
  scenario                      pass2%  pass1%   medErr    us/run
  clean                          100.0   100.0        0     27.10
  moderate_noise                  92.7    92.7        0     26.72
  high_noise+interference         59.3    59.3        1     26.64
  heavy_interference               7.4     7.4       88     26.68
  mismatched_library              59.8    59.8        1     26.66
  absent_top_fragment             16.0    16.0       65     26.63
  absent_precursor                59.3    59.3        1     26.68

=== broad apex-finding (n=500, tol=±2 cycles) ===
  scenario                      pass2%  pass1%   medErr    us/run
  broad_clean                    100.0   100.0        0    165.18
  broad_moderate_noise            50.6    50.6        1    159.25
  broad_high_noise+interf         27.6    27.6      308    158.38
  broad_hard_3x_density            0.8     0.8      487    158.49
  broad_mismatched_library        41.0    41.0      161    158.64
  broad_measured_density          49.6    49.6       13    187.53

=== narrow recovery (n=1000, tol=±2 cycles) ===
  scenario                      pass2%  pass1%   medErr    us/run
  narrow_clean                   100.0   100.0        0     18.41
  narrow_moderate_noise           81.3    81.3        0     18.15
  narrow_high_noise+interf        65.8    65.8        1     18.06
  narrow_hard_3x_density          10.9    10.9       34     18.19
  narrow_mismatched_library       62.8    62.8        1     18.04
  narrow_measured_density         83.6    83.6        0     20.69

=== narrow score discrimination (AUC, n_seed_pairs=1000) ===
  scenario                         AUC   med+signal    med-noise
  narrow_clean                   1.000     4.329e14      7.199e7
  narrow_moderate_noise          0.870      4.054e9      3.054e8
  narrow_high_noise+interf       0.830      2.761e9      3.602e8
  narrow_hard_3x_density         0.684      3.644e9      1.244e9
  narrow_mismatched_library      0.842      2.172e9      3.575e8
  narrow_measured_density        0.858      4.341e9      2.562e8

commit c440386

jspaezp · 2026-09-18T16:58:38Z

 /// Borrows the arena the results were scored against to resolve each result's
 /// row handle into the external library and competition-group IDs.
 pub struct ResultParquetWriter<'a> {
+    sample: Option<crate::sample_identity::SampleIdentity>,


move the import up

Moved SampleIdentity into the top-level imports in fbfb7a5.

jspaezp · 2026-09-18T16:59:49Z

            ArrowWriter::try_new(file, schema, Some(props)).map_err(std::io::Error::other)?;

        Ok(Self {
+            sample: None,


Q: why not pass the identity directly instead of having this as an option + over-write?

The optional setter only preserved the old constructor API; no current caller needed identity-less output. Changed new/raw to require &SampleIdentity and establish metadata at construction. Removed the optional state and setter; updated all callers and empty/nonempty raw/rescored tests. 306 unit tests pass, clippy clean (fbfb7a5).

jspaezp · 2026-09-18T17:01:53Z

+// Fixed FNV-1a 64-bit over UTF-8 bytes. Not DefaultHasher: its implementation
+// is not a persistence contract. Parent text includes its trailing slash.
+fn fnv1a64(bytes: &[u8]) -> u64 {
+    bytes.iter().fold(0xcbf29ce484222325, |hash, byte| {


O.O pretty interesting ...

jspaezp · 2026-09-18T17:02:20Z

+    let mut stem = name;
+    loop {
+        let before = stem;
+        for ext in [".idx", ".tar", ".gz", ".d", ".raw", ".mzML", ".mzml"] {


Q: do we define these elsewhere?

Overlapping checks exist, but no shared suffix list: tims_stage::uri::parse_uri_shape classifies .idx/.tar; vendor readers sniff formats (e.g. mzML). Those answer format support, not stable identity normalization. Named this SAMPLE_STORAGE_SUFFIXES and documented that distinction in fbfb7a5; enabling another reader should not silently change persisted IDs. In particular, stripping .raw/.mzML.gz here does not claim those formats are currently readable.

Q2: why is this defined as caps sensitive match?

Case sensitivity was inherited from the old helper, not intentional. bef2891 makes suffix matching ASCII-case-insensitive (.D, .RAW, .MzMl.GZ, etc.) while preserving stem and parent URI case. Added mixed-case, Unicode and duplicate-identity fixtures. Also renamed writer new() to rescored() to make the schema distinction from raw() explicit.

Updated per our discussion: a19ef90 removes the separate sample suffix list. RawReader now owns sample_name(); built-in readers share the same case-insensitive rule for sniffing and naming, and ReaderRegistry dispatches naming. Staging unwraps .idx/.tar (preserving standalone wrapper names without a raw suffix); SampleIdentity retains validation and URI hashing. Unsupported raw formats are no longer assigned guessed stems. Verified 410 unit tests, nine registry tests without default features, CLI duplicate-input integration, formatting and clippy.

Copilot

Copilot review overview

🟡 Changes recommended

Windows-invalid characters in derived directory suffixes can cause valid remote-key inputs to fail output creation.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

Reject Windows-invalid characters in derived staging directory IDs · New

What changed in this PR

Adds stable, location-derived sample identities across search outputs, reports, artifacts, and Parquet metadata.

Changes:

Adds FNV-1a IDs and duplicate-input validation.
Propagates identities through outputs, cleanup, uploads, and reports.
Updates PSI-MS metadata version and development documentation.

File	Description
`rust/timsseek/src/scoring/timings.rs`	Adds identity to reports.
`rust/timsseek/src/scoring/parquet_writer.rs`	Stores identity metadata in Parquet files.
`rust/timsseek/src/sample_identity.rs`	Implements identity derivation and validation.
`rust/timsseek/src/lib.rs`	Exposes the identity module.
`rust/timsseek_cli/tests/sample_identity.rs`	Tests duplicate rejection.
`rust/timsseek_cli/src/search.rs`	Integrates identity resolution into searches.
`rust/timsseek_cli/src/sample_identity.rs`	Resolves identities and detects duplicates.
`rust/timsseek_cli/src/processing.rs`	Propagates identities to writers and reports.
`rust/timsseek_cli/src/output_sink.rs`	Removes legacy basename derivation.
`rust/timsseek_cli/src/main.rs`	Registers the CLI module.
`rust/timsseek_cli/src/artifacts.rs`	Uses identities for artifact probing.
`rust/timsquery/src/serde/psims_origin_type.rs`	Updates the ontology version.
`docs/development.md`	Documents identity and output-path behavior.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

jspaezp · 2026-09-21T17:48:06Z

+    if name.contains('\\') || name.chars().any(char::is_control) {
+        return None;
+    }


NGL this suggestion looks disgusting ...

ngl this looks horrendous and im unsure when it actually matters.
I do like that there is something raised in the sense that some remote keys can include byte-range assets (say define a file inside a .zip) but this is very much not within the scope of this PR

Leaving the proposed Windows-character restriction/encoding expansion out per your scope feedback. This concern is specifically a remote key being reused as a Windows staging-directory component; it does not require archive/member or byte-range URI handling, and none is added here. The existing minimal separator/control-character checks remain unchanged.

jspaezp · 2026-09-22T19:04:36Z

NGL this feels like a solution to a non-problem ...

Use stable location-derived sample identities for search outputs

1ac019a

jspaezp commented Sep 18, 2026

View reviewed changes

Refresh PSI-MS vocabulary and clarify overwrite reruns

8917867

jspaezp commented Sep 18, 2026

View reviewed changes

jspaezp requested a lite review from Copilot September 18, 2026 22:27

Require sample identity when constructing result writers

fbfb7a5

Copilot started reviewing on behalf of jspaezp September 18, 2026 22:30 View session

Copilot AI reviewed Sep 18, 2026

View reviewed changes

jspaezp added 4 commits September 22, 2026 11:17

Clarify result writer modes and normalize suffix case

bef2891

Delegate canonical sample naming to raw reader registry

a19ef90

Trim sample identity documentation to usage essentials

5279b63

Reuse selected raw reader for sample naming and loading

4a9227d

Conversation

jspaezp commented Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Compatibility

CI vocabulary refresh

Verification

Uh oh!

github-actions Bot commented Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Apex-finder bench

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jspaezp commented Sep 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jspaezp commented Sep 18, 2026 •

edited

Loading

github-actions Bot commented Sep 18, 2026 •

edited

Loading