feat(prepro): Add deacon by maverbiest · Pull Request #6876 · loculus-project/loculus

maverbiest · 2026-07-09T14:50:15Z

Partially resolves #6758

Summary

This PR incorporates Deacon into the preprocessing pipeline. Deacon is used to check submitted raw reads files for host contamination and rejects files where the proportion of raw reads above a threshold configured via preprocessing.configFile.deacon_max_host_proportion in the values.yaml.

Details

Memory cap for preprocessing pods is raised to accomodate loading the 3.1G panhuman index into RAM (set it at 8G for now)
FileIdAndName -> FileIdAndNameAndReadUrl (same as the backend type). This is because prepro now needs to download raw reads from S3 so it needs the URLs
At startup, preprocessing downloads the panhuman-1.k31w15.idx that's needed for host depletion
When preprocessing gets a batch of sequences to process, a deacon server is started. The server is shut down again after a batch is processed. This prevents the 3.1G index from sitting in RAM when it's not used. Downside is that it will need to be read from disk for each batch of sequences (100 seqs for most organisms), which takes ~6 seconds
Deacon filter is run on each (pair of) raw reads file(s), a summary of the run is output as json and loaded in to a DeaconSummary. This information is used for deciding to reject a file or not
A new test file is added to cover the new functionality (Deacon calls are mocked, however)

Manual testing

I monitored the prepro logs in Grafana to confirm that:

Index gets fetched once at prepro startup
Deacon server starts/stops as expected when a batch of unprocessed sequences is received from the backend
Deacon filter runs

I also started a preview to check that:

Raw reads files without host sequences are not rejected
Raw reads files with too many host sequences get rejected
Raw reads file is attached to submission and downloadable from the sequence details page after release

Here, the first submission has file ERR17072040.fastq.gz, which is just west-nile sequences. The second submission (with the error) has the same file where I added 10000 reads sourced from the human reference genome:

When the sequence is released, the raw reads file is accessible on the sequence details page:

PR Checklist

~~- [ ] All necessary documentation has been adapted.~~ -> will be added in #6760

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

Partly resolves #6758 Currently, user-submitted data are not read in by the nextclade preprocessing pipeline, causing them to be dropped form the submission process. Since we're now working to implement file sharing on Loculus, we need to make it so user-submitted files make it through the preprocessing pipeline as well. This PR makes it so user-submitted files are forwarded through the nextclade pipeline without any processing or checking of file contents. Future PRs will add functionality to, for example, check for host sequences in submitted raw reads. - `parse_ndjson` now also parses the file related information sent to preprocessing by the backend - `UnprocessedData` and `UnprocessedAfterNextclade` both get a `files` attribute - test factories in `factory_methods.py` can now be given files to add to test objects, also added a test case in `test_nextclade_preprocessing.py` that carries file information I created a preview to test whether files now make it through the nextclade pipeline. When I submit sequences to the preview with attached raw reads, this file now appears in the submission review page: <img width="1594" height="581" alt="grafik" src="https://github.com/user-attachments/assets/45a2c3a7-ef9a-4738-aec7-be8ef5717a1b" /> And also on the sequence details page after the sequence is released: <img width="1124" height="595" alt="grafik" src="https://github.com/user-attachments/assets/578e16c0-6f0e-4e9b-b5e2-2c544b826086" /> One thing I ran in to when implementing this is that you need to add file categories two times in the config: one time under `submissionDataTypes` (file categories that users are allowed to submit) and then again under a top-level `files` field (files accepted as outputs of prepro pipelines): ``` defaultOrganismConfig: &defaultOrganismConfig schema: &schema submissionDataTypes: &defaultSubmissionDataTypes consensusSequences: true maxSequencesPerEntry: 1 files: enabled: true categories: - name: raw_reads displayName: Raw reads ... files: - name: annotations displayName: Annotations - name: raw_reads displayName: Raw reads ``` Would it be nicer to always allow file categories listed under `submissionDataTypes.files` to be output but prepro? Or will we ever have cases where users submit one thing, prepro processes it, and then outputs another filetype? Probably safest to keep as-is for now but just wanted to flag since not doing this properly got me into a weird state where submissions stay in 'processing' indefinitely but never error because the backend doesn't accept the preprocessing output (it only logs errors in the backend). ~- [ ] All necessary documentation has been adapted.~ - [x] The implemented feature is covered by appropriate, automated tests. - [x] Any manual testing that has been done is documented (i.e. what exactly was tested?) 🚀 Preview: https://pass-files-through.loculus.org

