Skip to content

fix(ingest): regroup revoke needs to work when number of sequences is greater than batch size, add tests - #7249

Open
anna-parker wants to merge 13 commits into
mainfrom
fix_regroup_revoke
Open

fix(ingest): regroup revoke needs to work when number of sequences is greater than batch size, add tests#7249
anna-parker wants to merge 13 commits into
mainfrom
fix_regroup_revoke

Conversation

@anna-parker

@anna-parker anna-parker commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

If more than batch_chunk_size records to regroup the new sequences are already POSTed to Loculus by the time the KeyError fires, and it fires before the revoke loop, so zero old accessions get revoked → the mis-grouped originals stay live alongside the new ones. Worse failure than just erroring out early.

Screenshot

PR Checklist

  • All necessary documentation has been adapted.
  • The implemented feature is covered by appropriate, automated tests.
  • Any manual testing that has been done is documented (i.e. what exactly was tested?)

🚀 Preview: Add preview label to enable

Cornelius Roemer and others added 8 commits August 27, 2026 19:41
The header was re-added by checking `record_counter % batch_chunk_size == 1`, which is never true for a chunk size of 1 because `n % 1` is always 0. Every batch from the second one on then went out as a metadata TSV whose first line was a data row.

Whether a batch needs the header does not depend on the chunk size at all - it depends on whether the batch buffer is still empty, which the code already knows. With that, a chunk size of 1 works and the schema minimum can go back to 1.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Xv8GMzy6r4dXGQZpemjAmU
…tters

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Xv8GMzy6r4dXGQZpemjAmU
The tail-chunk submit was guarded by `record_counter % batch_chunk_size != 0`, the same modulo shape as the header bug. At a chunk size of 1 it never fires, so a metadata file with only a header left `response` unbound and the function raised UnboundLocalError. A completely empty metadata file did the same at every chunk size.

Whether a tail chunk needs submitting is again a property of the buffer, not of the counter: submit if anything beyond the header is still unsent. On top of that, submit_or_revise now returns early for zero records instead of sending a request with no data - it already counts the lines and logs "Submitting 0 sequence(s)".

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Xv8GMzy6r4dXGQZpemjAmU
`record_counter` counted lines of the metadata file, so the header occupied tick 1 and every piece of arithmetic derived from it was off by one. That is what forced the two workarounds in the previous commits: the header re-append and the tail submit both had to be phrased as buffer-state checks because the counter could not be trusted.

Counting records instead, and detecting the header by whether we have one yet rather than by the line number, makes the plain modulo arithmetic correct at every chunk size. Batches are now exactly batch_chunk_size records rather than one short for the first batch (2500 records at the default now go out as 1000/1000/500 rather than 999/1000/501), and the logged batch numbers start at 1 rather than 2 when the chunk size is 1.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Xv8GMzy6r4dXGQZpemjAmU
@anna-parker
anna-parker changed the base branch from main to fix-ingest-batch-header-chunk-size September 7, 2026 10:32
@claude claude Bot added the ingest Ingest pipeline label Sep 7, 2026
@anna-parker anna-parker changed the title Fix regroup revoke fix(ingest): regroup revoke needs to work when number of sequences is greater than batch size, add tests Sep 7, 2026
@anna-parker
anna-parker marked this pull request as ready for review September 7, 2026 11:35
@claude

claude Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Claude finished @anna-parker's task in 3m 13s —— View job


Claude Code Review

  • Gather context (read diff + surrounding code)
  • Review loculus_client.py changes
  • Review new tests
  • Post review

The core fix is correct and the diagnosis in the description is right: post_fasta_batches previously kept only the last batch's response, so submission_id_to_new_accessions was missing every submissionId outside the final chunk, and the KeyError fired after the new sequences were already POSTed. Accumulating across batches and pushing .json() down into post_fasta_batches (so submit_or_revise no longer has to unwrap a possibly-None response) is the right shape. Converting the bare json.load(open(...)) to a context manager is a good drive-by.

Four inline comments, in rough priority order:

  1. Test helper writes one base per line (test_regroup_and_revoke.py:82) — extend on a string iterates characters. Harmless but unintended and slow. Fix this →
  2. Abort message is misleading (loculus_client.py:395-404) — the submit has already happened, so aborting here is the "old and new both live" state, not a prevention of it. Either revoke the resolvable accessions before raising, or reword so the operator knows manual cleanup is needed.
  3. Batching test doesn't assert batching (test_regroup_and_revoke.py:160-169) — it would still pass if batch_chunk_size were ignored, and the fake backend ignores sequenceFile, so metadata/fasta misalignment across a batch boundary isn't covered.
  4. Unbounded result accumulation (loculus_client.py:266) — one dict per record is now retained for the whole run, including in the submit/revise path where call_loculus.py:92 discards it. Not a blocker, cheap to bound.

Nothing security-relevant; make_request already raises on non-OK, so failed batches propagate rather than silently producing a malformed results list.

The three tests are well constructed otherwise — building on the real CCHF pipeline output rather than hand-rolled fixtures means they exercise the actual add_seq_to_batch join, the import style matches test_merge_group_overrides.py, and the relative paths line up with how CI invokes pytest tests/ from ingest/.

Note: I was not able to run pytest or ruff check in this environment (commands outside the permitted set), so the review is static only. ingest/ isn't covered by the ruff-format pre-commit hook (it's scoped to ena-submission/ and preprocessing/nextclade/) and ingest-tests.yaml only runs snakefmt, so a few blocks in the new test file are formatted more loosely than ruff format would produce — harmless, just flagging in case you run it locally.
· branch fix_regroup_revoke

Comment thread ingest/tests/test_regroup_and_revoke.py Outdated
Comment thread ingest/scripts/loculus_client.py
Comment thread ingest/tests/test_regroup_and_revoke.py Outdated
Comment thread ingest/scripts/loculus_client.py
Base automatically changed from fix-ingest-batch-header-chunk-size to main September 9, 2026 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ingest Ingest pipeline

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants