Skip to content

feat!(config, prepro, raw-reads-processing): create new raw-reads-processing microservice which handles raw reads validation - #6987

Merged
anna-parker merged 42 commits into
mainfrom
file-validation
Jul 31, 2026
Merged

feat!(config, prepro, raw-reads-processing): create new raw-reads-processing microservice which handles raw reads validation#6987
anna-parker merged 42 commits into
mainfrom
file-validation

Conversation

@anna-parker

@anna-parker anna-parker commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

resolves #6906
partially resolves #6856

This PR moves all download and validation of submitted raw reads from the preprocessing to one raw reads processing service. It is currently disabled by default in the values.yaml and added as an override in the preview and e2e test values.yaml.

We decided to create a separate service as we will additionally check raw reads for human contamination using deacon, see #6916 and deacon requires almost 8G of RAM, we didnt want to add this requirement to each prepro pod so it made sense to have one service that runs deacon. Additionally, it made sense to perform all file validation steps in the same place as downloading files from S3 twice (in prepro and the file-processing service) didnt make sense.

This PR moves code from prepro into this service and additionally wraps the ENA raw read file validation JAR, called by the ENA submission CLI (see our fork: https://github.com/loculus-project/readtools) to ensure that submitted files are in a format accepted by ENA.

Breaking Change

Now, if an administrator would like to allow users to supply raw read input files and use the nextclade preprocessing pod they need to additionally enable the file processing service by adding the following to the values.yaml - this PR enables this on the previews and e2e but not in the default values.yaml so current users of Loculus should not be affected:

disableRawReadsProcessingService: false
RawReadsProcessingService:
  raw_reads_processing_service_url: http://loculus-raw-reads-processing:5000

Screenshot

image image image

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.

Potential Future Issues

  • On the preview I submitted raw read files of invalid formats and confirmed they were rejected, however I also noted that raw reads validation is quite slow for larger files.

  • In my understanding users can still upload an interleaved file which will then be submitted to ENA as not a single (not paired) file in the manifest. 2 interleaved files can be submitted but should still get flagged as they are unlikely to share enough headers.

🚀 Preview: https://file-validation.loculus.org

@anna-parker
anna-parker changed the base branch from main to raw-reads July 25, 2026 12:59
@claude claude Bot added deployment Code changes targetting the deployment infrastructure preprocessing Issues related to the preprocessing component labels Jul 25, 2026
@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

This PR may be related to: #6906, #6758, #6934

@anna-parker
anna-parker force-pushed the file-validation branch 2 times, most recently from f94d050 to ddecc11 Compare July 25, 2026 13:10
@anna-parker
anna-parker changed the base branch from raw-reads to feat/taxonomy-service-config-refactor July 25, 2026 13:10
@anna-parker anna-parker changed the title File validation feat!(config, prepro, file-processing): create new file-processing microservice which handles file validation Jul 25, 2026
Comment thread kubernetes/loculus/values.yaml Outdated
Comment thread preprocessing/nextclade/tests/test_nextclade_preprocessing.py Outdated
@anna-parker
anna-parker marked this pull request as ready for review July 25, 2026 14:18
@claude

This comment was marked as outdated.

@anna-parker anna-parker added the preview Triggers a deployment to argocd label Jul 25, 2026
Comment thread file-processing/src/file_processing/functions.py Outdated
Comment thread file-processing/src/file_processing/functions.py Outdated
Comment thread file-processing/src/file_processing/file_validation.py Outdated
Comment thread file-processing/src/file_processing/functions.py Outdated
@anna-parker
anna-parker marked this pull request as draft July 25, 2026 14:44
@anna-parker
anna-parker marked this pull request as ready for review July 25, 2026 15:24
@claude

claude Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Claude finished @anna-parker's task in 2m 51s —— View job


Review: file-processing microservice

  • Gather context (diff, files, prior review comments)
  • Check whether previously flagged issues were addressed
  • Review remaining changed files (config plumbing, k8s templates, CI workflows, integration tests)
  • Post inline comments for any new/unresolved issues
  • Post summary

All four issues flagged in the earlier review pass have been fixed in 881f413 ("get claude to fix errors reported by claude"):

  • Path traversal via file.name → now sanitized with _sanitize_file_name (strips directory components, rejects empty/./..), with unit tests covering traversal attempts.
  • Early return in process_submitted_files dropping errors from other categories → now accumulates into the outer errors/warnings lists.
  • Missing subprocess.run timeout → run_validation now takes a configurable timeout_seconds, converts TimeoutExpired into a structured Annotation, with a regression test.
  • Narrow except requests.HTTPError → now catches requests.RequestException.

The testing gap is also resolved — file-processing/test/test_functions.py now covers _sanitize_file_name and the download-path-containment behavior of validate_raw_reads_submission, and preprocessing/nextclade/tests/test_file_processing_service.py thoroughly covers FileProcessingService (missing URL, request payload shape, timeout config, success/error/warning mapping, HTTP and network errors).

I reviewed the rest of the diff (k8s templates/values, CI workflows, config plumbing in prepro.py/config.py/backend.py, integration test updates) and didn't find further correctness or security issues — this looks consistent with existing patterns in the repo (e.g. taxonomy-service).

One minor, non-blocking doc nit left inline in file-processing/README.md.

Comment thread file-processing/README.md Outdated
@anna-parker
anna-parker force-pushed the feat/taxonomy-service-config-refactor branch from 79e3a83 to 7048451 Compare July 27, 2026 13:47
@anna-parker
anna-parker force-pushed the feat/taxonomy-service-config-refactor branch from 7048451 to 497a22e Compare July 27, 2026 14:30
Comment thread raw-reads-processing/src/raw_reads_processing/api.py
@anna-parker anna-parker changed the title feat!(config, prepro, file-processing): create new file-processing microservice which handles file validation feat!(config, prepro, raw-reads-processing): create new raw-reads-processing microservice which handles raw reads validation Jul 30, 2026
@anna-parker

Copy link
Copy Markdown
Contributor Author

@claude review

@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

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


Review: raw-reads-processing service

  • Gather context (diff, files, prior review comments)
  • Check whether previously flagged issues were addressed
  • Review new raw-reads-processing service files (functions, validation, config, k8s templates)
  • Review integration test / config plumbing changes
  • Post summary

Previously flagged issues — status

The service was renamed file-processingraw-reads-processing since the last review pass; all four previously-flagged issues carried over correctly and are fixed:

  • Path traversalraw-reads-processing/src/raw_reads_processing/process_files.py:57 now builds the local download path from file.fileId alone (Path(tmp_dir) / f"{file.fileId}"), no longer incorporating the user-controlled file.name at all. Stronger than the suggested fix.
  • Missing subprocess.run timeoutfile_format_validation.py:131-161 (validate_with_readtools) now takes timeout_seconds, converts TimeoutExpired into a ProcessingFailure, and is covered by test_validation_timeout_is_reported_as_error.
  • Narrow except requests.HTTPErrorprocess_files.py:34 now catches requests.RequestException.
  • Doc nit re: READTOOLS_JAR export — moot; the standalone unexported line no longer exists in the new raw-reads-processing/README.md.

Remaining issue: partial fix for the error-dropping early return

preprocessing/nextclade/src/loculus_preprocessing/external_services.py:236-244 (FileProcessingService.process_files):

for category, file_list in files.items():
    if not file_list:
        continue
    if category != FileCategory.RAW_READS:
        message = (...)
        return [self._annotation([file.name for file in file_list], message)]

This still returns on the first non-RAW_READS category encountered with files. Since FileCategory is a plain string alias in the backend (SubmissionTypes.kt:457), a submission isn't restricted to rawReads/annotations — a submission with files in two different unsupported categories will only report an error for whichever one iterates first; the other's error is silently dropped, and any legitimate RAW_READS files present alongside an unsupported category are never sent for validation. This is a much narrower version of the previously-fixed bug (RAW_READS itself can no longer clobber other errors), but consider accumulating into a list across all categories instead of returning on the first hit, for completeness. No test currently covers a mixed-category submission.

Fix this →

Minor doc nits — raw-reads-processing/README.md

  • Line 7: the fenced code block opens with sh `` (two backticks) instead of ```sh — won't render as a code block.
  • "What this service does" references raw_reads_processing.functions.process_submitted_files, but the actual entry point is raw_reads_processing.process_files.validate_raw_reads_submission — looks like a leftover from the rename.
  • The example response ({fileName: <fileName>}) should be fileNames: [<fileName>, ...] (plural list) to match Annotation in datatypes.py.

Everything else

Kubernetes templates/values (raw-reads-processing-deployment.yaml, -service.yaml, -config.yaml, values.yaml, values.schema.json, values_e2e_and_dev.yaml, values_preview_server.yaml), the CI workflows, and the backend/website plumbing (SubmissionTypes.kt, backend.ts) all look consistent with existing patterns (e.g. taxonomy-service), correctly gated behind disableRawReadsProcessingService (default true), and the loculus.resources helper falls back to defaultResources safely since no raw-reads-processing entry is defined yet. Test coverage in raw-reads-processing/test/ (path traversal, timeout, format/readtools validation) and preprocessing/nextclade/tests/test_file_processing_service.py (URL missing, payload shape, timeout, HTTP/network errors) is thorough. The test_nextclade_preprocessing.py merge-conflict content Anna flagged has been correctly moved to test_metadata_processing_functions.py.

One optional, low-priority note: the Dockerfile downloads readtools.jar from a GitHub release URL without pinning a checksum — low risk since it's from the project's own fork, but worth a sha256 pin if you want to harden the supply chain later.
· Branch: file-validation

Comment thread kubernetes/loculus/values.schema.json
Comment thread kubernetes/loculus/values.schema.json
Comment thread preprocessing/nextclade/src/loculus_preprocessing/external_services.py Outdated

@corneliusroemer corneliusroemer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't been able to go through everything in detail, I guess we can always change things later

@theosanderson

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d9157ae6cb

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread kubernetes/loculus/templates/raw-reads-processing-deployment.yaml
Comment thread preprocessing/nextclade/src/loculus_preprocessing/prepro.py Outdated
Comment thread preprocessing/nextclade/src/loculus_preprocessing/config.py Outdated
@anna-parker
anna-parker merged commit 82a452b into main Jul 31, 2026
59 checks passed
@anna-parker
anna-parker deleted the file-validation branch July 31, 2026 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

deployment Code changes targetting the deployment infrastructure preprocessing Issues related to the preprocessing component preview Triggers a deployment to argocd

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MVP: Handle interleaved fastq submissions MVP: Validate fastq files

3 participants