Partially resolves #6758 This PR adds general functionality to validate user-submitted files in the nextclade preprocessing pipeline. How files are processed is determined based on the `FileCategory` and a simple switch statement. The PR introduces a function to validate a submission of raw sequencing reads as a first use case: ```python def validate_raw_reads_submission( files: list[FileIdAndName], ) -> tuple[list[ProcessingAnnotation], list[ProcessingAnnotation]]: ``` The validation is very basic for the moment, and only considers the number of submitted files and their files extensions. This could be built out in the future to include more robust checks. ## Alternative approaches Rather than having a switch statement on `FileCategory` in prepro to determine how different files are processed, we could define this in the `values.yaml`, similar to how we do for metadata fields. E.g.: ```yaml submissionDataTypes: files: enabled: true categories: - name: raw_reads displayName: Raw reads preprocessing: function: validate_raw_reads_submission ... ``` I decided against this for now to get a working version with a minimal diff, but I would be happy to implement this if people feel it's better. ### PR Checklist - [x] All necessary documentation has been adapted. - [x] 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: https://simple-fq-validation.loculus.org

once at startup

maverbiest · 2026-07-13T13:13:02Z

@claude review

claude · 2026-07-13T13:13:16Z

Claude finished @maverbiest's task in 4m 0s —— View job

Claude finished reviewing

Gather context (diff against origin/raw-reads)
Understand the request
Review changed files
Post review feedback

CI is green (preprocessing-tests, which includes ruff + pytest, passed), so this is a design/logic-level review rather than a lint/test-failure review.

Findings

1. Debug-only preview overrides are still in the diff
kubernetes/loculus/values_preview_server.yaml (+21/-1) adds a defaultOrganisms block disabling every organism except west-nile and bumps preprocessing.limits.memory from 3Gi to 8Gi. This matches your own 9f90c06 TO REVERT: ... commit — flagging so it isn't accidentally left in when this PR is merged into raw-reads/main. Might be worth adding this explicitly to the PR checklist.

2. Possible race between starting the Deacon server and using it
start_deacon_server() (file_processing_functions.py:177-185) does a non-blocking subprocess.Popen(...) and returns immediately; prepro.py:752-756 calls it and then immediately calls process_all(...), which can invoke deacon --use-server filter right away. If the server takes any nonzero time to bind before it's ready to accept --use-server calls, the first filter call in a batch could fail with a connection error. Since stdout/stderr of the server process are both sent to DEVNULL, such a startup failure would also be silent and hard to diagnose. Worth double-checking whether deacon's client retries/blocks until the server socket exists, or whether an explicit readiness check/wait is needed here.

3. Only HTTPError is special-cased around the download step; Deacon-filter failures aren't caught at all
In validate_raw_reads_submission (file_processing_functions.py:112-136), only requests.HTTPError from download_file gets a friendly per-file ProcessingAnnotation. Other request failures (timeout, connection reset, etc.) and any failure from run_deacon_filter (which raises a bare Exception on non-zero exit, line 173) aren't caught here — they'll propagate up and be handled by the generic except Exception in process_all's per-entry loop, which just marks the whole entry as failed with a generic message rather than the specific file-related annotation the rest of this function produces. If that's acceptable (i.e., "this should basically never happen") it's fine, but worth confirming it's intentional rather than an oversight, since it's an inconsistent error-handling story within the same function.

4. Duplicated “magic” Deacon index identifiers
config.py's DEACON_INDEX = "panhuman-1.k31w15.idx" and file_processing_functions.py's run_deacon_fetch's hardcoded "panhuman-1" argument are two independent magic strings that must stay in sync (the .k31w15.idx suffix encodes the k/w params deacon chooses for that index name). Not a bug today, but if the index profile ever changes, it's easy for these to silently drift out of sync. Consider deriving one from the other, or at least a comment noting the coupling.

5. Minor: raw-reads-pipeline coverage moved out of test_nextclade_preprocessing.py without a replacement full-pipeline check
The removed cases in test_nextclade_preprocessing.py (e.g. "with file") verified that files survive end-to-end into ProcessedData. The new test_file_processing.py thoroughly covers validate_raw_reads_submission/process_submitted_files in isolation (nice test suite, good use of the mock_deacon fixture), but nothing currently exercises that a validated raw-reads file mapping actually flows through process_single/process_all into the final processed output. Probably fine given the isolated unit tests, just flagging the coverage gap.

Nits

run_deacon_filter raises a bare Exception(message) (file_processing_functions.py:173) while run_deacon_fetch raises RuntimeError for the analogous failure (line 149) — consider using the same exception type for consistency.
The PR description's TODO list ("Implement running Deacon in server mode…", "Implement Deacon server start -> process batch -> Deacon server stop…") both look already implemented by the latest commits (4618873, 319db4d) — might be worth updating the PR body/checklist so reviewers know what's actually still outstanding (currently only "Test that contaminated file gets rejected" seems to remain, and that is covered by test_host_contamination_exceeds_threshold).

Overall this is solid, well-tested work — the new test_file_processing.py suite in particular is thorough (paired reads, extension validation, threshold edge cases, missing-URL handling). The main things worth a second look before merging into raw-reads are the preview-only k8s overrides (item 1) and the server-readiness question (item 2).

