feat(samples): add samples import command for public-repository accessions - #18
Conversation
…essions Adds `client.samples.import_samples`/`get_import` (POST/GET `/v2/sample-imports`) and the `flowbio samples import` CLI verb, mirroring `upload-batch`'s validate-up-front / report structure for a CSV accession sheet (an `accession` column plus optional name/organism and metadata columns, in place of reads1/reads2/project). Unlike upload-batch, the server tracks every accession submitted together as a single job (one status/error, accessions/sample_ids returned positionally) rather than a per-row upload. The CLI submits all valid rows in one call and polls that one job to completion (--poll-interval/--timeout), per the ticket amendment to poll per job rather than per accession. A completed job reports every row imported; a failed or timed-out job reports every row failed with the job's message, since the job has one outcome for the whole batch. _sheet.py's per-row metadata validation (required/closed-option/annotation rules) is extracted into a shared `metadata_errors` helper so the new accession-sheet validation in _accession_sheet.py reuses it instead of duplicating it; behaviour of validate_row is unchanged. Verified with respx-mocked unit tests (client methods, sheet parsing, and the CLI command including polling/timeout/failure paths) and manual CLI --help smoke checks; full suite passes (343 tests). Refs FLOW-689. Acceptance-test driver's import_by_accession is not routed through flowbio in this PR - see PR description for why.
|
@claude please review this PR |
|
Claude finished @mhusbynflow's task in 4m 9s —— View job Review:
|
|
Thanks for the thorough review — working through the findings now. Planning to fix 1–6 and the convention nits I agree with (positions/typing), and will reply inline on the ones I'm deliberately leaving as-is (7, the sheet-format tradeoffs in 9, and part of 6) with reasoning. Will push shortly. |
- Surface job_id/job_status/execution_id in the JSON document (null when no job was created), so a timed-out or interrupted import can be resumed with client.samples.get_import instead of leaving an untrackable server-side job. - Reject non-positive --poll-interval/--timeout as a usage error (exit 2) instead of crashing with ZeroDivisionError/ValueError. - Switch _poll_job from an attempt-count budget to a time.monotonic() deadline, so --timeout is actually a wall-clock bound rather than poll_interval * attempts (which silently added unbounded request latency and ignored a --timeout smaller than --poll-interval). - A COMPLETED job whose sample_ids don't cover every submitted accession no longer silently reports those rows as imported with sample_id: null; they are now reported failed with an explanatory message. - A failed/timed-out job now still reports the sample id for any accession the job did manage to create before failing, so a retry doesn't have to guess what to skip. - The timeout message no longer reads as "ended with status RUNNING"; it now says the job didn't finish in time and points at get_import to resume. - Reordered _sheet.py so the public metadata_errors sits above the private _build_row/_resolve helpers. Addresses Claude's review on flowbio#18; see PR comments for the findings left as deliberate, explained trade-offs (SampleImportSpec as a dataclass, sample_ids staying int, the accession-format regex, and not adding poll-error retry tolerance). Refs FLOW-689.
|
Pushed fixes for findings 1–6 (job id/status/execution_id now in the JSON document; On the rest, here's what I did and why:
|
|
@tessl-change-review |
|
@claude please review this PR |
|
Claude finished @mhusbynflow's task in 4m 16s —— View job Review:
|
| # | Fix | Status |
|---|---|---|
| 1 | job_id/job_status/execution_id in document (_samples.py:657-666), None only when no job was created, timeout message points at get_import |
✅ |
| 2 | _validate_poll_options raises CliUsageError for non-positive values (_samples.py:718-722) |
✅ |
| 3 | COMPLETED row with no matching sample id now reported failed (_completed_outcomes, _samples.py:797-811) |
✅ |
| 4 | Failed/timed-out rows carry sample_id when the server returned one (_samples.py:830-838) |
✅ |
| 5 | Distinct "did not finish within the configured timeout (still RUNNING)" message, asserted not to say "ended with status RUNNING" | ✅ |
| 6a | time.monotonic() deadline replaces the attempt-count budget (_poll_job, _samples.py:766-772) |
✅ (one residual, below) |
The rejections you argued for (7, 8, the sheet-format tradeoffs in 9) are all reasonable as stated — matching the server's StrEnum, keeping sample_ids at the server's wire type, and not widening metadata_errors' contract in a slice that shares it with upload-batch are the right calls, and test_transient_poll_error_propagates documenting the deliberate propagation is the right way to close 6b. No further comment on those.
New / residual findings
1. SampleImportSpec and SampleImportJob aren't exported from flowbio.v2
flowbio/v2/__init__.py:42-66 re-exports every other public sample model — Sample, SampleType, MetadataAttribute, Project, Organism, MultiplexedUpload, SampleTypeId — but not the three new names. SampleImportSpec is a required input to import_samples: a user cannot call the method without constructing one, so it's the type that most needs to be reachable from the package root. The docs feel this: source/v2/samples.rst has to write from flowbio.v2.samples import SampleImportSpec while the package's own Quick Start uses from flowbio.v2 import Client, UsernamePasswordCredentials.
Adding SampleImportSpec, SampleImportJob, and SampleImportJobId to the import block and __all__ (and switching the docs example to from flowbio.v2 import SampleImportSpec) closes the gap. Fix this →
2. The sleep isn't clamped to the remaining deadline, so --timeout can still be overshot
deadline = time.monotonic() + timeout
while job.status == "RUNNING" and time.monotonic() < deadline:
time.sleep(poll_interval)_samples.py:766-772. The deadline fixes the accumulating overrun from round 1, but a single sleep is still unbounded relative to it: --timeout 10 --poll-interval 300 checks 0 < 10, then blocks for 300 s — 30× what --help promises ("Maximum seconds to wait for the import job to finish"). time.sleep(min(poll_interval, deadline - time.monotonic())) makes the flag mean what it says, and lets --poll-interval be tuned independently without silently dominating --timeout. Fix this →
3. dict(zip(accessions, sample_ids)) now mass-fails (or mis-attributes) on a length mismatch
_import_result (_samples.py:775) still zips, and zip truncates silently. Fixing finding 3 changed the consequence rather than removing it:
len(sample_ids) < len(accessions)→ the unpaired rows are reported failed with exit 1, even if the job genuinely imported them.len(sample_ids) > len(accessions)(what an SRX experiment expanded into two runs would look like) →zippairs by index, so row 2 is handed row 1's second sample id: a plausible-looking but wrong sample id, reported as a success.
Both are only reachable if the server's two lists ever disagree in length, which your own model docstring says they don't ("correspond positionally"). Given the whole outcome mapping rests on that invariant, it's worth asserting rather than assuming: when len(job.sample_ids) not in (0, len(job.accessions)), emit an advisory and fail the rows rather than pairing by index. That also removes the quieter variant of the same risk — the client upper-cases accessions (_accession_sheet.py:154) and then matches on the server's echo, so any server-side normalisation difference turns a successful import into exit 1. Fix this →
4. A timed-out job puts still-running work in the failed bucket
_failed_outcomes is used for both FAILED and the timeout path, so a run that simply hit --timeout reports counts: {"failed": N} while job_status is "RUNNING". The message is now clear for a human, and job_status does let a machine consumer tell the two apart — but the bucket name is the thing agents branch on, and the natural reaction to failed is to re-run, which would duplicate whatever the still-live job goes on to create. Consider a per-entry "status" ("failed" vs "running"), or splitting the timeout rows into their own list, so the coarse read of the document isn't wrong. At minimum, source/cli.rst's exit-code paragraph could spell out that exit 1 with job_status == "RUNNING" means unknown, still in flight — poll, don't re-run.
5. --poll-interval 0 is rejected only after authentication
_validate_poll_options is the first statement in the handler, but _dispatch resolves credentials and calls client.log_in() before invoking any handler (_main.py:74-87). With no token configured, flowbio samples import --poll-interval 0 … therefore runs the interactive login flow and only then prints the usage error. An argparse type= callable (a small positive_float helper raising argparse.ArgumentTypeError) rejects it at parse time, before any credential work — same exit code 2, and it also puts the constraint in --help automatically. Fix this →
6. Typing nits on _ImportResult
job_id: int | None and job_status: str | None (_samples.py:645-647) widen types the module already has precise names for — SampleImportJobId | None and SampleImportStatus | None. Both are already imported or trivially importable from flowbio.v2.samples, and CLAUDE.md's "named types over a bare primitive" applies directly. (execution_id: int | None matches the model, so it's fine as-is.)
7. Test-fixture brittleness
test_timeout_reports_failure_without_hanging patches time.monotonic globally with a four-element side_effect. That's a module-wide patch during live respx/httpx request handling — any call to time.monotonic() from inside those libraries consumes an element and the test dies with StopIteration rather than a useful failure, and the list has to be re-counted by hand whenever _poll_job's loop shape changes. A monotonically increasing fake (itertools.count(step=...), or a tiny closure over a mutable clock) is sturdier and expresses the intent — "time advances 1 s per tick" — better than four literals. Minor: class TestImportSamples (tests/unit/v2/test_samples.py) has one blank line above it where the file uses two; no linter runs in CI, so cosmetic only.
8. A sheet with no accession column degrades confusingly (low priority)
parse_accession_sheet doesn't check that the reserved column exists, so a sheet headed run,name yields "missing required value: accession" on every row and silently ships run as metadata. upload-batch has the same shape for name, so this is consistent rather than a regression — but since there's no import-template to generate the header, this sheet is hand-made more often than upload-batch's, and a single "sheet has no accession column" usage error would land better. Fine to leave for the same follow-up you scoped the unknown-column warning into.
The fixes from round 1 all landed as described, and the ones you pushed back on are well argued. Of the above, 1 (missing exports — a user-facing API gap in a new public surface) and 3 (the zip invariant, now that an unmatched row changes the exit code) are the two I'd want addressed before merge; 2, 5, 6 and 7 are small and mechanical; 4 and 8 are judgement calls you may reasonably defer. Happy to implement any subset — just say which.
• branch implement/FLOW-689-samples-import-command
|
Thanks — round 2 findings look right. Fixing 1 (missing |
- Export SampleImportSpec/SampleImportJob/SampleImportJobId from flowbio.v2, matching every other public sample model; update the samples.rst example to import from the package root instead of the submodule. - Clamp _poll_job's sleep to the time remaining before the deadline, so --poll-interval can no longer make a single sleep overshoot --timeout (e.g. --timeout 10 --poll-interval 300 previously blocked for 300s). - Stop pairing job.accessions/job.sample_ids positionally via zip when their lengths disagree — that silently truncated (reporting a row failed even though the job created its sample) or mis-attributed (handing a row another row's sample id) instead of raising. Both directions are now reported as an explicit, unmatchable failure instead. - Give each failed-row entry a status: "failed" (safe to retry), "running" (timed out but the job may still finish — don't re-run) or "unknown" (accessions/sample_ids couldn't be matched), so an automated caller doesn't treat a timeout the same as a genuine failure. - Move --poll-interval/--timeout validation into an argparse type= callable, so a non-positive value fails at parse time (exit 2) before _dispatch resolves credentials, instead of after an interactive login. - Narrow _ImportResult.job_id/job_status to SampleImportJobId/ SampleImportStatus instead of bare int/str. - Replace the timeout test's hand-counted time.monotonic() side_effect list with itertools.count(...), an infinite fake clock that doesn't need re-counting whenever _poll_job's loop shape changes. Addresses Claude's round-2 review on flowbio#18. Finding 8 (sheet with no accession column degrading confusingly) is left for the same follow-up as round 1's unknown-column-warning deferral, as suggested. Refs FLOW-689.
|
Pushed round-2 fixes:
Left 8 (a sheet with no All 355 tests pass locally (added 5 more this round, on top of the 7 from round 1). |
|
@tessl-change-review |
|
@claude please review this PR |
|
Claude finished @mhusbynflow's task in 4m 25s —— View job Review:
|
| # | Fix | Status |
|---|---|---|
| 1 | SampleImportSpec/SampleImportJob/SampleImportJobId exported from flowbio.v2 (__init__.py:48-50, __all__:65-67), docs example imports from the package root, test_sample_import_types_are_exported asserts identity |
✅ |
| 2 | _poll_job sleeps min(poll_interval, remaining) (_samples.py:768-774); test_poll_interval_exceeding_timeout_is_clamped asserts sleep(10.0), not sleep(300) |
✅ |
| 3 | _sample_ids_unmatchable (_samples.py:794-799) refuses positional pairing on a length mismatch, tested both directions |
✅ |
| 4 | Per-entry status of failed/running/unknown, documented in cli.rst with the "don't re-run a running row" guidance |
✅ |
| 5 | --poll-interval/--timeout validated by the _positive_float argparse type= callable, so a bad value fails before credential resolution (one hole, finding 3 below) |
✅ |
| 6 | _ImportResult.job_id/job_status are `SampleImportJobId |
None/SampleImportStatus |
| 7 | itertools.count(0.0, 1.0) clock in the timeout tests, blank line fixed |
✅ (one straggler, finding 8) |
New findings
1. The fix for #3 silently swallows the fix for #4 — a partially-completed failed job now reports no sample ids at all
_sample_ids_unmatchable (_samples.py:794-799) runs before the status branch, so it catches FAILED jobs too. The single most likely partial-failure shape — job fails halfway, so accessions == ["ERR1", "ERR2"] but sample_ids == [101] — hits len(sample_ids) not in (0, 2) → _mismatched_outcome, which emits status: "unknown" and drops job.sample_ids entirely. That is precisely the scenario round-1 finding 4 asked you to surface, and it now reports less than before the fix: the user is told "cannot match them to rows" and never learns sample 101 exists, so a re-run duplicates it.
test_failed_job_with_all_sample_ids_created_reports_them_per_row only covers the all-or-nothing case ([101, 102] for two accessions), which is why this didn't show up.
Refusing to pair a short list positionally is right; refusing to report it isn't. A job-level created_sample_ids: [101] in the document (plus an advisory) keeps the safety property and restores the information. Fix this →
2. COMPLETED with an empty sample_ids is labelled "failed" — the one label documented as safe to re-run
_sample_ids_unmatchable treats len(sample_ids) == 0 as always acceptable ("expected on a RUNNING/FAILED job"), but for a COMPLETED job with N accessions it is exactly as much a broken-invariant response as len == 1. The consequence is inconsistent:
COMPLETED,accessions=[A,B],sample_ids=[101]→status: "unknown"→ don't re-run (cli.rst: "an unexpected shape from the server")COMPLETED,accessions=[A,B],sample_ids=[]→status: "failed"(_completed_outcomes,_samples.py:817-824) → "safe to re-run once fixed"
The empty case is the more degenerate of the two, and the server has told us it completed — re-running is the duplicate-sample risk finding 4 was about. Gating the emptiness allowance on job.status != "COMPLETED" makes the two paths agree. test_completed_job_missing_sample_id_reports_row_as_failed currently locks in the inconsistency, so it would need updating alongside. Fix this →
3. _positive_float accepts nan and inf, reopening the round-1 crash/hang
_positive_float (_samples.py:264-268) only checks parsed <= 0, and every comparison against nan is False:
--timeout nan→deadline = nan,remaining = nan,remaining <= 0isFalseforever → the command polls without a timeout, silently ignoring the flag.--timeout 1e400(inf) does the same.--poll-interval nan→min(nan, remaining)returnsnan→time.sleep(nan), which CPython rejects withValueError;_dispatch(_main.py:88-96) catches onlyCliUsageError/FlowApiError, so it's an uncaught traceback rather than exit 2 — the same failure mode round-1 finding 2 closed for0.
if not math.isfinite(parsed) or parsed <= 0 covers both, and reads as what the help text already promises. Fix this →
4. SampleImportStatus is the one new public name still not exported
job.status is a SampleImportStatus, _ImportResult.job_status is annotated with it, and cli.rst documents job_status as part of the output contract — but a user writing def on_status(s: SampleImportStatus) still has to reach into flowbio.v2.samples. It's the same one-line addition as the other three in flowbio/v2/__init__.py:48-50/__all__.
5. Test gap: the sheet's name column is never asserted to reach the payload
IMPORT_HEADERS includes name, but no CLI test sets it — test_sends_sample_type_and_metadata_in_kickoff_payload covers accession/sample_type/organism/metadata only. Deleting name=row.name from _run_import (_samples.py:749) would leave the suite green, and since the server substitutes the accession when name is absent (cli.rst's documented behaviour), the bug would be invisible in a smoke test too. One extra record with "name": "liver_r1" and a payload assertion closes it. Fix this →
6. Human-mode wording still says "import failed" for a row whose status is running
_failed_outcomes (_samples.py:849) builds every advisory as Row N (ERR1): import failed — import job 1 did not finish within the configured timeout (still RUNNING) …. The JSON consumer now gets the distinction (round-2 fix 4), but the human reading stderr is told the import failed in the same breath as being told it's still running — the same self-contradiction round-1 finding 5 fixed in the message body. import did not finish — for the timeout branch would make the two agree.
7. Docs nits in source/cli.rst
- The
--jsonsample document shows{"row_number": 2, "accession": "bogus", …}inskipped, but_build_rowupper-cases accessions (_accession_sheet.py:154), so the real output is"BOGUS"— your owntest_skip_invalid_imports_valid_rowsasserts that. Worth matching so nobody keys off the lower-case form. - The two worked examples disagree with each other: the human run imports 2 rows from
./accessions.csv, the--jsonrun of the same file imports 1.
8. One time.monotonic fixture didn't get the round-2 treatment
test_poll_interval_exceeding_timeout_is_clamped still uses mock_monotonic.side_effect = [0.0, 0.0] — a hand-counted two-element list — while its two siblings now use itertools.count(0.0, 1.0). It happens to work today (_poll_job calls monotonic exactly twice before the job comes back COMPLETED), but it's the same StopIteration-on-refactor brittleness you fixed elsewhere, and it's cheaper to make it consistent now than to debug it later.
Findings 1 and 2 are the ones I'd want addressed before merge — both are cases where the round-2 hardening changed which wrong thing happens rather than removing it, and both end with the user unable to tell "safe to retry" from "you'll duplicate samples". 3 is a small, contained hole in validation you've already invested in twice. 4–8 are mechanical.
Everything else about the slice reads well: the up-front validation property holds, _completed_outcomes/_failed_outcomes/_mismatched_outcome are a clear decomposition, the deadline-based poll loop is correct (including the final poll after a clamped sleep), and the docs carry concepts plus one example as CLAUDE.md asks.
• branch implement/FLOW-689-samples-import-command
|
Good catches — 1 and 2 are real regressions from how round 2's fixes interacted with each other. Fixing 1–8 now; this is round 3 per the review cap, so I'll fold in everything reasonable this pass. Pushing shortly. |
- Stop discarding a job's sample_ids just because they can't be paired positionally: a length mismatch (the normal partial-failure shape, e.g. a FAILED job that created one of two samples before failing) now surfaces those ids verbatim as job_sample_ids in the document instead of silently dropping them, while still refusing to attribute them to specific rows. - Treat a COMPLETED job that returns zero sample ids for its accessions as unmatchable (status "unknown", don't re-run) instead of "failed" (safe to re-run) — a completed job reporting nothing created is exactly as broken an invariant as any other length mismatch, and re-running it risks duplicating whatever it did create. An empty sample_ids on a RUNNING/FAILED job is still the normal, expected shape. - Reject non-finite --poll-interval/--timeout values (nan, inf): nan previously disabled the poll deadline entirely (every comparison against nan is False) and reached time.sleep(nan), an uncaught ValueError. - Export SampleImportStatus from flowbio.v2 alongside the other three new names, so a caller never has to reach into flowbio.v2.samples. - Distinguish "import failed" (genuine failure) from "import did not finish" (timeout) in the human-readable advisory, matching the JSON status field's distinction. - Add a test asserting the sheet's name column reaches the kickoff payload. - Reword source/cli.rst's failed/running/unknown status guidance, document job_sample_ids, and make the human/--json worked examples agree (both import the same two accessions) instead of silently disagreeing. - Replace the last hand-counted time.monotonic() side_effect list with itertools.count(...), matching the round-2 fixture fix. Addresses Claude's round-3 review on flowbio#18. Refs FLOW-689.
|
Pushed round-3 fixes:
All 360 tests pass locally (added 8 more this round). |
|
@tessl-change-review |
|
@claude please review this PR |
|
Claude finished @mhusbynflow's task in 3m 46s —— View job Review:
|
| # | Fix | Status |
|---|---|---|
| 1 | job_sample_ids always in the document (_samples.py:666, 809), asserted in the partial-failure tests |
✅ (one gap, finding 1) |
| 2 | _sample_ids_unmatchable gates the empty-list allowance on job.status == "COMPLETED" (_samples.py:819-821), with the reasoning in a comment |
✅ |
| 3 | math.isfinite in _positive_float (_samples.py:267), plus nan/inf tests on both flags |
✅ |
| 4 | SampleImportStatus exported (__init__.py:51, __all__:69) |
✅ |
| 5 | test_sends_name_in_kickoff_payload (test_samples.py:1087) |
✅ |
| 6 | Three distinct verbs — import failed / import did not finish / import outcome unknown |
✅ |
| 7 | "BOGUS" upper-cased in the docs, both worked examples now import the same two accessions, samples.rst gained its trailing newline |
✅ |
| 8 | itertools.count(0.0, 0.5) in the clamp test |
✅ (assertion still loose, finding 3) |
The decomposition has held up well across three rounds of change: _sample_ids_unmatchable / _mismatched_outcome / _completed_outcomes / _failed_outcomes each do one thing, the deadline loop is correct, and the metadata_errors extraction reads as though it was always shared.
New findings
1. On the mismatch path the server's failure reason is dropped, and the rescued sample ids are --json-only
Two halves of the same gap, both on the path round-3 fix 1 was written for (FAILED, accessions=["ERR1","ERR2"], sample_ids=[101]):
job.errornever reaches the user._mismatched_outcome(_samples.py:827-830) builds its message purely from the two list lengths.job.erroris only read in_failed_outcomes(_samples.py:880), and_ImportResult.documenthas no top-level error field at all — so"download failed"appears in neither stderr nor the JSON. Your owntest_failed_job_with_partial_sample_ids_still_surfaces_themsetserror="download failed"and assertsjob_sample_ids == [101], but nothing asserts the reason survives, which is why this didn't show. The user is told that the shapes don't line up and never told why the job failed.job_sample_idsis invisible in human mode.emit_advisoryis suppressed under--json(_output.py:53) and the per-row advisories on this path don't mention ids;emit_resultprints onlysummaryin human mode. So the ids are reachable only with--json— butcli.rstsays "Whatever sample ids the job did return are always available". A human running the command seesimport outcome unknown — … cannot match them to rowsand nothing else, which is exactly the duplicate-on-re-run hazard the fix was meant to close.
Appending job.error to the mismatch message, adding job_error to the document next to job_sample_ids, and emitting one job-level advisory naming the created ids covers both. Fix this →
2. A COMPLETED job whose accession strings don't match is still labelled "failed" — i.e. retry-safe
_sample_ids_unmatchable compares lengths only. Round-3 finding 2 established the principle you accepted: a COMPLETED job that didn't produce a usable id for a row is a broken invariant, not a per-row failure, so don't label it retry-safe. The equal-length-but-different-strings case still violates it — and your own new test pins the wrong behaviour in:
_job_json(1, "COMPLETED", ["ERR1", "err-other"], [101, 102]) # test_samples.py:1353Row ERR2 isn't in accession_to_sample_id, so _completed_outcomes (_samples.py:853-859) reports status: "failed" — documented in cli.rst as "safe to re-run once fixed". But the job completed and created sample 102 for err-other, which is very plausibly ERR2 under a server-side normalisation the client didn't anticipate (the client upper-cases, _accession_sheet.py:154). Re-running duplicates it. Same input, same hazard, opposite label from the len == 0 case one branch up.
Comparing set(job.accessions) against the submitted accessions and routing any difference to _mismatched_outcome makes the two agree; _completed_outcomes' sample_id is None branch then becomes genuinely unreachable dead-code-safety rather than a live, mislabelled path. Fix this →
3. The clamp test's assertion is looser than the property it's protecting
test_poll_interval_exceeding_timeout_is_clamped (test_samples.py:1255) asserts 0 < slept_for < 300. With the itertools.count(0.0, 0.5) clock the clamped value is exactly 9.5 (deadline = 0.0 + 10, second monotonic() → 0.5, min(300, 9.5)), so assert slept_for == pytest.approx(9.5) pins the actual arithmetic. The current bound passes for any sleep at all — including a regression that dropped the min() and slept a hard-coded 1 s. Fix this →
4. _mismatched_outcome repeats one job-level message once per row
The message is identical for every row (it names only the two lengths), so a 200-accession sheet prints 200 copies of the same sentence to stderr and stores 200 copies in the document. The per-row entry is right — callers index by row — but the human output would read better as one job-level advisory plus the per-row entries, and that's also the natural place to hang finding 1's error text and created ids.
5. Typing nit: job_sample_ids in the document
document is dict[str, JsonValue] and JsonValue's list arm is List["JsonValue"] (_types.py:19) — invariant, so a list[int] isn't assignable. Declaring the field list[JsonValue] (or building [int(i) for i in …] into a list[JsonValue]) keeps --strict clean; you already went out of your way for job_id/job_status in round 2, so this is the last one in the group.
6. Docs: two of the four new exports aren't in the API Reference
source/v2/samples.rst autodocs SampleImportSpec and SampleImportJob, but SampleImportStatus and SampleImportJobId — both now in flowbio.v2.__all__, and both named in the prose (job.status, the job.id you pass to get_import) — have no entry, so there's nothing for :class: to link to. Also, the library polling example loops while job.status == "RUNNING": time.sleep(5) with no bound, which is the one thing the CLI section is careful to give a deadline; a sentence noting that a real caller should cap it would keep the two consistent.
7. execution_id/error are required keys on SampleImportJob (low)
Neither has default=None (samples.py:191-192), so a server response that omits them rather than sending null raises ValidationError — uncatchable by _dispatch. This matches MetadataAttribute.options in the same file, so it's consistent rather than a regression, and your round-2 argument about model-level validation being a _main.py concern applies. Noting only because these two are the fields most likely to be omitted by a serializer that skips nulls, and default=None costs nothing.
1 and 2 are the ones I'd want before merge — both are the same recurring theme, that a user who hits an unexpected server shape can't tell "safe to retry" from "you'll create duplicates", and both are now one step from closed. 3–7 are mechanical.
Setting those aside, the slice is in good shape: the up-front validation contract holds, the poll loop is correct including the clamped final sleep, the failure taxonomy (failed/running/unknown) is a genuine improvement over where round 1 started, and the test suite covers every branch of it. Worth noting for the record that I still haven't been able to execute the tests in any round — the 360-passing claim is unverified from my side.
• branch implement/FLOW-689-samples-import-command
|
This is round 4 of review, and I've hit my 3-round fix-and-re-request cap, so I'm stopping here and handing this to @mhusbynflow for the final call rather than pushing another round of changes. What's resolved across rounds 1–3: the core Unresolved from this round (findings 1–2, the ones flagged as wanted before merge):
Findings 3–7 (a looser-than-ideal test assertion, a repeated per-row message, a Over to you — happy to take another pass at 1–2 specifically if you'd like this iterated further, or this can go in with those noted as known gaps. |
|
Ok, a few generic comments before I start my review:
|
Per operator direction on the PR: - Drop all client-side pre-validation of accession sheets (accession format, duplicates, required/closed-option metadata). The API already validates every one of these server-side; duplicating the rules client-side was a second, driftable copy of the same logic for no benefit now that the command doesn't need a local reject/skip decision. A malformed sheet surfaces as a normal FlowApiError instead of a local pre-flight rejection — 'samples import' makes exactly one API call. - Make 'samples import' non-blocking: it submits the whole sheet as one batch, reports the created job's id/status, and returns immediately instead of polling to completion. Add a new 'samples import-status --job-id ID' verb (a thin, one-shot wrapper over client.samples.get_import) so a user (or script) can check on a job whenever they choose; there is no built-in polling. This removes _poll_job, the failed/running/unknown outcome taxonomy, job_sample_ids, the positional accessions/sample_ids matching and its mismatch handling, --skip-invalid/--poll-interval/--timeout, and the accession-format regex and duplicate-detection helpers — all of which existed to support blocking-with-local-validation behaviour that no longer applies. flowbio/v2/samples.py (import_samples/get_import) is unchanged; only the CLI orchestration on top of it is simplified. Refs FLOW-689.
|
Makes sense on both counts — pushed a rework:
This also let me delete essentially all of the outcome-matching complexity from the last few rounds (the Updated the PR description to match, and pushed. Ready whenever you want to take a look. |
|
@tessl-change-review |
|
@claude please review this PR |
|
Claude finished @mhusbynflow's task in 5m 15s —— View job Review:
|
|
Good catch on 1 (leftover from the pre-rework design) and 2 is a fair call to make explicit. Addressing 1–7 now. |
- Revert flowbio/cli/_sheet.py to master: the metadata_errors extraction existed to be shared with accession-sheet validation, which the non-blocking rework removed entirely. Its only caller is validate_row again, so the helper goes back to being private (_metadata_errors), and the stale docstring claiming a shared caller that no longer exists is gone with it. - samples import-status now exits RUNTIME for a FAILED job (was always SUCCESS), so a caller polling it can branch on the exit code alone without parsing --json output. - Drop the client-side accession.upper() call, the only remaining rewrite of user input left over from the pre-rework design. The server already normalises case itself; letting it do so is more consistent with letting the API validate than silently uppercasing client-side first. - samples import now rejects a header-only sheet locally (CliUsageError, exit 2) instead of spending a round trip to be told by the API that at least one accession is required -- this is a structural check (does the sheet have anything to submit), not a validation-rule duplication. - Documented the utf-8-sig BOM-handling rationale in _accession_sheet.py (previously only in _sheet.py, now that the two parsers aren't sharing code). - Docs: replaced the ambiguous "exit 5 or 1" with the one code this endpoint's 422 responses actually map to; added a bounded polling example to the library docs; added API Reference entries for SampleImportStatus and SampleImportJobId; gave SampleImportSpec's fields rendered descriptions (:members: plus :param: docstring, matching ClientConfig's pattern); fixed a stray single/double-backtick inconsistency; documented import-status's new exit-code contract with a --json/jq polling example. Verified the Sphinx build renders all of this without new warnings (a pre-existing, unrelated legacy.rst directive error aside). - Polish: extra blank lines, and _job_summary no longer renders an empty 'Sample ids: .' for a completed job with none. Addresses Claude's round-5 review on flowbio#18. Refs FLOW-689.
|
@claude please review this PR |
|
Claude finished @mhusbynflow's task in 4m 57s —— View job Review:
|
| Commit | Change | Status |
|---|---|---|
a607b79 |
_check_headers rejects unnamed columns by 1-based position (_accession_sheet.py:150-167), so a trailing comma in the header can no longer become a metadata attribute named "" — the round-19 finding, fixed by rejecting rather than ignoring, per your direction |
✅ |
a2e1f1a |
Duplicate column names rejected too, with both problems reported in one error (test_unnamed_and_duplicated_columns_reported_in_one_error) — the right call, since csv.DictReader zips duplicates last-wins and would silently discard the earlier column |
✅ |
d6da4a7 / e40eb36 / 34ec69b |
Row width: a too-wide row is rejected only when the overflow carries a value, a short row always. test_shifted_right_row_with_a_real_value_is_usage_error_not_skipped (,,,ERR1) versus test_wholly_blank_row_with_extra_trailing_comma_is_still_skipped (,,,) is exactly the pair that pins the distinction — nice test, since those two records differ only in the overflow cell |
✅ (finding 1) |
3b2382a |
_import_spec_fields is asdict + one rename + a filter (flowbio/v2/samples.py:527-533); the name=""/organism_id="" omission the refactor introduces is deliberately pinned by test_empty_string_optional_fields_are_omitted_not_the_required_ones |
✅ (finding 3) |
0bbee72 |
setup.py:8 → 0.10.0. Grepped the tree: no __version__, and source/conf.py sets no version/release, so setup.py is the only place and the bump is complete |
✅ |
Header-name trimming composes correctly with both new checks: " name" and "name" now collide as duplicates rather than one silently shadowing the other, and a " "-only header strips to "" and lands in the unnamed bucket.
Findings
1. The docs promise a comma-only trailing line is skipped, but it's only skipped when its comma count happens to match the header
cli.rst:411-413: "A row with every cell blank (e.g. a trailing comma-only line some spreadsheet exports leave below the data) is skipped rather than treated as a row missing values." The width check runs before _is_blank_row (_accession_sheet.py:114-120), so for a four-column header:
| Trailing line | Cells | Outcome |
|---|---|---|
,,, |
4 | skipped ✅ |
,,,, |
5, blank overflow | skipped ✅ (e40eb36) |
,, |
3 | exit 2, data row(s) 501 has a different number of cells than the header |
All three are the same artefact — a comma-only line below the data — and the third one fails an otherwise-correct 500-row sheet while pointing the user at a line they see as empty. Rejecting a short row is your explicit direction and is right for a row that has data (its missing trailing cells genuinely can't be attributed). The blank subset is the one case where that rationale doesn't apply: there's nothing to attribute, which is precisely why the wide-blank variant is already skipped.
The fix keeps the shifted-right case rejected: make blankness account for the overflow (record.get(None)) and run the blank skip before the width check — ,,,ERR1 still fails because its overflow carries a value, which is what test_shifted_right_row_with_a_real_value_is_usage_error_not_skipped exists to pin. If you'd rather keep the strict short-row rule, then the docs sentence needs narrowing, because as written it's true only by coincidence. Fix this →
2. Short and over-wide rows share one bucket, so the error can't say which — and half the remedy is always wrong
wrong_cell_count collects both conditions (_accession_sheet.py:114-118), and the message is built from one clause plus a fixed remedy (:141-145):
Accession sheet data row(s) 1 has a different number of cells than the header: ./accessions.csv.
If a value legitimately contains a comma, quote it; otherwise remove the extra cell(s), or fill in the missing one(s).
The design distinguishes the two cases carefully — short is always fatal, wide only with a non-blank overflow — and the user-facing message erases that distinction. For a short row, "if a value contains a comma, quote it" is advice that can only ever produce the opposite problem, and "remove the extra cell(s)" names a cell they don't have. Two lists (short_rows, overflow_rows) give each its own clause and its own remedy, and the row numbers stay per-cause — which matters here, because with a mixed sheet the user currently can't tell which of the listed rows is which kind. Nothing tests a sheet containing one of each. Fix this →
3. asdict makes the dataclass's field set the wire contract, which flips the payload from an allow-list to a deny-list
fields = asdict(spec)
organism = fields.pop("organism_id")
optional = {"name": fields.pop("name"), "organism": organism, "metadata": fields.pop("metadata")}
return {**fields, **{key: value for key, value in optional.items() if value}}flowbio/v2/samples.py:527-533. The simplification is a clear win over the field-by-field build, and the empty-string behaviour change is tested. The consequence worth recording: only the three named fields are filtered or renamed — everything else in fields goes to the API verbatim under its Python attribute name. So adding, say, library_strategy: str | None = None to SampleImportSpec would immediately start sending "library_strategy": null on every request, with nobody having touched this method. Previously an unwired field simply wasn't sent.
Not a defect, and I'm not relitigating asdict — but the rule is now implicit where it used to be spelled out in code, so it's worth one line of docstring ("every field is sent under its own name; organism_id is renamed and the three optional fields are omitted when empty"), or dropping None values across the whole dict so a new optional field defaults to being omitted rather than to being sent as null.
4. Minor
_is_blank_row(record, headers: Sequence[str])(_accession_sheet.py:182) takes aSequence[str]whereheadersandmetadata_columnsarelist[str]at every other boundary in the module (_build_row,_row_reason_clause). One or the other.- Four lines overshoot the file's ~88-char wrap by 7–10 characters (
_accession_sheet.py:115,:129,:130,:135, plussamples.py:530). No linter runs in CI — there's no lint config in the repo at all — so purely cosmetic. cli.rst:406-407— "a purely blank overflow … is dropped silently" reads as the row being dropped; it's the extra cells that are. "…is ignored" would be unambiguous.- The bullet list is a genuine improvement over the prose paragraph it replaced — four structural rules, each with the artefact that produces it. Worth noting that it's now the only place a user can learn the sheet's shape, since there's deliberately no
import-template; the copy-pasteableaccessions.csvblock at:446-452carries the rest.
5. Standing items, unchanged and not reopened
SampleImportSpec as a frozen dataclass rather than a pydantic model (and the hash(spec) TypeError for any spec carrying metadata); _import_spec_fields's unparameterised -> dict; sample_ids: list[int] vs Sample.id: str; organism_id renamed on the row→spec→wire hop; _job_id accepting negatives; SampleImportStatus as a closed Literal; id/status as the only required fields on SampleImportJob; case-sensitive reserved-column matching and a non-UTF-8 CSV raising UnicodeDecodeError rather than exit 2 (both identical to _sheet.py); import_samples([]) making a request for something the client can see is empty; import --json not reporting how many rows were submitted (a key would break the model_dump purity you asked for); the missing-column message enumerating every row (waived); and the import-template follow-up — still reads1/reads2/project shipping as metadata from a reused batch-template sheet, now joined by the annotation-column question you traced through flow-api (a <id>__annotation column is silently ignored, and combining it client-side needs to know allow_annotation, which is the pre-validation call the redesign removed).
Nothing here blocks merge, and there's no correctness bug in the command logic. 1 is the one I'd want a decision on — the code and the docs disagree about the most common spreadsheet artefact there is, and either half can move. 2 is a message-quality fix on an error the user is meant to act on. 3 is a note for the record; 4 is polish.
On the shape of the thing: _import_command is a parse, one comprehension, one API call and a model_dump; _accession_sheet.py is a CSV read plus structural preconditions; import-status is a fetch plus a projection. test_sends_every_row_without_local_validation still pins "the API validates" as an executable property, test_parses_job_with_only_id_and_status pins tolerance of a minimal response, and the timestamp tests pin the one computation the client performs on a server value. This round's additions are all in the "structurally unresolvable, so reject" category the module docstring now states explicitly — which is the right line to have drawn.
• branch implement/FLOW-689-samples-import-command
Round-20 automated review: the row-width check ran before the blank-row check, so a trailing comma-only line below the data (e.g. spreadsheet noise) was silently skipped at the header's exact width or wider, but became a hard usage error if it happened to have fewer commas than the header — contradicting the docs, which promise any wholly-blank line is skipped regardless of shape. Reordered so the blank check (now aware of the overflow too) runs first; a short row is still rejected outright if it has any data, and a shifted-right row with a real value in its overflow is still rejected, same as before. Also split the combined 'wrong cell count' error into distinct short-row and over-wide-row buckets, each with its own clause and its own remedy — previously a short row's error carried remove-the-extra-cell advice that could never apply to it, and there was no way to tell which kind of row was which in a mixed sheet. Plus: a one-line docstring on _import_spec_fields recording that it sends every dataclass field verbatim except the three it renames/filters (so a newly-added optional field isn't silently sent as null without a second thought); 'is dropped silently' -> 'is ignored' in cli.rst (was ambiguous about whether the row or the cell was being dropped); trimmed an overly-mechanical comment the operator flagged as unnecessary. Linear: FLOW-689
|
Thanks for round 20 — addressed in Finding 1 (docs/behaviour mismatch on blank trailing lines): fixed in code, not docs. Reordered the checks so a wholly-blank row (now checking the overflow too) is skipped before the width check runs, so Finding 2 (mixed bucket, wrong remedy): split into Finding 3 ( Finding 4 (minor): Also addressed @mhusbynflow's inline note on the same commit — trimmed the loop comment down to the one non-obvious sentence. 388 unit tests pass (+1), pyflakes/mypy clean, docs rebuild clean. |
|
@tessl-change-review |
|
@claude please review this PR |
|
Claude finished @mhusbynflow's task in 6m 12s —— View job Review:
|
| Ask | State |
|---|---|
| Reject an unnamed header column | _check_headers (_accession_sheet.py:157-174) rejects unnamed and duplicate columns in one error, each with its own remedy clause, and skips empty names when computing duplicates so a trailing-comma header isn't reported twice. Six tests at sheet level, two at CLI level |
Simplify _import_spec_fields with dataclasses.asdict |
asdict + one pop per optional field (samples.py:537-540); the docstring records the non-obvious part (a new field ships automatically unless added to optional), and test_empty_string_optional_fields_are_omitted_not_the_required_ones pins the falsy filter |
| Bump the version for publishing | setup.py:8 → 0.10.0, and grep confirms that's the only place a version is declared (no __init__ or conf.py copy to drift) |
The short/over-wide/blank-row trio also reads correctly end to end, which is the part I checked hardest since it's the newest logic: overflow is detected before the blank-row check (so a shifted-right row whose data landed in the restkey isn't mistaken for noise), the short-row check runs before the overflow check (so a row can't be both), and _is_blank_row treats a short row's None cells as blank (so a comma-only line narrower than the header is still skipped). All four combinations are pinned by tests, including the two that pull in opposite directions (test_shifted_right_row_with_a_real_value_is_usage_error_not_skipped vs test_wholly_blank_row_with_extra_trailing_comma_is_still_skipped).
Findings
1. A sheet with no header row is diagnosed as every row having "more cells than the header"
headers = [h.strip() for h in reader.fieldnames or []] (flowbio/cli/_accession_sheet.py:100) yields [] when csv.DictReader finds no header — which happens for an empty file and for a file whose first line is blank, since csv returns [] for a blank line and DictReader.fieldnames takes it as the header. _check_headers([]) finds no unnamed and no duplicate columns and returns, so with fieldnames == [] every cell of every data row lands under the restkey. Result, for a file that starts with a stray newline:
Accession sheet data row(s) 1, 2 have more cells than the header: ./accessions.csv.
If a value legitimately contains a comma, quote it; otherwise remove the extra cell(s).
It errors, which is your stated bar for the missing-column case — but here both the diagnosis and the remedy point away from the actual problem (there is no header row at all), and following the remedy makes the file worse. The empty-file path is fine, because it reaches the "no rows" check instead; it's specifically header-less but data-bearing that misreports. A if not headers: raise CliUsageError(...) guard in _check_headers covers both, and it's the same shape as the unnamed/duplicate checks already there. Nothing tests it either way. Fix this →
2. The record annotations now contradict the logic built on top of them
_cell(record: dict[str, str], …) (:201) and _is_blank_row(record: dict[str, str], …) (:189) say the values are str. The parser now depends on both ways that's false:
if any(record.get(header) is None for header in headers)(:113) — the short-row check exists precisely becauseDictReaderfills missing cells withrestval, i.e.None.record.get(None) or [](:107) — the overflow check reads alist[str]stored under aNonekey.
In earlier rounds this was a cosmetic nit (the None-ness was incidentally absorbed by (record.get(column) or "")); now two of the four structural rules are built on shapes the annotation denies, so a reader who trusts the type can't follow the code. CLAUDE.md's "type the actual shape" applies directly. A small alias (ParsedRow = Mapping[str | None, str | list[str] | None]) makes both facts visible, and pulling record.get(None) into a named _overflow_cells(record) would also get line 107 (92 chars) back under the file's wrap. Fix this →
3. _import_spec_fields is now the only unparameterised dict return in SampleResource
-> dict (flowbio/v2/samples.py:529). The reason this was left alone in earlier rounds was consistency with the neighbour — but the neighbour is _build_sample_fields(...) -> dict[str, str] (:592-598), i.e. already parameterised, so the argument now points the other way (the remaining bare dicts in the file are parameters on _resolve_options/_create_metadata_attribute, which are pre-existing and untouched here). asdict returns dict[str, Any], so annotating dict[str, str | dict[str, str]] type-checks without a cast. Fix this →
4. The short-row remedy tells the user to do the one thing that isn't the fix
"fill in the missing cell(s), or remove the row" (:146). The likeliest way to write a short row is the one cli.rst itself names — "trailing optional columns omitted entirely, rather than left as empty cells" — i.e. a hand-authored ERR1,rna_seq under an accession,sample_type,name,organism header, where the user deliberately has no name or organism. For them "fill in the missing cell(s)" reads as "you must supply a name and an organism", and "or remove the row" as "drop this accession"; the actual fix is to add the trailing commas and leave the cells empty. Since this message exists specifically because you chose rejection over inference, it's worth having it name that option. Fix this →
5. The structural rule list is now maintained in four places
The same four rules (readable .csv; named, unique columns; cell count vs header; accession and sample type present) are spelled out in _accession_sheet.py's module docstring (:9-17), parse_accession_sheet's :raises: (:78-82), _import_command's :raises: (flowbio/cli/_samples.py:631-635), and source/cli.rst. They agree today — I checked each against the code — but the command-level :raises: is a verbatim copy of the parser's, and it's the one that will silently fall behind. :raises CliUsageError: If the accession sheet is structurally invalid (see :func:\parse_accession_sheet`).` keeps the cross-reference without the copy.
6. Minor
_check_headerstakesSequence[str],_is_blank_rowtakeslist[str]for the sameheadersvalue in the same module (:157vs:189);headers/metadata_columnsarelist[str]everywhere else.- Line 166 is ~110 chars and line 127 ~95, where the rest of the file wraps at ~88. No linter runs in CI, so cosmetic.
"; and ".join(remedies)(:170) renders "…give the column(s) a name; and rename the repeated column(s)…" when a sheet has both problems — the semicolon-plus-and reads oddly; a plain". "join would be two sentences like the row-level remedies already are.- The missing-accession/
sample_typeclauses get no remedy sentence while the short/over-wide ones do — defensible (the fix is obvious there), just noting the asymmetry is deliberate rather than missed. - A wholly-blank row is skipped with no advisory. Correct call — there's no data to lose, and
for N accession(s)reflects what was submitted — but it means the only difference between "sheet had 500 rows" and "sheet had 501, one was noise" is that count. Fine as-is; flagging so it reads as chosen.
7. Standing items, unchanged and not reopened
SampleImportSpec as a frozen dataclass rather than a pydantic model (and the resulting hash(spec) TypeError for a spec carrying metadata); sample_ids: list[int] vs Sample.id: str; organism_id renamed to organism on the wire; _job_id accepting negatives; SampleImportStatus as a closed Literal; id/status being the only required fields on SampleImportJob; a non-UTF-8 CSV raising UnicodeDecodeError rather than exit 2 (identical to _sheet.py); case-sensitive reserved-column matching, so Accession errors as "no accession" rather than naming the mis-cased column (waived — it errors); import_samples([]) making a request for something the client can see is empty; import --json not reporting how many rows were submitted (a key would break the model_dump purity you asked for); and the import-template follow-up, now down to reads1/reads2/project plus the annotation-column question you and I traced through flow-api earlier in this thread.
Nothing here blocks merge, and I found no correctness bug in the command logic. 1 is the only one I'd want a decision on — it's the last structural mistake whose message actively misdirects, and it's a one-line guard next to the checks you just added. 2 and 3 are typing accuracy that CLAUDE.md asks for and that this round's own changes made newly relevant; 4 is one sentence on the newest error message; 5–7 are docs duplication, polish and settled items.
On the shape of the thing after twenty rounds: _import_command is a parse, one comprehension, one API call and a model_dump; import-status is a fetch plus a projection; _accession_sheet.py is a CSV read plus four structural preconditions, each with a targeted message. test_sends_every_row_without_local_validation still pins "the API validates" as an executable property rather than a comment, and the header/row-shape rules that grew over the last few rounds are each pinned by a test that would fail if the rule flipped — including the two that pull against each other.
• branch implement/FLOW-689-samples-import-command
Round-20 review, finding 1: a blank first line parses via csv.DictReader
as a present-but-empty fieldnames list, distinct from an empty file
(fieldnames=None). Left unguarded, every data row's cells landed under
the overflow restkey against a zero-column header, so the sheet was
rejected as 'more cells than the header' with a quote-your-commas remedy
that pointed away from the actual problem. Now raised explicitly as
'has no header row' before that can happen; the true-empty-file case is
unaffected and still reaches the existing 'has no rows' check.
Also, from the same round:
- accurate record typing: record's cells really are 'str | None' (None
for a short row) with an overflow 'list[str]' under the restkey None,
which the dict[str, str] annotations on _cell/_is_blank_row/_build_row
denied even though the short/overflow checks built on top of them now
depend on exactly that shape. Introduced a ParsedRow alias and an
_overflow_cells helper so the types match what the code actually reads.
- parameterised _import_spec_fields's return type, now that its neighbour
_build_sample_fields is the only other bare dict return in the class.
- reworded the short-row remedy: the likeliest cause is a hand-authored
row that omitted trailing optional columns entirely (as cli.rst already
says), not one missing a value it was supposed to have.
- collapsed _import_command's duplicated :raises: list into a cross-
reference to parse_accession_sheet, the copy most likely to drift.
- _check_headers's remedy join ('; and') read oddly with two clauses;
now two plain sentences like the row-level remedies.
- _check_headers takes list[str] now, matching every other boundary in
the module.
Linear: FLOW-689
|
Addressed in Finding 1 (real bug): a blank first line was parsing as a present-but-empty header ( Finding 2 (typing accuracy): Finding 3: parameterised Finding 4: reworded the short-row remedy — the likeliest cause is a hand-authored row that omitted trailing columns entirely (as Finding 5: collapsed Finding 6: fixed the 389 tests pass (+1), pyflakes/mypy clean, docs rebuild clean. |
|
@tessl-change-review |
|
@claude please review this PR |
|
Claude finished @mhusbynflow's task in 6m 20s —— View job Review:
|
| Row shape | Outcome | Test |
|---|---|---|
| exact width, blank overflow, all cells blank | skipped | test_wholly_blank_row_with_extra_trailing_comma_is_still_skipped |
| short and wholly blank | skipped | test_wholly_blank_row_shorter_than_the_header_is_still_skipped |
| short with data | fewer cells |
test_row_shorter_than_the_header_is_usage_error |
| over-wide, blank overflow | accepted | test_row_with_a_blank_overflow_cell_is_still_accepted |
| over-wide, overflow has a value | more cells |
test_row_with_more_cells_than_the_header_is_usage_error |
| named cells blank, data in overflow | more cells |
test_shifted_right_row_with_a_real_value_is_usage_error_not_skipped |
The last row of that table is the one that would have been easy to get wrong — checking overflow_has_value before _is_blank_row is what stops a shifted-right row from being mistaken for spreadsheet noise, and it's pinned by a test that says so. _overflow_cells/_cell both narrow off ParsedRow rather than trusting DictReader's annotation, which closes the "optimistic dict[str, str]" note from earlier rounds. _check_headers runs before any row is read, so an unnamed or duplicated column can't reach metadata_columns — the empty-key and last-wins-overwrite hazards from round 19 are both gone by construction. setup.py is the only place a version lives, so the 0.10.0 bump is complete.
Findings
1. A truly empty line mid-sheet doesn't consume a data-row number, while a comma-only line does — so a reported row number can be off
csv.DictReader.__next__ discards records equal to [] (a line with no fields at all) before returning, so enumerate(reader, start=1) (_accession_sheet.py:117) never assigns a number to one. A comma-only line is different: csv returns it as ['', ''], it gets a number, and then _is_blank_row skips it. The two are indistinguishable to a user in a spreadsheet or editor, but:
accession,sample_type
ERR1,rna_seq
← truly empty line
,rna_seq
reports data row(s) 2 has no accession for what the user counts as the third data row, while replacing that line with , reports 3. cli.rst commits to "Rows are counted from 1 for the first data row, after the header", and test_blank_row_in_the_middle_does_not_shift_later_row_numbers deliberately pins the comma-only flavour — so the divergent case is both undocumented and uncovered.
This matters more than it used to because row numbers are now the entire content of four hard errors. Worth noting the fix needs care: DictReader.line_num is assigned before its blank-skipping loop, so it's stale after a skip and can't be used directly — the practical options are to enumerate the underlying csv.reader yourself, or to document that empty lines aren't counted. Given you've already waived precision on the missing-column message, documenting may well be the right call — but silently disagreeing with the docs isn't. Fix this →
2. The message-assembly logic is written twice, and three clause helpers do one job
_check_headers (:171-187) and the tail of parse_accession_sheet (:145-167) each build the same shape independently: filter clauses, "; ".join(...), then capitalise each remedy's first letter with f"{r[0].upper()}{r[1:]}" and append. Two copies of that idiom, differing only in separator (". ".join vs "".join with a leading space) — which is why the two errors punctuate their remedies slightly differently today.
Underneath, _row_reason_clause, _unnamed_columns_clause and _duplicate_columns_clause (:190-216) are the same function three times: join the items, pick a verb by count, interpolate. One _clause(items, verb_pair, template) plus one _reject(path, clauses, remedies) would collapse ~45 lines to ~15 and make the two errors punctuate identically by construction. Nothing is wrong today; it's that the parser has grown five accumulators and two message builders across four rounds of additive fixes, and this is the round where consolidating is cheap. Fix this →
3. The blank-header-row comment narrates flow — the same class you've now trimmed three times
# A present-but-empty fieldnames list means the first line was
# blank, not absent (an empty file gives fieldnames=None instead,
# caught by the "no rows" check below); left unchecked, every data
# row would overflow a zero-column header instead._accession_sheet.py:105-108. The load-bearing "why" is one clause — csv reports a blank first line as [] but an absent one as None — and the rest is what the code would do otherwise plus a pointer at "the check below", which is exactly the "the X below"/"so the rest of … works" form CLAUDE.md rules out and which you've asked to trim on this file three times now.
The ParsedRow comment (:32-34) is the same shape read differently: it documents a genuinely non-obvious external contract, but as a # comment above an alias whose sibling aliases in flowbio/v2/samples.py (SampleImportJobId, SampleImportStatus) use a docstring string literal instead. Moving it to a docstring makes it render in the API docs and stops it reading as commentary on the next line. Fix this →
4. _import_spec_fields relies on three pops inside a dict literal
fields = asdict(spec)
organism = fields.pop("organism_id")
optional = {"name": fields.pop("name"), "organism": organism, "metadata": fields.pop("metadata")}
return {**fields, **{key: value for key, value in optional.items() if value}}flowbio/v2/samples.py:537-540. Correct — dict literals evaluate left to right, so fields is exactly {accession, sample_type} by the time it's splatted — but a reader has to know that rule to see it, and the docstring's promise ("a field added to SampleImportSpec is sent even when unset unless it's also added to optional here") is then enforced by which keys got popped rather than by anything named optional. Naming the set directly is the same length and self-evidently matches the docstring:
fields = asdict(spec)
fields["organism"] = fields.pop("organism_id")
optional = ("name", "organism", "metadata")
return {key: value for key, value in fields.items() if key not in optional or value}Also worth noting asdict deep-copies, so metadata is no longer the caller's dict — a behaviour improvement over the previous hand-built version, and one nothing asserts.
5. Test gap: nothing asserts a quoted comma actually works, which is what the new remedy tells users to do
The over-wide-row error advises "if a value legitimately contains a comma, quote it", and test_sheet_with_both_a_short_and_an_over_wide_row_reports_both asserts that sentence reaches the user — but no test parses ERR1,rna_seq,"liver, left lobe" and checks it lands as one cell. That's the happy-path counterpart of the rule this round added, it's the exact action the message recommends, and it also pins that the reader needs no quoting= configuration. Cheap to add next to the existing over-wide tests. Fix this →
6. The structural rules are now stated in three places
The module docstring (_accession_sheet.py:9-17), parse_accession_sheet's :raises: (:83-87) and cli.rst's bullet list say the same five rules in three different phrasings. The docstring already ends with "See :func:parse_accession_sheet", so it could stop at the principle — structural only, rejected rather than guessed at — and let the function's own contract carry the enumeration. Purely a drift-risk note; all three agree today.
7. Minor
_row_reason_clauserendersdata row(s) 1 has …— the parenthesised plural with a singular verb reads oddly, and it's now in four messages. Consistent and tested, so leaving it is fine; noting it because it's the most-seen string in the feature.- Line 138 (
rows.append(_build_row(...))) is ~94 chars and_check_headers's:172/:173/:180run to ~100-110, where the rest of the file wraps around 88. No lint config exists in the repo, so cosmetic. import_samples([])still posts{"imports": []}; unreachable from the CLI now that a header-only sheet is a usage error. Standing.samples importexits0even if the kickoff response is alreadyFAILED, and the human line then reads "Started import job 42 … (status: FAILED)" —cli.rstcommits to this deliberately and it's very likely unreachable (synchronous rejections come back as422), so it stays a documented contract rather than a defect. Standing from round 19.
8. Standing items, unchanged and not reopened
SampleImportSpec as a frozen dataclass rather than a pydantic model (and the resulting hash(spec)/set(specs) TypeError for any spec carrying metadata, shared with SampleImportJob's list fields); sample_ids: list[int] vs Sample.id: str; organism_id renamed to organism on the wire; _job_id accepting negatives; SampleImportStatus as a closed Literal; id/status being the only required fields on SampleImportJob; a non-UTF-8 CSV raising UnicodeDecodeError rather than exit 2, and case-sensitive reserved-column matching (both identical to _sheet.py); import --json not reporting how many rows were submitted (a key would break the model_dump purity you asked for); the missing-column message enumerating every row (waived); and the import-template follow-up, now reads1/reads2/project plus the newly-documented silently-ignored __annotation columns.
Nothing here blocks merge, and I found no correctness bug in the new structural checks — the six-way row taxonomy is right and each branch is pinned by a test that names the case it's distinguishing. 1 is the only finding I'd want a decision on, because it's a documented promise the code doesn't quite keep and row numbers are now the whole payload of four errors; 2 and 4 are simplifications this round is the natural place for; 3 is the comment class you've now trimmed three times, applied to the two comments left; 5 is one test; 6-8 are notes.
On the shape of the thing: _import_command is a parse, one comprehension, one API call and a model_dump; import-status is a fetch plus a projection; the only client-side logic left is CSV structure, and it's now exhaustively enumerated rather than guessed at. test_sends_every_row_without_local_validation still pins "the API validates" as an executable property rather than a comment, and the asdict change means the wire payload follows the dataclass automatically instead of being maintained field-by-field.
• branch implement/FLOW-689-samples-import-command
Round-20 review, finding 1: csv.DictReader discards a line with no commas at all before yielding it, so it doesn't get a row number, while a comma-only blank line does (and is then skipped by _is_blank_row) — the two look identical in a spreadsheet but count differently, and row numbers are now the entire payload of four errors. Renumbering from the underlying csv.reader would be more correct but is a bigger, riskier change for what the previous round already treated as an acceptable precision tradeoff (the missing-column message enumerating every row is similarly waived); documenting the gap is the proportionate fix here. Added a test pinning it. Also, from the same round: - _import_spec_fields: named the optional/omit-when-empty field set directly instead of enforcing it via which keys got popped out of a dict literal, so it visibly matches its own docstring. - trimmed the two comments the review flagged as narrating flow rather than stating the one non-obvious fact, and moved ParsedRow's doc into a docstring literal to match its siblings in v2/samples.py. - added a test confirming a quoted comma-containing value is parsed as one cell, which is what the over-wide-row error's remedy recommends doing. Left as-is, explained in the PR: consolidating the two message-builders and three clause helpers (finding 2) and shortening the module docstring now that :raises: repeats it (finding 6) — both flagged as 'nothing wrong today', and the last round of this cap window is not the place to start a broader refactor. Linear: FLOW-689
|
Addressed in Finding 1: confirmed the discrepancy — a wholly-empty line (no commas) is discarded by Finding 3: trimmed both comments to their one non-obvious fact, and moved Finding 4: Finding 5: added a test confirming a quoted comma-containing value parses as one cell, which is the exact fix the over-wide-row error's remedy recommends. Findings 2 and 6 — left as-is: both are explicitly "nothing wrong today" per your own review (consolidating the two message-builders/three clause helpers, and shortening the module docstring now that 391 tests pass (+2), pyflakes/mypy clean, docs rebuild clean. This is my third fix round this cycle. If the next review still has open findings, I'll stop iterating here and hand this off to @mhusbynflow rather than continuing further rounds. |
|
@tessl-change-review |
|
@claude please review this PR |
|
Claude finished @mhusbynflow's task in 5m 37s —— View job Review:
|
| Commit | Change | Status |
|---|---|---|
a607b79 |
_check_headers rejects unnamed columns by 1-based position (_accession_sheet.py:169-191) — closes round-19 finding 1, and covers the " "-only header too, since names are stripped first |
✅ |
a2e1f1a |
Duplicate column names rejected, with the DictReader last-wins rationale in the test rather than the code |
✅ |
d6da4a7 / e40eb36 |
Over-wide rows rejected only when the overflow carries a value; a blank overflow is tolerated. test_shifted_right_row_with_a_real_value_is_usage_error_not_skipped vs test_wholly_blank_row_with_extra_trailing_comma_is_still_skipped pin the two sides of that distinction, which is the part that could plausibly have been got backwards |
✅ |
34ec69b / 40caadb |
Short rows rejected outright, except when the row is wholly blank — so a spreadsheet's trailing comma-only line survives whatever width it happens to be | ✅ |
b990dc3 |
reader.fieldnames == [] distinguishes a blank first line from an absent one |
✅ (see finding 3) |
3b2382a |
_import_spec_fields is asdict + one rename + one filter (v2/samples.py:529-541) |
✅ (see finding 2) |
0bbee72 |
setup.py → 0.10.0 |
✅ |
598dd09 |
Docs note that a comma-less line doesn't consume a row number, with test_a_line_with_no_commas_at_all_does_not_consume_a_row_number pinning it |
✅ |
I traced the loop ordering carefully, since the four row-shape rules interact: overflow_has_value → blank-row skip → short → overflow → missing values. The blank-row skip running before both width checks is what makes ,, and ,,,, both work as trailing noise, and overflow_has_value being computed first is what stops a shifted-right row from being mistaken for one. That ordering is load-bearing and all four permutations have a test.
Findings
1. test_row_shorter_than_the_header_is_usage_error has a second, unrelated test fused into its body
def test_row_shorter_than_the_header_is_usage_error(self, tmp_path: Path) -> None:
path = tmp_path / "sheet.csv"
path.write_text("accession,sample_type,name,organism\nERR1,rna_seq\n")
with pytest.raises(CliUsageError, match=r"data row\(s\) 1 has fewer cells than the header"):
parse_accession_sheet(path)
sheet = parse_accession_sheet(_write_sheet(tmp_path, _record(name="liver_r1", organism="Hs")))
assert sheet.rows[0].name == "liver_r1"
assert sheet.rows[0].organism == "Hs"tests/unit/cli/test_accession_sheet.py:55-71. Everything from sheet = parse_accession_sheet(...) onward is the body of a test_name_and_organism_are_parsed — the counterpart to test_name_and_organism_are_optional four tests up (:47) — that 34ec69b inserted the short-row check into the middle of, rather than beside. Both halves pass, so CI can't see it, but a failure in the name/organism assertions is reported under a name about row width, and the parsing test no longer exists under a name anyone would grep for. Splitting it back into two restores both. Fix this →
2. _import_spec_fields' allow-list defaults a newly-added optional field to being sent as null
fields = asdict(spec)
fields["organism"] = fields.pop("organism_id")
optional = ("name", "organism", "metadata")
return {key: value for key, value in fields.items() if key not in optional or value}flowbio/v2/samples.py:537-541. The asdict simplification is the right call and the docstring is honest about the consequence — "A field added to SampleImportSpec is sent even when unset unless it's also added to optional here." But that's an opt-out list, so the failure mode of forgetting it is {"accession": …, "new_field": null} on the wire for every import, which the server may well reject; the pre-asdict version simply didn't send a field until it was wired up.
Inverting it to if value (or if value is not None) defaults the other way and needs no list at all: accession and sample_type are required and non-empty for any spec the CLI builds, so the emitted payload is identical for every case the tests cover, and a future optional field is omitted-when-unset by default. Worth weighing against the one behaviour change — a library caller passing accession="" directly would stop having it sent — which is arguably an improvement anyway. Fix this →
3. A wholly-empty file reports "no rows"; a file whose first line is blank reports "no header row"
b990dc3 added the reader.fieldnames == [] check (_accession_sheet.py:107-108) precisely because None and [] mean different things — but the None case (an empty file) then falls through headers = [], _check_headers([]), an empty loop, and out the sheet_is_empty branch as Accession sheet has no rows (:141-142). Both files have no header; only one says so. test_wholly_empty_file_is_usage_error pins the current wording, so this is deliberate-by-test rather than missed — noting it because the two checks now sit four lines apart and disagree about the same condition. if reader.fieldnames in (None, []) would unify them.
4. The two error builders assemble clauses and remedies twice, in two different idioms
parse_accession_sheet (:143-165) does '; '.join(clauses) then "".join(f" {remedy[0].upper()}{remedy[1:]}." …); _check_headers (:174-185) does '; '.join(clauses) then ". ".join(f"{r[0].upper()}{r[1:]}" …) followed by a manual .. Same message grammar — Accession sheet <clauses>: <path>. <Capitalised remedies>. — built two different ways, with the capitalisation trick written out twice and the loop variable shortened to r in one of them. One helper taking (clauses, remedies, path) would state the message shape once; the four _row_reason_clause/_unnamed_columns_clause/_duplicate_columns_clause builders already feed it cleanly.
5. Small simplification: sheet_is_empty restates the four error lists
sheet_is_empty = (
not rows and not short_rows and not overflow_rows
and not missing_accession and not missing_sample_type
)
if sheet_is_empty:
raise CliUsageError(f"Accession sheet has no rows: {path}.")
if short_rows or overflow_rows or missing_accession or missing_sample_type:
..._accession_sheet.py:137-143. Swapping the two blocks — report the structural problems first, then if not rows: for the empty case — is behaviourally identical (when both hold, the current code already reports the problems, because sheet_is_empty is false) and drops the five-term predicate to one.
6. Minor
_job_idsits between the two_configure_*functions (flowbio/cli/_samples.py:273-277), splitting the pair. Moving it below_configure_import_statuskeeps the parser configuration contiguous._import_status_commandtestsjob.status == "FAILED"twice (:661,:664) — once for the advisory, once for the exit code. A local would say it once._accession_sheet.py:136is ~100 chars where the rest of the module wraps at ~88. No linter runs in CI.- The header errors' remedy text is untested —
test_trailing_empty_header_column_is_usage_errorand its siblings match only the clause, while the row-shape equivalent (test_sheet_with_both_a_short_and_an_over_wide_row_reports_both) asserts both halves. If the remedy sentences are worth writing they're worth oneincheck. data row(s) 1 has fewer cells…conjugates the verb correctly but keeps the(s), which reads slightly oddly in the singular. It's consistent across all four clause builders, so it's a house style at this point rather than a defect.
7. Standing items, unchanged and not reopened
SampleImportSpec as a frozen dataclass rather than a pydantic model (and the hash(spec) TypeError that frozen=True implies for any spec carrying metadata); sample_ids: list[int] vs Sample.id: str; organism_id: str | None rather than a NewType; _job_id accepting negatives; SampleImportStatus as a closed Literal; id/status being the only required fields on SampleImportJob; a non-UTF-8 CSV raising UnicodeDecodeError rather than exit 2 (identical to _sheet.py); case-sensitive reserved-column matching (also identical to _sheet.py); import_samples([]) making a request for something the client can see is empty; samples import exiting 0 on a kickoff response that is already terminal (round-19 finding 2, deferred as likely unreachable); the missing-column message enumerating every row (waived); and the import-template follow-up, now down to reads1/reads2/project plus the newly-documented "annotation columns are silently ignored server-side" — which your flow-api trace makes a genuinely separable design decision rather than an oversight.
Nothing here blocks merge, and there's no correctness bug in the command logic or the sheet parser. 1 is the one I'd fix — it's two minutes and it stops a real test from hiding inside a differently-named one. 2 is a small robustness choice on brand-new code that the docstring already flags as a hazard; 3–6 are consistency and simplification.
On the state of the slice: _import_command is a parse, one comprehension, one API call and a model_dump; import-status is a fetch plus a projection; _accession_sheet.py is a CSV read plus a set of structural rules that each have a test on both sides. test_sends_every_row_without_local_validation still pins "the API validates" as an executable property via respx's unmocked-request failure, and the timestamp tests pin the one computation the client performs on a server value across human, --json and library output alike. Version bumped to 0.10.0, so it's publishable as-is.
• branch implement/FLOW-689-samples-import-command
|
This is round 4 on this cycle, and per my round cap I said I'd stop at 3 and hand this off rather than keep iterating — doing that now, even though finding 1 below is small enough that I was tempted to just fix it. Summary for review: this PR implements Unresolved from this round's review, for your call:
None of these were flagged as correctness bugs or merge-blockers by the reviewer. Over to you, @mhusbynflow. |
|
Do the test hygiene bug, but nothing else. Do not ask claude for another review. |
Round-20 finding 1: an earlier edit inserted the short-row rejection check into the middle of what used to be test_name_and_organism_are_parsed instead of adding a sibling test, fusing an unrelated name/organism assertion onto the end of a differently-named test. Both halves already passed, so nothing was broken, but a name/organism regression would have been reported under a test about row width. Split back into two. Linear: FLOW-689
|
Pushed |
Per operator request: the function had grown to ~90 lines across four concerns (suffix/existence checks, header validation, the per-row classification loop, and building the combined structural error). Extracted the header-validation block into _read_header (returns the header and metadata columns, raising on a missing/blank header row or a header problem) and the post-loop 'sheet has no rows, or N rows failed a check' logic into _check_rows, leaving parse_accession_sheet as the suffix check plus the row loop. No behavioural change: every raised message, accepted/rejected row shape, and existing test is unchanged (392 passed, same as before this commit). Linear: FLOW-689
Per operator review: _read_header, _check_rows, and (necessarily, since _read_header calls it) _check_headers only ever used path to echo it back in their own error message — which is already the file the user passed via --sheet, so printing it back adds nothing. Dropped the parameter from all three and the ': <path>' from their messages. parse_accession_sheet itself still takes and uses path (for the .csv suffix check, existing_file, and opening the file) and still reports it in its own '.csv file' message, which wasn't part of this request. No test asserted on the path appearing in any of these messages, so nothing needed updating; 392 passed. Linear: FLOW-689
|
Pushed 392 tests pass, pyflakes/mypy/docs clean. No further review requested, per your earlier instruction. |
2 participants
What changed and why
Implements Slice 3 of the "Import samples from public repositories" project: a
flowbio samples importcommand that lets a user import samples from public-repository accessions (SRR/ERR/DRR run or SRX/ERX/DRX experiment accessions) instead of uploading files.client.samples.import_samples/get_import(flowbio/v2/samples.py) — newSampleResourcemethods hittingPOST/GET /v2/sample-imports, plus theSampleImportSpec(input) andSampleImportJob(output) types, exported fromflowbio.v2alongside the rest of the public models.flowbio samples importCLI verb (flowbio/cli/_samples.py) — parses a CSV accession sheet, submits every row as one batch, and reports the created job's id/status. Non-blocking: it does not wait for the job to finish.flowbio samples import-status --job-id ID— a thin, one-shot wrapper overclient.samples.get_import, for checking on a job whenever you choose. There's no built-in polling; wrap this in your own loop if you want it.flowbio/cli/_accession_sheet.py(new) — CSV accession-sheet parsing plus per-rowto_spec()construction (parallel to_sheet.pybut with its own reserved columns:accession,name,organism,sample_type— noreads1/reads2/project, since the import API has neither files nor a project field). A missingaccessionorsample_typeis a hardCliUsageErrorlisting every offending row, not a silent skip.tests/unit/v2/test_samples.py,tests/unit/cli/test_accession_sheet.py,tests/unit/cli/test_samples.py).source/v2/samples.rst(new models/methods + a worked library example) andsource/cli.rst(samples importandsamples import-statussections).Design, per operator direction on this PR
Two rounds of automated review (Tessl/Claude) had pushed this toward a much heavier design — client-side pre-validation mirroring
upload-batch(accession format, duplicates, metadata rules) plus a blocking poll loop with afailed/running/unknownoutcome taxonomy for matchingaccessions/sample_idsback to specific rows. The operator redirected this on the PR:samples importmakes exactly one API call (the kickoff); a malformed sheet surfaces as a normalFlowApiError, not a local pre-flight rejection. The only remaining client-side checks are ones the API structurally can't do before receiving anything (the sheet must be a readable.csv, and every row needs an accession and a sample type to mean anything).samples importsubmits the batch and reports the job id immediately; it does not poll to completion.samples import-statusis a separate, one-shot command for checking a job's state — if a caller wants to poll, that's their own loop now.This removed most of what earlier rounds added: the poll loop, the outcome-matching/mismatch handling,
--skip-invalid/--poll-interval/--timeout, and the accession-format/duplicate-detection helpers.flowbio/v2/samples.py(import_samples/get_import) is unchanged throughout — only the CLI orchestration on top of it was simplified.Two further rounds of operator direction tightened this:
accessionbecame a required field (enforced, not silently skipped); JSON output now comes straight fromjob.model_dump(mode="json")instead of a hand-built dict;SampleImportJob'screated/started/finishedaredatetime(normalised to UTC by a model validator) rather than rawinttimestamps; andsample_typeis exclusively an accession-sheet column — there is no--sample-typeCLI flag at all, so a sample type can only ever be supplied per row in the sheet.Structural validation of the sheet has also been tightened across several rounds: every header column must be uniquely named (no blank/duplicate columns), and every data row must have exactly as many cells as the header — a short row is always a usage error (per explicit operator direction, rather than treating its missing trailing cells as blank), and an over-wide row is rejected too unless its overflow is entirely blank (harmless trailing spreadsheet noise).
How it was verified
pytest tests/unit— 387 passed.flowbio samples import --help/flowbio samples import-status --helpsmoke checks.mypy --strictandpyflakeson the touched files: no new errors versus the pre-existing baseline (this repo doesn't run mypy in CI).sphinx -b html— no new warnings.Not done in this PR: routing the acceptance-test driver through flowbio
The ticket's scope also lists routing the acceptance-test driver's
import_by_accessionthrough flowbio. Both triage passes independently flagged this as an optional, separable third slice, outside the ticket's "Done when".I left it out because it lives in
flow-api'sacceptance-tests, a different repo —acceptance-tests/pyproject.tomlpinsflowbiofromgoodwright/flowbio's publishedmasterbranch, so the driver can only route through the new methods once this PR has merged there. I also have no Docker in this session to verify the swap end-to-end. Once this merges, it's a small, self-contained follow-up.Linear
https://linear.app/goodwright/issue/FLOW-689/slice-3-flowbio-samples-import-command