feat(samples): add samples import command for public-repository accessions - #18
feat(samples): add samples import command for public-repository accessions#18mhusbynflow wants to merge 19 commits into
samples import command for public-repository accessions#18Conversation
…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.
|
@tessl-change-review |
|
@claude please review this PR |
|
Claude finished @mhusbynflow's task in 5m 8s —— View job Review:
|
| # | Fix | Status |
|---|---|---|
| 1 | SampleImportJob._assume_utc_if_naive field validator (flowbio/v2/samples.py:222-231); _timestamp_suffix is now a plain astimezone(timezone.utc).strftime(...); test_naive_timestamp_is_treated_as_utc (model) and test_naive_started_is_reported_as_utc_in_json_too (--json → "2024-04-05T19:34:38Z") pin both ends |
✅ partial — see finding 3 |
| 2 | cli.rst:373-375 now reads "applying a default sample type that each row can override" |
✅ |
| 3 | if args.sample_type is None: hoisted outside the comprehension (_samples.py:643-651) — the rule is stated once and the sheet isn't walked when the flag was given |
✅ |
| 4 | assert "data row(s) 2" in result.stderr; test_missing_sample_type_is_usage_error gained @respx.mock + assert route.call_count == 0, matching its siblings |
✅ |
| 5 | The "sheet-level usage errors are only detected after authentication" trade-off is now on the record in cli.rst's exit-code paragraph |
✅ |
| 6 | Left informational, as framed | ✅ |
Two stragglers from earlier rounds also close out: source/v2/samples.rst:172 is now exactly 42 ~ for a 42-character title (^~{43,}$ no longer matches) and the file ends with a newline, and deleting _job_document in favour of model_dump(mode="json") retired the standing JsonValue list-invariance nit — test_json_document_matches_job_shape / test_json_document_reports_job_fields now pin the exact document both verbs emit, so "the document is the model" is guarded by tests rather than by a parallel dict.
Findings
1. --sample-type "" slips past the new conditional-required check and ships an empty sample type
The guard and the consumer disagree on what "no sample type" means:
if args.sample_type is None: # _samples.py:643 — identity
...
sample_type = self.sample_type or default_sample_type # _accession_sheet.py:58 — truthinessSo flowbio samples import --sheet ./s.csv --sample-type "" skips the guard entirely ("" is not None), and every row without its own sample_type is built as SampleImportSpec(sample_type="") and POSTed. The user gets exit 1 and a server-worded rejection of the whole atomic batch, where the identical sheet condition (a blank sample_type cell) correctly falls back to the default — _cell treats empty-after-strip as absent, and test_blank_sample_type_cell_falls_back_to_default pins that.
That's the same "an empty string is not a value the API could ever accept, and the client can see it without asking" rule this PR already applies to accession (round 6 finding 1, then hardened into __post_init__ in round 12) — applied to the cell but not to the flag. Treating an empty --sample-type as absent (so the existing guard fires and names the rows, exit 2) or rejecting it at parse time via an ArgumentTypeError, the way _job_id now does, closes it. Nothing covers it either way. Fix this →
2. A sheet missing the accession or sample_type column entirely enumerates every data row instead of naming the column
Both hard errors are built from per-row numbers with no special case for "all of them":
parse_accession_sheet(_accession_sheet.py:118-125) — a sheet headedname,reads1,reads2,project(i.e. a reusedbatch-templatesheet, the exact mistake the deferredimport-templateticket is about) producesAccession sheet data row(s) 1, 2, 3, …, 500 have no accession: …. The user's actual problem — there is noaccessioncolumn — is never stated, and the message is unreadable at any real sheet size._import_command(_samples.py:644-651) — same shape forsample_type: a sheet with no such column and no--sample-typelists every row rather than saying "nosample_typecolumn and--sample-typewas not given".
This is round-2 finding 8, deferred back when it was one advisory among other output; it's now the entire content of a hard exit-2 error, so it matters more than it did. Two len(missing) == len(records) special cases (or checking the header for the column) cover both. Note test_sheet_with_no_accession_column_is_usage_error asserts only the exit code and the path, so nothing pins the message today. Fix this →
3. The validator guarantees aware, not UTC — so --json can still emit a non-UTC offset while human mode always prints UTC
if value is not None and value.tzinfo is None:
return value.replace(tzinfo=timezone.utc)
return valueflowbio/v2/samples.py:228-230. An already-aware +02:00 value is returned untouched, so:
- human mode →
started 2024-04-05 17:34:38 UTC(_timestamp_suffixconverts), pinned bytest_started_with_non_utc_offset_is_reported_in_utc --json→"2024-04-05T19:34:38+02:00", notZ— and nothing asserts the document on that path, only on the naive one
Nothing is wrong (the instant is unambiguous, and cli.rst promises only "ISO 8601"), but the round-14 note describes the fix as "the model itself always yields aware UTC datetimes", and that half isn't true: a --json consumer bucketing by date or string-comparing two responses can get two representations of the same instant, and a library caller reading job.created.hour gets a local wall-clock hour. value.astimezone(timezone.utc) for the aware branch as well makes the claim literal, makes --json uniformly Z-suffixed, and lets _timestamp_suffix drop its own conversion (and the comment explaining why it can trust the model). Fix this →
4. Two sheet-level tests can be satisfied by the temp path rather than the message — the round-14 fix landed only at CLI level
pytest's tmp_path basename is derived from the test's own name, and both assertions read the full message (which ends in that path):
test_row_with_blank_accession_is_usage_errorusesmatch="row"(tests/unit/cli/test_accession_sheet.py:132) — the path is…/test_row_with_blank_accession0/sheet.csv, so "row" is in the path. The assertion holds even if the message were reduced to just the path.test_usage_error_names_every_row_missing_an_accessionusesmatch="1.*3"(:151) — exactly the incidental-digit problem round 14 fixed fortest_row_missing_sample_type_without_default_is_usage_error;/tmp/pytest-of-runner/pytest-13/…satisfies it, so dropping the row numbers from the message wouldn't fail the test that exists to pin them.
match=r"data row\(s\) 2" and match=r"data row\(s\) 1, 3" pin what the names claim. Fix this →
5. Minor
_job_summary'sFAILEDcomment points at another function. "The error, if any, is on stderr as an advisory (see_import_status_command) rather than repeated here…" (_samples.py:678-680). The "why" — a human shouldn't read the same sentence twice — stands on its own; thesee _import_status_commandpointer is the cross-reference-to-surrounding-flow CLAUDE.md rules out. Same for_timestamp_suffix's "SampleImportJob normalises a naive value to UTC" comment, which finding 3 would delete anyway._cell(record: dict[str, str], …)is optimistic aboutcsv.DictReader. A short data row yieldsNonevalues and a long one puts the overflow under theNonekey as alist[str]. Harmless in practice (metadata columns come from the header, so theNonekey is never read, and(record.get(column) or "")absorbsNone), but the annotation says something the reader can't get.AccessionSheetRowandSampleImportSpecarefrozen=Truedataclasses with adictfield, sohash(spec)/set(specs)raisesTypeError—frozenadvertises hashability that doesn't hold for any spec carrying metadata. Left informational last round; still worth knowing for a public input type.SampleImportJob's field block mixes ~110-char one-liners (started,accessions) with wrapped calls that are shorter (finished,sample_ids). No linter runs in CI.- Sample-type casing is split:
"RNA-Seq"in the docstrings and both.rstfiles,"rna_seq"throughout the tests. Both are passed through and server-validated, so nothing is wrong — it just reads as two conventions for one field.
6. Standing items, unchanged and not reopened
SampleImportSpec as a dataclass rather than a pydantic model; sample_ids: list[int] vs Sample.id: str; _import_spec_fields's bare dict; _job_id accepting negatives; a non-UTF-8 CSV raising UnicodeDecodeError rather than exit 2 (identical to _sheet.py); import --json not reporting how many rows were submitted (now more defensible, since a key would break the model_dump purity the operator asked for); to_spec's ValueError being unreachable from the CLI (the guard exists to name all offending rows, which to_spec can't — finding 1 is the one case where the two disagree); and the import-template follow-up already flagged for Linear.
Nothing here blocks merge. 1 is the one I'd fix first — it's a two-line change, and it's the last place where the client's own "empty means absent" rule is applied inconsistently, turning a local exit-2 error into an opaque all-or-nothing server rejection. 2 is the same message quality problem in both new hard errors and is what a reused batch-template sheet actually hits. 3 finishes the round-14 fix for the two consumers it didn't reach; 4 finishes the round-14 test fix at sheet level; 5–6 are polish and settled items.
On the arc: the operator's two directives plus three rounds of inline comments have left _import_command at one parse, two structural preconditions, one API call and a model_dump, and import-status at 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 across all seven optional fields, and this round's +02:00/naive tests pin the one computation the client still performs on a server value. Everything above is at the edges because there is very little left in the middle.
• branch implement/FLOW-689-samples-import-command
|
This is round 4 of review since my counter last reset (rounds 12–14 were my three fix rounds this cycle), so per my round cap I'm stopping here and handing this to @mhusbynflow rather than pushing another round of changes. What's resolved across rounds 12–14: the operator's 7 inline review comments (required Unresolved from round 15 (findings flagged as wanted first):
Findings 5–6 (a couple of comments that point at other functions rather than standing alone, a slightly-optimistic type annotation on Over to you — happy to take another pass at 1–4 specifically if you'd like this iterated further, or this can go in with those noted as known gaps. |
|
Please continue with the fixes. Just eliminate the possibility of supplying sample type in any other place than the accession sheet. You may ignore finding 2. It is good enough as long as it errors. |
|
Understood — bigger change than a truthiness fix: removing |
Per operator direction: eliminated the possibility of supplying a sample type anywhere but the accession sheet's own column for 'samples import'. - Removed the --sample-type flag from 'samples import' entirely. - AccessionSheetRow.sample_type is now a required SampleTypeId (was optional with a CLI-supplied default). parse_accession_sheet rejects a sheet with any row missing accession or sample_type, naming every offending row for each column independently, the same way it already did for accession alone. - AccessionSheetRow.to_spec() takes no arguments now that there is nothing to resolve a default against. - This fully resolves round-15 finding 1 (an empty --sample-type value slipping past the required check) by removing the flag it depended on, rather than patching the identity/truthiness mismatch. Also fixed two more round-15 findings while in here: - The naive-timestamp UTC validator on SampleImportJob now also converts an already-aware non-UTC offset to UTC (astimezone), not just naive values — so --json and library callers get a uniformly UTC-normalised value, not merely a tz-aware one. Added a --json test for a +02:00 server timestamp. - Tightened two sheet-parsing tests whose match= patterns were satisfiable by digits/words in pytest's tmp_path rather than the actual error message. Left round-15 finding 2 (the missing-column message enumerating every row) as-is per operator direction — it errors, which is sufficient. Linear: FLOW-689
|
Pushed
Also fixed two more items from round 15 while in here:
Left the missing-column message (round-15 finding 2) as-is per your note — it errors, which is sufficient. Also noticed the PR body had dropped its Linear issue marker again — restored it, along with an updated description of the design. Full suite: 363 passed. Unit tests green on 3.11/3.12/3.13; |
|
@tessl-change-review |
|
@claude please review this PR |
|
Claude finished @mhusbynflow's task in 4m 19s —— View job Review:
|
| # | Fix | Status |
|---|---|---|
| 1 | --sample-type "" — the flag no longer exists |
✅ by removal |
| 2 | Missing-column message enumerates every row | ⏭️ waived per your note |
| 3 | Validator now does astimezone(timezone.utc) for aware values too (flowbio/v2/samples.py:220-231); test_non_utc_offset_started_is_reported_as_utc_in_json_too asserts "2024-04-05T17:34:38Z" for a +02:00 input — the right test, since == on two aware datetimes compares instants and would have passed either way |
✅ |
| 4 | match=r"data row\(s\) 2 has no accession" / match=r"data row\(s\) 1, 3 have no accession" |
✅ |
Findings
1. The two required columns are checked in sequence, so a sheet broken in both ways takes two fix-and-rerun cycles
if missing_accession:
raise _missing_column_error("accession", missing_accession, path)
if missing_sample_type:
raise _missing_column_error("sample_type", missing_sample_type, path)flowbio/cli/_accession_sheet.py:109-112. Both lists are fully populated by the loop above, and then only the first non-empty one is reported. A sheet with a blank accession on row 2 and a blank sample_type on row 5 tells the user about row 2, they fix it, re-run, and learn about row 5. A row missing both is appended to both lists and only ever surfaces as an accession problem.
That's the one property this slice has held onto through every redesign — _sheet.py's module docstring states it explicitly for the sibling parser ("Validation then collects every problem on each row up front (FR-028) so a batch never half-uploads before surfacing a fixable mistake"), and the PR description describes this check as "a hard CliUsageError listing every offending row". It holds within a column and breaks across them. Raising once with both clauses ("data row(s) 2 have no accession; data row(s) 5 have no sample_type: <path>") keeps it, and the existing match= assertions on each phrase still pass. Fix this →
2. Four of the sheet-rejection CLI tests can't tell which error fired
TestSamplesImport has five tests for locally-rejected sheets, and they assert at four different strengths:
| Test | Assertion |
|---|---|
test_row_missing_sample_type_is_usage_error |
"data row(s) 2 has no sample_type" in stderr ✅ |
test_header_only_sheet_is_usage_error |
str(sheet) in stderr |
test_sheet_with_only_blank_accessions_is_usage_error |
str(sheet) in stderr |
test_sheet_with_no_accession_column_is_usage_error |
str(sheet) in stderr |
test_mixed_valid_and_blank_accession_rows_is_usage_error |
exit code + call_count == 0 only |
Every CliUsageError from parse_accession_sheet ends in the path, so the middle three are satisfied by any of the four rejection paths — test_sheet_with_no_accession_column_is_usage_error would still pass if the sheet were rejected as "has no rows", and test_header_only_sheet_is_usage_error would still pass if it were rejected for a missing accession. Given finding 1 changes which message fires for which input, these are the tests that should catch a mistake there and currently can't. The first row of the table is the pattern; the sheet-level suite (test_accession_sheet.py) already uses it. Fix this →
3. _missing_column_error reports missing values, not a missing column
flowbio/cli/_accession_sheet.py:116 — the function is called with a list of row numbers whose cell was blank, and its message is "data row(s) … have no <column>". The only input for which the name is literally true (the header lacks the column entirely) is the case you've waived, and it's also the case where the name is most misleading, because the output is a list of every row rather than a statement about the column. _missing_value_error (or _blank_cells_error) describes what it does at both call sites. Cosmetic, but it's a two-week-from-now readability cost for a one-word change.
4. __post_init__ now carries two invariants, both raising bare ValueError
flowbio/cli/_accession_sheet.py:42-46. This is the right place for them — it's what makes "an AccessionSheetRow has an accession and a sample type" true by construction rather than only true of rows that came through the parser, and test_row_rejects_empty_accession_by_construction / test_row_rejects_empty_sample_type_by_construction pin both. Worth knowing the consequence: these are the only raises in flowbio/cli/ that aren't CliUsageError, so if anything ever constructs a row outside parse_accession_sheet the failure escapes _dispatch (_main.py:88-96) as a traceback rather than exit 2. Not reachable today.
5. Docs: the sheet's header is described in prose but never shown
source/cli.rst's samples import section names the columns correctly ("required accession/sample_type columns, plus optional name/organism and metadata columns") and explains why there's no --sample-type. What it doesn't have — and can't get from a template verb, since there deliberably isn't one — is a copy-pasteable example:
accession,sample_type,name,organism
ERR1160845,RNA-Seq,liver_r1,Hs
ERR10677146,RNA-Seq,,
Three lines directly above the existing worked example would make the required header unambiguous, and both required columns being blank-intolerant now makes getting the header right the single most likely first-run failure. (Related: the docstring examples in flowbio/v2/samples.py and both .rst files use "RNA-Seq" while the whole test suite uses "rna_seq" — both are passed through and server-validated, so nothing is wrong; it just reads as two conventions for one field.)
6. Minor
specs: list[SampleImportSpec] = [row.to_spec() for row in sheet.rows](_samples.py:635) — the annotation was load-bearing when the comprehension took a default; it's now inferable fromto_spec's return type.import_samples([])posts{"imports": []}(flowbio/v2/samples.py:492-510) — unreachable from the CLI now that a header-only sheet is a usage error, but a library caller gets a server422for something the client can see. Cheap to short-circuit, easy to argue is the server's call.SampleImportSpecandAccessionSheetRowarefrozen=Truedataclasses with adictfield, sofrozenadvertises hashability that doesn't hold —hash(spec)/set(specs)raisesTypeErrorfor any spec carrying metadata. Left informational in round 15; unchanged._cell(record: dict[str, str], …)is optimistic aboutcsv.DictReader, which yieldsNonevalues for short rows and alist[str]under theNonekey for long ones. Absorbed by(record.get(column) or ""), so harmless.
7. Standing items, unchanged and not reopened
SampleImportSpec as a dataclass rather than a pydantic model; sample_ids: list[int] vs Sample.id: str; _import_spec_fields's bare dict; _job_id accepting negatives; a non-UTF-8 CSV raising UnicodeDecodeError rather than exit 2 (identical to _sheet.py); import --json not reporting how many rows were submitted (more defensible than ever, since a key would break the model_dump purity you asked for); and the import-template follow-up already flagged for Linear — note it's now down to reads1/reads2/project, since sample_type is a real reserved column.
Nothing here blocks merge, and there's no correctness bug in the command logic. 1 is the one I'd fix — it's the property this slice has protected through every redesign, and it's four lines. 2 is the test half of the same area. 3–7 are naming, docs and polish.
On the shape of the thing: _import_command is now a parse, one comprehension, one API call and a model_dump; _accession_sheet.py is a CSV read plus two structural preconditions; import-status is a fetch plus a projection. test_sends_every_row_without_local_validation (unmocked-request failure proving no pre-flight call happens) still pins "the API validates" as an executable property, test_parses_job_with_only_id_and_status pins tolerance of a minimal response across all seven optional fields, and the four timestamp tests pin the one computation the client still performs on a server value. Making sample_type sheet-only removed the last flag whose meaning depended on the sheet's contents — which is also what removed round 15's finding 1 outright rather than patching it.
• branch implement/FLOW-689-samples-import-command
|
Thanks — addressing these now. Fixing 1 (combining the accession/sample_type checks into one error so a sheet broken in both columns doesn't take two fix-and-rerun cycles — this is the one that matters, since it's the "collect every problem up front" property this slice has kept through every redesign), 2 (tightening the four CLI tests that can't currently tell which rejection path fired), 3 (renaming the error helper to describe what it actually reports), and 5 (adding a copy-pasteable sheet example to the docs). Leaving 4 and the |
- parse_accession_sheet now raises a single CliUsageError naming both missing accessions and missing sample types when a sheet is broken in both columns, instead of only ever reporting the accession problem and requiring a second run to learn about the sample_type one. This restores the 'collect every problem up front' property this slice has kept through every redesign. - Renamed the error-building helper (_missing_column_error -> _missing_value_clause) to describe what it actually reports (blank cells across a set of rows, not a missing column), and it now builds one clause per column rather than a whole exception. - Tightened four CLI-level sheet-rejection tests that could previously pass for any locally-rejected sheet (they asserted only that the sheet path appeared in stderr, which every parse_accession_sheet error satisfies) to assert the distinguishing phrase instead. Added a test for the both-columns-broken case at both the CLI and parser level. - Added a copy-pasteable accession-sheet example to cli.rst, now that there's no --sample-type flag or template verb to get the header from and both required columns reject a blank cell. - Dropped the now-unnecessary list[SampleImportSpec] annotation (and the import it required) on _import_command's list comprehension. Left as informational/deferred, per the review's own framing: the two ValueError invariants in AccessionSheetRow.__post_init__, the frozen- dataclass-with-dict hashability wrinkle, and import_samples([])'s round-trip to the server for an empty list (consistent with 'let the API validate'). Linear: FLOW-689
|
Pushed
Left 4 (the two Full suite: 365 passed. Docs rebuilt locally to confirm they still render. |
|
@tessl-change-review |
|
@claude please review this PR |
|
Claude finished @mhusbynflow's task in 5m 15s —— View job Review:
|
| # | Fix | Status |
|---|---|---|
| 1 | One loop collects missing_accession/missing_sample_type, and a single CliUsageError joins both clauses (_accession_sheet.py:100-116); test_sheet_broken_in_both_columns_reports_both_in_one_error pins a sheet broken in each column reporting both at once — the collect-every-problem-up-front property now holds across columns, not just within one |
✅ |
| 2 | The four CLI rejection tests now assert distinguishing phrases ("no rows", "data row(s) 1, 2 have no accession", "data row(s) 2 has no accession", "data row(s) 1 has no accession") instead of just the sheet path, so each pins which path fired |
✅ |
| 3 | _missing_value_clause builds one clause per column and the caller joins them — the name now describes what it reports |
✅ |
| 5 | A copy-pasteable accessions.csv block precedes the worked example in cli.rst |
✅ |
| 6 | The inferable list[SampleImportSpec] annotation and its import are gone (_samples.py:635) |
✅ |
Also confirmed the long-running underline straggler: source/v2/samples.rst:172 is exactly 42 ~ for its 42-character title (^~{43,}$ matches nothing in the file).
Findings
1. A wholly-blank data row — what a spreadsheet export appends below the data — now rejects the entire sheet
csv.DictReader skips truly empty lines, but not a comma-only line: ,,,,,, parses to a record whose every value is "". Excel and Sheets routinely emit those below the last real row once a user has clicked around. parse_accession_sheet (flowbio/cli/_accession_sheet.py:100-116) sees accession is None and sample_type is None for that record and rejects the whole sheet with:
Accession sheet data row(s) 501 has no accession; data row(s) 501 has no sample_type: ./accessions.csv.
for a file the user perceives as having 500 rows. Nothing was imported, and the row they're pointed at looks empty in their spreadsheet.
Worth separating from the directive that produced this behaviour: "allowing empty accessions does not make sense and we should enforce accessions to be specified instead of skipping them" is about a row that has data but no accession — a real mistake that must not be silently dropped. A record with no values at all is a different thing: it's the same class as the blank lines csv already discards for you, and skipping it loses nothing (there is no data to lose). Filtering records where every cell is empty before the required-column check keeps the operator's rule intact for every row that says anything, and stops the most common spreadsheet artefact from failing an otherwise-correct sheet.
No test covers a ,,,-style row either way — _import_record always sets at least sample_type, and _write_import_sheet always writes all seven columns. Fix this →
2. Annotation companion columns are neither documented nor tested for the accession sheet
upload-batch's sheet has a documented convention: a <identifier>__annotation column carries the free-text annotation for <identifier>, and _sheet.py validates the pairing (ANNOTATION_SUFFIX, _sheet.py:21, 161-176) while batch-template emits the headers so the sheet round-trips. The accession sheet has neither — an x__annotation column is forwarded as a metadata key named x__annotation, which either works exactly as it does for uploads (likely, since the same server-side metadata handling receives it) or rejects the whole atomic batch.
Your own test fixture hints at the ambiguity: IMPORT_HEADERS includes source__annotation (tests/unit/cli/test_samples.py:929) but _import_record never populates it, so the column is always blank and dropped — the path is present in the fixture and exercised by nothing. Since there's deliberately no import-template to discover the convention from, a sentence in cli.rst (annotation columns work the same way as in upload-batch, and are validated server-side) plus one payload assertion would settle it. If it doesn't work server-side, that's worth knowing before someone adapts an annotated upload-batch sheet. Fix this →
3. Cells are whitespace-stripped, while the docstring and the docs both promise "exactly as entered"
_cell does (record.get(column) or "").strip() (_accession_sheet.py:130-132), so " ERR1160845 " is submitted as "ERR1160845". That's the right behaviour — but parse_accession_sheet's :returns: says "Values are otherwise passed through unchanged, including accession, sent to the server exactly as entered" (:73-75) and cli.rst says the sheet's values are "all sent as-is". Round 5 finding 3 was specifically about the client silently rewriting the accession (upper-casing), and the resolution was to stop rewriting it; the strip survived that pass, so the two statements now overshoot by one transformation. "with surrounding whitespace trimmed" in both places makes them literal — and stripping is worth stating, since it's the reason a copied-with-a-space accession works.
4. parse_accession_sheet reads the whole file into memory and builds rows it may throw away
Two small things in the same loop (_accession_sheet.py:94-117):
records = list(enumerate(reader, start=1))materialises every record before the loop, purely so the "no rows" check can run first. Iterating the reader directly and checkingif not rows and not missing_accession and not missing_sample_typeafter the loop gives the same messages without holding records and rows at once. (_sheet.pystreams the reader into its comprehension, so this is a step away from the sibling parser, not toward it.)if accession is not None and sample_type is not None:(:107) re-states the two conditions tested on the two lines above — the third evaluation of a predicate already decided — and the rows it appends are discarded whenever either list ends up non-empty. Collectingmissingin the loop and building rows in a second comprehension after the guard states each rule once and never builds a row it won't return.
5. Doc/comment nits
:returns:refers the reader onward. "…(other thanaccession/sample_type, which reject the sheet instead — see below)" (_accession_sheet.py:73-74) — the:raises:two lines down already says it, and CLAUDE.md rules out exactly this kind of "the X below" pointer. Dropping "— see below" loses nothing._missing_value_clause's numbering comment is in the wrong function. "Data row 1 is the first row after the header…" (:124-126) explainsenumerate(reader, start=1)(:94), which is 30 lines away in a different function; the clause builder just formats numbers it's handed._import_command's human line readsfor 2 accession(s)— fine, but the docs example shows the same, so a reader can't tell the singular case rendersfor 1 accession(s). Cosmetic.
6. Test observations
test_posts_imports_and_parses_job/test_parses_completed_jobcompare the result against aSampleImportJob(created=1700000000, …)built from the same epoch ints, so both sides go through_normalize_to_utc— those assertions can't see the validator at all. That's fine, becausetest_naive_timestamp_is_treated_as_utcand the four CLI timestamp tests do pin it; noting only so the equality assertions aren't mistaken for coverage of the normalisation.test_row_with_no_accession_column_is_usage_error(test_accession_sheet.py:121-129) asserts only thatCliUsageErroris raised, with nomatch=. Its siblingtest_row_with_no_sample_type_column_is_usage_erroris the same. That's the message you've waived, so a barepytest.raisesis a reasonable place to land — just noting these two are the only rejection tests left without a phrase assertion after round-16 fix 2.- Header-casing (
Accession,Sample_Type) lands in the waived bucket too: every row reports a missing accession and the mis-cased columns ship as metadata._sheet.pymatches reserved columns case-sensitively as well, so this is the repo's existing behaviour rather than something new here.
7. Standing items, unchanged and not reopened
SampleImportSpec as a frozen dataclass rather than a pydantic model (and the resulting hash(spec) TypeError for any spec carrying metadata, shared with SampleImportJob/MultiplexedUpload); sample_ids: list[int] vs Sample.id: str; _import_spec_fields's bare dict; organism_id: str | None rather than a NewType (matches upload_sample's existing signature); _job_id accepting negatives; id/status being the only required fields on SampleImportJob, so a response omitting either raises an uncatchable ValidationError; a non-UTF-8 CSV raising UnicodeDecodeError rather than exit 2 (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 rows (waived); and the import-template follow-up — now down to reads1/reads2/project, since sample_type is a real reserved column and cli.rst says so.
Nothing here blocks merge. 1 is the only one I'd want a decision on before it goes in — not because the current behaviour is wrong on the rule you set, but because a comma-only trailing row is spreadsheet noise rather than a row missing an accession, and failing a 500-row sheet on it is a bad first-run experience with no local signal that it's what happened. 2 is a five-minute confirmation plus a sentence, on the one sheet convention a user can't discover from a template. 3–6 are docs, polish and test notes.
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 two structural preconditions; import-status is a fetch plus a projection. Making sample_type sheet-only removed the last option whose meaning depended on the sheet's contents, and round 16's single-error change restored the collect-everything-up-front property across both required columns. 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 four timestamp tests pin the one computation the client still performs on a server value.
• branch implement/FLOW-689-samples-import-command
|
Thanks — addressing these now. Fixing 1 (skipping wholly-blank rows before the required-column check, so a spreadsheet's trailing comma-only rows don't reject an otherwise-valid sheet), 3 (the "exactly as entered"/"as-is" docstring and docs overshooting by the whitespace-strip), 4 (streaming instead of materialising records, and stating each row's accession/sample_type presence once), and the doc/comment nits in 5. For 2 (annotation companion columns): I checked Pushing shortly. |
- parse_accession_sheet now skips a row whose every cell is blank (the trailing comma-only line a spreadsheet export leaves below the data) instead of treating it as a row missing accession and sample_type — that's spreadsheet noise, not a row with data but no accession, and rejecting a whole sheet on it was a bad first-run experience. A sheet of nothing but blank rows still reports 'no rows', same as header-only. - Documented, based on reading flow-api's _assign_metadata/ SampleType.validate_metadata directly: a <name>__annotation metadata column is not given upload-batch's special handling for samples import — it's forwarded as an ordinary key that matches no real MetadataAttribute, so it's silently ignored rather than attached as an annotation. Added a test pinning the client-side forwarding behaviour (the ignoring itself is server-side and can't be tested from here). - Fixed parse_accession_sheet's docstring and cli.rst, which promised values are sent 'exactly as entered'/'as-is' when cells are in fact whitespace-stripped. - parse_accession_sheet no longer materialises every record into a list before processing (streams the reader directly, matching _sheet.py's style), and states the accession/sample_type presence check once instead of re-testing an already-decided condition. - Moved the row-numbering convention comment to the enumerate() call it actually explains, and dropped a '— see below' cross-reference the following :raises: line already covered. Linear: FLOW-689
|
Pushed
Full suite: 368 passed. Docs rebuilt locally to confirm they still render. |
|
@tessl-change-review |
|
@claude please review this PR |
|
Claude finished @mhusbynflow's task in 4m 37s —— View job Review:
|
| # | Fix | Status |
|---|---|---|
| 1 | _is_blank_row (_accession_sheet.py:103, 126-127) skips a record whose every cell is blank before the required-column check; test_trailing_blank_row_is_skipped and test_sheet_of_only_blank_rows_is_usage_error pin both halves — a comma-only trailing line no longer sinks a valid sheet, and a sheet of nothing but blank rows still reports "no rows" |
✅ |
| 2 | Annotation columns: cli.rst states plainly that <identifier>__annotation gets no special handling here and is silently ignored server-side, and test_annotation_suffixed_column_is_forwarded_as_a_plain_metadata_key pins the client-side half. Checking flow-api's _assign_metadata/validate_metadata rather than inferring it from the fixture was the right move — silent ignoring is exactly the thing a user can't discover from the CLI |
✅ |
| 3 | :returns: now says "with empty cells dropped and surrounding whitespace trimmed" |
✅ / cli.rst phrasing, finding 3 |
| 4 | enumerate(reader, start=1) is consumed directly — the list(...) materialisation is gone, and the "no rows" check moved after the loop |
✅ / |
| 5 | The row-numbering comment now sits above the enumerate it explains (:100-101); the "— see below" pointer is gone from :returns: |
✅ |
Also confirmed the long-running heading straggler: source/v2/samples.rst:172 is exactly 42 ~ for its 42-character title, and both new cli.rst headings match their titles (18/18 at :370-371, 25/25 at :444-445).
Findings
1. Cell values are trimmed but header names aren't — accession, sample_type, name rejects every row of a sheet that plainly has both columns
_cell strips (_accession_sheet.py:139), so " ERR1160845 " works. metadata_columns and every record.get(column) lookup use reader.fieldnames verbatim (:96-99), so a header line written as
accession, sample_type, name, organism
yields fieldnames ["accession", " sample_type", " name", " organism"]. Consequence: accession matches (no leading space on the first column), sample_type doesn't, and the user gets
Accession sheet data row(s) 1, 2, …, 500 have no sample_type: ./accessions.csv.
for a file whose second column is visibly sample_type — while " sample_type", " name" and " organism" are simultaneously forwarded to the server as three bogus metadata keys.
This is _sheet.py's behaviour too, so it's consistent with the repo — but the risk profile is different, and that difference is deliberate in this PR: upload-batch's sheet comes from batch-template, which emits a correct header, whereas the accession sheet is hand-authored by design (cli.rst says so, and the missing-import-template follow-up is already on the ticket). accession, sample_type with a space after the comma is what a person types. The client already normalises whitespace on the value axis; doing the same on the header axis (headers = [h.strip() for h in reader.fieldnames or []], plus looking cells up by the stripped name) closes it in one place. Nothing tests it either way. Fix this →
2. _timestamp_suffix's conversion is now a no-op, and its comment says so — while the model it points at is the thing that made it redundant
def _timestamp_suffix(label: str, timestamp: datetime | None) -> str:
if timestamp is None:
return ""
# SampleImportJob normalises a naive value to UTC, so every timestamp
# reaching here is already aware.
return f" ({label} {timestamp.astimezone(timezone.utc).strftime(...)})"flowbio/cli/_samples.py:686-692. Round 15's finding 3 was resolved by making _normalize_to_utc do astimezone(timezone.utc) for aware values as well — which means every value reaching here is not just aware but already UTC, so the astimezone call can only ever return timestamp unchanged. Either drop the conversion and keep a one-line "why" on the model side, or keep the conversion as belt-and-braces and drop the comment; carrying both leaves a reader unsure which layer owns the rule.
Same file, still open from round 15's finding 5: _job_summary's FAILED comment (:678-680) reads "The error, if any, is on stderr as an advisory (see _import_status_command) rather than repeated here…". The "why" — a human shouldn't read the same sentence twice — stands on its own; the see _import_status_command pointer is the cross-reference-to-surrounding-flow CLAUDE.md rules out. Fix this →
3. Two docs sentences don't quite say what the code does
cli.rst(import section): "The sample type, accession format, and metadata rules are all sent as-is (surrounding whitespace trimmed) and validated server-side" — rules aren't sent; values are, and rules are what's validated. The sentence merges the two halves of the design into one clause that's false on both readings. Splitting it ("values are sent as-is, with surrounding whitespace trimmed; the accession format, sample type and metadata rules are validated server-side") says the same thing correctly._accession_sheet.py's:returns:documents the trim and the dropped empty cells but not the blank-row skip, which is now an observable property of the return value (a 501-row file can yield 500 rows). The module docstring covers it; the function's own contract is where a caller reads it. Fix this →
4. Round-17 fix 4's second half didn't land, and the blank-row skip isn't pinned against renumbering
- The predicate is still stated three times.
if accession is None:/if sample_type is None:/if accession is not None and sample_type is not None:(_accession_sheet.py:107-112), and the row built in that third branch is discarded whenever eithermissinglist ends up non-empty. The streaming half of the finding landed; the "state each rule once" half didn't. Anelse-free variant that appends tomissingand only builds rows after the guard (or a singleif/elif/elsechain) removes the third evaluation. Also:112is ~100 chars where the rest of the file wraps at ~88 — no linter runs in CI, so cosmetic. row_numbercomes fromenumeratebefore the skip, so a blank row in the middle correctly leaves the rows after it numbered as the user sees them in the file. That's the right choice and it's the part a future refactor (filter-then-enumerate) would silently break — buttest_trailing_blank_row_is_skippedonly covers a blank row at the end, where numbering can't drift. One sheet ofvalid / blank / blank-accessionassertingdata row(s) 3 has no accessionpins it, and there's no CLI-level blank-row test at all. Fix this →
5. Minor
_is_blank_rowinspects only the header's columns, so a row whose sole content landed undercsv.DictReader'srestkey(more fields than headers, i.e. a shifted-right row) is silently skipped rather than reported. Vanishingly unlikely, and the alternative (any(record.values()), which sees thelist[str]overflow) has its own edge; noting it only because the skip is now silent by design._cell(record: dict[str, str], …)is optimistic aboutDictReader, which yieldsNonefor short rows and alist[str]under theNonekey for long ones. Absorbed by(record.get(column) or ""), so harmless — but the annotation promises something the reader can't rely on._is_blank_rowtakesSequence[str]whilemetadata_columnsislist[str]at every other boundary; one or the other.__post_init__now enforces two invariants with bareValueErrors (:46-50), the only non-CliUsageErrorraises inflowbio/cli/. Correct place for them, both pinned by construction tests; just means a row built outsideparse_accession_sheetescapes_dispatchas a traceback rather than exit2. Not reachable today.test_row_with_no_accession_column_is_usage_errorandtest_row_with_no_sample_type_column_is_usage_errorare the only rejection tests left without amatch=. That's the message you waived, so a barepytest.raisesis a reasonable landing spot — noting it so it reads as deliberate rather than missed.- Sample-type casing is still split —
"RNA-Seq"in the docstrings and both.rstfiles,"rna_seq"throughout the tests. Both are passed through and server-validated, so nothing is wrong; it just reads as two conventions for one field. SampleImportJob's field block mixes ~110-char one-liners (started,accessions) with wrapped calls that are shorter (finished,sample_ids). No linter in CI.
6. 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 — shared with SampleImportJob's list fields); sample_ids: list[int] vs Sample.id: str; _import_spec_fields's bare dict; organism_id: str | None rather than a NewType; _job_id accepting negatives; id/status being the only required fields on SampleImportJob, so a response omitting either raises an uncatchable ValidationError; SampleImportStatus as a closed Literal; a non-UTF-8 CSV raising UnicodeDecodeError rather than exit 2 (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 — it errors, which you said is sufficient); and the import-template follow-up, now down to reads1/reads2/project.
Nothing here blocks merge, and there is no correctness bug in the command logic. 1 is the only one I'd want a decision on — it's a case where the client's own value-trimming makes the absence of header-trimming surprising, on the one sheet in this CLI that's hand-authored by design, and the resulting error names the very column the user did provide. 2–4 are a redundant conversion, two comments, two docs sentences and two test additions; 5–6 are polish and settled items.
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 two structural preconditions; import-status is a fetch plus a projection. test_sends_every_row_without_local_validation (unmocked-request failure proving no pre-flight call happens) 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 four timestamp tests pin the one computation the client still performs on a server value. Making sample_type sheet-only and skipping wholly-blank rows were the last two behavioural questions; both are now settled in the code and in the docs.
• branch implement/FLOW-689-samples-import-command
|
This is my third fix round this cycle, so addressing everything reasonable in this pass. Fixing 1 (stripping header names, not just cell values — the real one, since it can reject every row of a sheet whose header just has a stray space), 2 (the now-redundant |
- Accession-sheet header names are now trimmed too, not just cell values: reassigning DictReader.fieldnames to the stripped list means a hand-authored header like 'accession, sample_type, name' (a space after each comma) is recognised correctly instead of treating ' sample_type'/' name' as bogus metadata columns and rejecting every row for a missing sample_type it visibly has. - Removed the now-redundant astimezone(timezone.utc) call in _timestamp_suffix (SampleImportJob's validator already guarantees a UTC-normalised value, aware or not, so the call was a no-op) along with its comment, and trimmed _job_summary's FAILED-branch comment to drop its cross-reference to _import_status_command. - Fixed a cli.rst sentence that said sample-type/accession-format/ metadata *rules* are sent as-is, when it's values that are sent and rules that are validated server-side. - parse_accession_sheet's own :returns: now mentions the blank-row skip as an observable property of what it returns, and states the accession/sample_type presence check once (an early continue) rather than a third redundant combined check. - Added tests: a header with spaces after commas, a blank row in the middle of a sheet (proving later rows keep their file-relative row numbers), and a CLI-level test that a trailing blank row is skipped and the remaining rows are still submitted. Linear: FLOW-689
|
Pushed
Left 5–6 as informational/settled per your own framing. Full suite: 371 passed. Docs rebuilt locally to confirm they still render. This is my third fix round this cycle — if the next review still has open findings, I'll stop iterating and hand this to @mhusbynflow per my round cap rather than continuing. |
|
@tessl-change-review |
|
@claude please review this PR |
|
Claude finished @mhusbynflow's task in 5m 5s —— View job Review:
|
| # | Fix | Status |
|---|---|---|
| 1 | headers = [h.strip() …] then reader.fieldnames = headers (_accession_sheet.py:100-101) — the getter consumes the header row first, so the assignment re-keys every subsequent record dict by the trimmed name, and metadata_columns is built from the trimmed list too. test_header_with_spaces_after_commas_is_still_recognised pins accession, sample_type, name resolving correctly and metadata == {}, which is the half that proves the space-prefixed columns aren't shipped as bogus metadata |
✅ (one residual, finding 1) |
| 2 | _timestamp_suffix (_samples.py:679-682) is now strftime only; the redundant astimezone and its comment are gone, and _job_summary's FAILED comment no longer points at _import_status_command |
✅ |
| 3 | cli.rst splits values-sent-as-is from rules-validated-server-side; parse_accession_sheet's :returns: documents the blank-row skip |
✅ (wording nit, finding 3) |
| 4 | The build-and-discard is gone — an early continue means a row is only constructed once both required cells are present (_accession_sheet.py:112-118); test_blank_row_in_the_middle_does_not_shift_later_row_numbers and the CLI-level test_trailing_blank_row_is_skipped both landed |
✅ |
The blank-row skip is the piece I looked hardest at, since it's the newest behaviour: row_number comes from enumerate before the skip, so a blank row in the middle leaves later rows numbered as the user sees them in the file — and that's now pinned by a test rather than left as an accident of ordering. Correct choice, correctly guarded.
Findings
1. An unnamed column (trailing comma in the header) becomes a metadata attribute named ""
metadata_columns is everything not in RESERVED_COLUMNS (flowbio/cli/_accession_sheet.py:102-104), with no check that the header name is non-empty. A spreadsheet that has been clicked around in exports trailing empty columns as well as trailing empty rows:
accession,sample_type,name,,,
ERR1,rna_seq,liver_r1,,,
headers becomes ["accession","sample_type","name","","",""], so metadata_columns == ["", "", ""]. Two consequences, both from csv.DictReader.__next__'s dict(zip(self.fieldnames, row)) — duplicate keys, last one wins:
- A stray value in the last unnamed column is submitted as
metadata: {"": "note"}. The server doesn't have a metadata attribute named"", so the whole atomic batch is rejected with a server-worded error about a key the user never typed. - A stray value in an earlier unnamed column is silently dropped, because a later empty cell overwrites it under the same
""key.
This is the same artefact class round 17 accepted for rows — "a trailing comma-only line some spreadsheet exports append below the data" — applied to the column axis, and the fix is the same shape: if header and header not in RESERVED_COLUMNS. It also composes with the round-18 header trim, since a " "-only header strips to "" and lands in the same bucket. Nothing covers it either way (_write_sheet/_write_import_sheet always pass a fully-named fieldnames). Fix this →
2. samples import exits 0 on a job the server hands back already FAILED, and says "Started import job 42 … (status: FAILED)"
_import_command (_samples.py:636-643) returns ExitCode.SUCCESS unconditionally and interpolates job.status into a sentence beginning "Started". _import_status_command (:663) returns ExitCode.RUNTIME for the identical job state — so the two verbs disagree about the same job, and the human line has the "ended with status RUNNING" flavour that round-1 finding 5 fixed on the other side.
cli.rst does commit to this deliberately ("0 the job was created (regardless of its eventual outcome — check that with import-status)"), and it's probably unreachable in practice — the POST is a kickoff and synchronous validation failures come back as 422, which test_api_rejection_propagates_as_error covers. So this is a judgement note rather than a defect: worth either a sentence confirming the server can't return a terminal status from POST, or reusing _job_summary's phrasing for the non-RUNNING case so the line can't contradict itself. No test asserts what happens when the kickoff response isn't RUNNING.
3. Docs wording
cli.rst, import section: "Every value is sent as-is (surrounding whitespace trimmed, including in header names)" — header names aren't values, and "sent as-is … trimmed" qualifies itself away in the same breath. Round 18 asked for the values/rules split, which landed; this parenthetical now bundles a third, different fact into the same clause. Two clauses ("values are sent as-is apart from surrounding whitespace, which is trimmed; header names are trimmed too") would read cleanly.- The row-numbering sentence doesn't say that skipped blank rows still consume a number. "Rows are counted from
1for the first data row, after the header" is true, and the behaviour (a blank row 2 means the next row is reported as3) is the right one — it's what makes the number match the user's spreadsheet. But it's exactly the detail a reader needs when an error names a row, and it's currently only discoverable fromtest_blank_row_in_the_middle_does_not_shift_later_row_numbers. - Same section has a mid-sentence hard wrap leaving a two-word line ("This command only checks / that the sheet is a readable
.csv"). RST reflows, so cosmetic.
4. Test notes
test_trailing_blank_row_is_skipped(CLI,test_samples.py:1106) asserts the payload but not the count in the human line.Started import job 1 for N accession(s)is the only signal a user gets that a row was dropped, andlen(specs)is what makes it right;assert "for 1 accession(s)" in result.stdoutpins it for one line.test_row_with_no_accession_column_is_usage_error/test_row_with_no_sample_type_column_is_usage_error(test_accession_sheet.py:132,:155) are the only rejection tests still without amatch=. That's the message you waived, so a barepytest.raisesis a defensible landing spot — noting it so it reads as deliberate.test_posts_imports_and_parses_job/test_parses_completed_jobbuild the expectedSampleImportJobfrom the same epoch ints as the mocked payload, so both sides go through_normalize_to_utcand the equality can't see the validator. The four CLI timestamp tests plustest_naive_timestamp_is_treated_as_utcdo cover it — just don't read the==assertions as coverage of the normalisation.
5. Minor
_is_blank_row(record, headers: Sequence[str])(:132) takes aSequence[str]whereheaders/metadata_columnsarelist[str]at every other boundary in the module._cell(record: dict[str, str], …)remains optimistic aboutDictReader, which yieldsNonefor short rows and alist[str]under theNonekey for long ones. Absorbed by(record.get(column) or ""), so harmless — the annotation just promises more than the reader delivers.- Three names for one concept: the sheet column is
organism,AccessionSheetRow.organism,SampleImportSpec.organism_id, and the wire field isorganismagain.organism_idmatchesupload_sample's existing signature, so it's consistent with the file; it just means the row→spec hop renames it and_import_spec_fieldsrenames it back. _accession_sheet.py:118is ~97 chars andSampleImportJob's field block mixes ~110-char one-liners with wrapped calls that are shorter. No linter runs in CI (there's nopyproject.toml/setup.cfglint config at all), so purely cosmetic.__post_init__'s two invariants raise bareValueError(:46-50) — the only non-CliUsageErrorraises inflowbio/cli/, so a row built outsideparse_accession_sheetwould escape_dispatchas a traceback. Right place for the guarantee, not reachable today, both pinned by construction tests.- Sample-type casing is still split —
"RNA-Seq"in the docstrings and both.rstfiles,"rna_seq"throughout the tests. Both are passed through and server-validated.
6. Standing items, unchanged and not reopened
SampleImportSpec as a frozen dataclass rather than a pydantic model (and the resulting hash(spec) TypeError for any spec carrying metadata); sample_ids: list[int] vs Sample.id: str; _import_spec_fields's bare dict; _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; 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 down to reads1/reads2/project, with the newly-documented "annotation columns are silently ignored server-side" belonging in the same ticket, since silent loss of an annotation is the one remaining case a warning-on-unrecognised-columns check would catch.
Nothing here blocks merge, and there is no correctness bug in the command logic. 1 is the only one I'd fix — it's the column-axis twin of the blank-row skip you added two rounds ago, and it's a one-condition change. 2 is a contract question worth one sentence either way; 3–6 are docs, tests and 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 two structural preconditions; import-status is a fetch plus a projection. test_sends_every_row_without_local_validation (unmocked-request failure proving no pre-flight call happens) 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 four timestamp tests pin the one computation the client still performs on a server value. The last two behavioural questions — sample_type being sheet-only, and blank rows being noise rather than errors — are now settled in the code, the tests and the docs alike.
• branch implement/FLOW-689-samples-import-command
|
This is round 4 of review since my counter last reset (rounds 16–18 were my three fix rounds this cycle), so per my round cap I'm stopping here and handing this to @mhusbynflow rather than pushing another round. What's resolved across rounds 16–18: combining the accession/sample_type validation into one error listing every offending row for both columns; tightening several tests that couldn't distinguish which rejection path fired; adding a copy-pasteable sheet example and documenting (verified against Unresolved from round 19 (the ones flagged as wanted first):
Findings 3–6 (a couple of docs wording nits, some test-coverage notes, and several already-settled standing items) are all minor or previously decided and don't need another round on their own. 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. |
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; and, most recently,sample_typeis now 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 (previously it was optional per-row with a CLI-level default, which review found could be bypassed with an empty flag value).How it was verified
pytest tests/unit— 363 passed.flowbio samples import --help/flowbio samples import-status --helpsmoke checks.mypy --stricton the touched files: no new errors versus the pre-existing baseline (this repo doesn't run mypy in CI).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