corneliusroemer · 2026-07-15T13:50:57Z

+
+# Preview only: run just the west-nile preprocessing pipeline while testing the deacon
+# host-depletion step. Each enabled organism runs its own prepro pod and fetches the
+# ~3.1Gi panhuman index, so disabling the rest keeps startup and memory manageable.
+# Deep-merges `enabled: false` onto the base defaultOrganisms entries.
+defaultOrganisms:
+  cchf:
+    enabled: false
+  cchf-multi-ref:
+    enabled: false
+  dummy-organism:
+    enabled: false
+  dummy-organism-with-files:
+    enabled: false
+  ebola-sudan:
+    enabled: false
+  enteroviruses:
+    enabled: false
+  not-aligned-organism:
+    enabled: false


This would disable all preview organisms but west nile, which we don't want.

So needs to be reverted before going on main

anna-parker · 2026-07-15T13:56:29Z

                    "type": "string",
                    "description": "Minimizer used for nextclade sort (if require_nextclade_sort_match or if segments are to be assigned using nextclade sort)"
                  },
+                  "deacon_max_host_proportion": {


we probably need to configure two thresholds.

Which percentage of reads are flagged

How many bp are flagged

anna-parker · 2026-07-15T13:59:21Z

+
+
+@dataclass
+class DeaconSummary:


its also important to know how many bp were flagged

anna-parker · 2026-07-15T14:17:13Z

+
+
+@pytest.fixture
+def mock_deacon(monkeypatch):


It might actually be good to create a little deacon index and example raw reads for testing. We also do that with nextclade as I noticed if we dont actually run nextclade in tests we miss things

maverbiest and others added 5 commits June 30, 2026 20:42

Enable raw reads sharing by default

d2ad68b

merge conflict

b2ec052

Merge branch 'main' into raw-reads

00c3a91

maverbiest force-pushed the add-deacon branch from d4a87ad to 9c4bd5e Compare July 9, 2026 18:46

maverbiest added 5 commits July 10, 2026 10:35

Exploration of adding deacon

e3ab8bc

Rename

3e6b2ba

More exploring (still rough!)

f05471a

Pass enabled file categories to prepro config. Download deacon index

12e5d7e

once at startup

Only run host check if basic checks pass

6a5b127

maverbiest force-pushed the add-deacon branch from d6caff9 to 6a5b127 Compare July 10, 2026 08:36

Move file processing tests to dedicated file

06278f9

maverbiest added preview Triggers a deployment to argocd and removed preview Triggers a deployment to argocd labels Jul 10, 2026

maverbiest added 2 commits July 10, 2026 15:46

TO REVERT: update deployment to only have west-nile, raise prepro memory

9f90c06

Log deacon filter stderr, switch to --deplete mode

319db4d

maverbiest added the preview Triggers a deployment to argocd label Jul 11, 2026

maverbiest added 3 commits July 13, 2026 12:24

Start deacon server only when needed

4618873

Some robustness

f6f1eed

Refactor, download files as stream

b7c1890

anna-parker reviewed Jul 13, 2026

View reviewed changes

Comment thread preprocessing/nextclade/src/loculus_preprocessing/file_processing_functions.py Outdated

maverbiest added 2 commits July 15, 2026 11:40

Run deacon on both files at once for paired submission

0e1345d

Make host contamination threshold configurable

f468c6d

corneliusroemer reviewed Jul 15, 2026

View reviewed changes

anna-parker reviewed Jul 15, 2026

View reviewed changes

maverbiest removed the preview Triggers a deployment to argocd label Jul 21, 2026

anna-parker force-pushed the raw-reads branch from 00c3a91 to 78175de Compare July 21, 2026 20:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(prepro): Add deacon#6876

feat(prepro): Add deacon#6876
maverbiest wants to merge 18 commits into
raw-readsfrom
add-deacon

maverbiest commented Jul 9, 2026 •

edited by loculus-bot

Loading

Uh oh!

maverbiest commented Jul 13, 2026

Uh oh!

claude Bot commented Jul 13, 2026 •

edited

Loading

Uh oh!

Uh oh!

corneliusroemer Jul 15, 2026

Uh oh!

anna-parker Jul 15, 2026

Uh oh!

anna-parker Jul 15, 2026

Uh oh!

anna-parker Jul 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants



		@dataclass
		class DeaconSummary:

Uh oh!

Conversation

maverbiest commented Jul 9, 2026 • edited by loculus-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Details

Manual testing

PR Checklist

Uh oh!

maverbiest commented Jul 13, 2026

Uh oh!

claude Bot commented Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Claude finished reviewing

Findings

Nits

Uh oh!

Uh oh!

corneliusroemer Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

anna-parker Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

anna-parker Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

anna-parker Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

maverbiest commented Jul 9, 2026 •

edited by loculus-bot

Loading

claude Bot commented Jul 13, 2026 •

edited

Loading