Read sample identity from the folder, not just the file name - #129
Read sample identity from the folder, not just the file name#129PoslavskySV wants to merge 1 commit into
Conversation
Patterns were matched against the bare file name, so a one-folder-per-sample
tree — what BaseSpace, bcl2fastq and CellRanger all produce — could not be
described at all. Where the folder was the only place a sample name appeared,
every file resolved to the same sample and getOrCreateSample collapsed them,
keeping only the last.
- Match against each file's path relative to the longest common directory of
the selection. Recomputed over the whole set on every render rather than
threaded through the file dialog, so it stays well defined when the
selection spans folders or grows over several "add more files" rounds. With
every file in one folder the remainder is the bare name, so flat imports are
byte-identical to before.
- Bound {{Sample}}, {{*}} and tag matchers to a single path segment; add
{{**}} for crossing them. Required, not cosmetic: a lazy `.+?` swallows the
separator and pulls the folder into the sample name.
- Infer over file-name, folder-ignored and folder-carries-identity forms,
taking the first that gives every file its own identity. Per-sample-folder
FASTQ and CellRanger MTX now infer.
- Recognise <Sample>_S<n>_L<lane>_<read>_001, so the Illumina sample number
stops being absorbed into the sample name. Three inference tests changed
expectation; all three inputs are real 10x names where the new answer is
the right one.
- Report files that collapse onto one identity and hold the import, rather
than overwriting silently.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const fileNames = relativeFilePaths(files); | ||
| if (data.files.length === 0) { | ||
| const inferredPattern = inferFileNamePattern(fileNames); |
There was a problem hiding this comment.
When a user selects files from one directory and then adds files from another, inference uses paths relative to the first batch while useParsedFiles recomputes every path against the full selection. The shorter common prefix introduces directory segments that the inferred segment-bounded pattern cannot match, causing previously selected files to become unmatched and either blocking the import or allowing only a partial selection to be imported.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/src/dialogs/ImportDatasetDialog.vue
Line: 271-273
Comment:
**Multi-round paths invalidate inference**
When a user selects files from one directory and then adds files from another, inference uses paths relative to the first batch while `useParsedFiles` recomputes every path against the full selection. The shorter common prefix introduces directory segments that the inferred segment-bounded pattern cannot match, causing previously selected files to become unmatched and either blocking the import or allowing only a partial selection to be imported.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| if (match.readIndex) parts.push("read=" + match.readIndex.value); | ||
| if (match.cellRangerFileRole) parts.push("role=" + match.cellRangerFileRole.value); |
There was a problem hiding this comment.
When equivalent read indices such as 1, r1, and R1 are selected, or a CellRanger group contains both genes.tsv and features.tsv, sampleKeyOf treats the raw values as distinct. The content builders normalize them to the same R1 or features.tsv slot, so duplicate detection permits the import and the later file silently overwrites the earlier one.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/src/dialogs/datasets.ts
Line: 227-228
Comment:
**Raw keys miss normalized collisions**
When equivalent read indices such as `1`, `r1`, and `R1` are selected, or a CellRanger group contains both `genes.tsv` and `features.tsv`, `sampleKeyOf` treats the raw values as distinct. The content builders normalize them to the same `R1` or `features.tsv` slot, so duplicate detection permits the import and the later file silently overwrites the earlier one.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
|
One rollout note that isn't obvious from this diff, so it doesn't get lost. This PR ships only the pattern engine. The other half of the work — cross-folder selection in the file dialog, and the "Include subfolders" control — lives in So there's a fourth step after milaboratory/platforma#1766 publishes: bump Full order: platforma#1766 → publish → bump ui-vue here → desktop SDK bump + platforma-desktop-app#524. This PR is independent of all of it and can merge first. The dialog is built to degrade safely in the meantime: it feature-detects whether the host honours |
|
Closing — implementation was premature. This feature is being specified first as a mispec corpus in docs/text; code will follow only after that spec is reviewed and approved. The branch is deleted; commits remain reachable from this closed PR if any of the analysis is wanted later. |
1 participant
Part 2 of 3 for folder-per-sample import. Spec: milaboratory/text#201. Independent of the other two — this can merge on its own.
Why
Patterns were matched against the bare file name (
datasets.ts:169didextractFileName(getFileNameFromHandle(handle))), so a one-folder-per-sample tree — what BaseSpace, bcl2fastq and CellRanger all produce — could not be described at all.Worse: where the folder is the only place a sample name appears, every file resolves to the same sample,
getOrCreateSamplematches by label, andfileGroup[readIndex] = handlekeeps only the last file. A wrong result that looks like a successful import. Auto-inference refuses (there's a collision guard), but a hand-written pattern walks straight into it.What changed
Match against relative paths.
useParsedFilesstrips the longest common directory prefix across all accumulated files and matches what remains. This needs no file-dialog API change: recomputing the prefix over the whole set beats threading a root through, because it stays well defined when the selection spans folders or grows over several "add more files" rounds. With every file in one folder the remainder is the bare file name, so flat imports are unchanged by construction.Segment-bounded matchers.
{{Sample}},{{*}}and tag matchers compile to[^/]+?; new{{**}}crosses segments. This is required, not cosmetic — a lazy.+?swallows the separator, and Synolo's sample name comes out asSel78_R1_02_L1-ds.bf24…/Sel78_R1_02. Backward compatible: today's inputs contain no/.escapeRegExpalready left/alone, so no new syntax was needed for the literal.Inference tries path variants — file name,
{{**}}/…, and two folder-carries-identity forms — taking the first that gives every file its own identity. The existing duplicate-key guard does the choosing for free. Newly inferable: per-sample-folder FASTQ (Sample_A/R1.fastq.gz) and per-sample-folder CellRanger MTX (Sample_A/matrix.mtx.gz).Duplicate identities are reported and block the import instead of overwriting silently.
One deliberate behaviour change, please look at this
<Sample>_S<n>_L<lane>_<read>_001— canonical Illumina naming — is now recognised, soA_S7_L001_R1_001.fastq.gzgives sampleA, notA_S7._S<n>is the sample number bcl2fastq assigns; no well-known pattern claimed it, so it was being absorbed into{{Sample}}.Synolo needs this: folder and file name both carry
_S<n>, and without it their samples come out asSel78_R1_02_S3.Three existing inference tests changed expectation. All three inputs are real 10x file names (
10k_PBMC_..._gex_1_S7_L001_R1_001.fastq.gz) where the new answer is the better one — I checked each rather than just re-baselining. A user who wants the number in the name can still say so in the pattern field. Flagging it because it changes inferred sample names for existing bcl2fastq/BaseSpace imports.Verification
pnpm checkclean (types, lint, format). 78 tests pass — 41 infile_name_parser.test.ts(7 new, including the real Synolo tree and a generic-basename tree) and 11 new indatasets.test.tscovering the prefix arithmetic and duplicate detection.Not verified in a running app — no desktop instance available. The pattern engine is where the risk is, and it's covered by tests.
Greptile Summary
This PR makes dataset imports path-aware and prevents detected identity collisions.
{{Sample}},{{*}}, and tags are now segment-bounded, while new{{**}}can cross directories._S<n>sample numbers.Confidence Score: 3/5
The PR should not merge until multi-round path inference remains stable and duplicate detection uses the same normalized identities as the dataset builders.
Adding files from another directory can invalidate the inferred pattern for existing files, while raw collision keys still allow equivalent read indices and CellRanger roles to overwrite one another silently.
Files Needing Attention: ui/src/dialogs/ImportDatasetDialog.vue, ui/src/dialogs/datasets.ts
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart LR A[Selected file handles] --> B[Normalize separators] B --> C[Remove common directory prefix] C --> D[Infer or compile path pattern] D --> E[Parse sample, lane, read, role, and tags] E --> F[Check duplicate identities] F -->|Unique| G[Build dataset content] F -->|Collision| H[Show error and block import]Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "Read sample identity from the folder, no..." | Re-trigger Greptile