-
Notifications
You must be signed in to change notification settings - Fork 9
feat!(website, prepro, backend, config, integration):multi pathogen - refactor multi segment submission in backend and edit page and have prepro assign segments #5382
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
7b12254 to
1190f91
Compare
There was a problem hiding this comment.
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.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
ace60ff to
680e4b7
Compare
a957c8b to
e74f175
Compare
f27aa67 to
fc9ec01
Compare
fengelniederhammer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On http://localhost:3000/cchf/submission/2/submit?inputMode=form, when I upload the same file twice, then the page crashes:
"Uncaught Error: A sequence with the fastaHeader LOC already exists."
Nice improvement though! (Although I may have preferred a separate PR on top of mine to avoid reviewing my own code in a way here)
873fd80 to
0a27555
Compare
|
@fengelniederhammer I fixed the error now - I will try to add a test for that case tomorrow |
0a27555 to
875921d
Compare
35c8c88 to
5feacae
Compare
resolves #4708, #4734 partially resolves #5392, #5185 (comment) Builds on #5382 ### BREAKING CHANGES When users submit to multi-segmented organisms and want to group multiple segments under one metadata entry they are now required to add an additional `fastaId` column with a space (or comma) -separated list of the `fastaIds` (fasta header IDs) of the respective sequences. If no `fastaId` column is supplied the `submissionId` will be used instead and the backend will assume that (as in the single-segmented case) there is a one-to-one mapping of metadata `submissionId` to `fastaId`. This new submission structure was voted for in microbioinfo: https://microbial-bioinfo.slack.com/archives/CB0HYT53M/p1760961465729399 and discussed in https://app.nuclino.com/Loculus/Development/2025-10-20-Weekly-6d5fe89f-8ded-4286-b892-d215e0a498f6 (and in other meetings) Nextclade sort will be used to assign segments/subtypes for all aligned sequences: ``` minimizer_index: <url_to_minimizer_index_used_by_nextclade_sort> ``` For organisms without a nextclade dataset we still allow the fasta headers to be used to determine the segment/subtype - entries must have the format <submissionId>_<segmentName> (as in current set up). As preprocessing now assigns segments it will return a map from the segment (or subtype) to the fastaHeader in the processedData: `sequenceNameToFastaHeaderMap`. This allows us to surface this assignment on the edit page. ### Testing You can use pathoplexus/dev_example_data#2 for testing. ### Prepro config changes Instead of having a dictionary for the nextclade datasets and servers we make `nucleotideSequences` a list of sequences: ``` nextclade_dataset_name: L: nextstrain/cchfv/linked/L M: nextstrain/cchfv/linked/M S: nextstrain/cchfv/linked/S nextclade_dataset_server: https://raw.githubusercontent.com/nextstrain/nextclade_data/cornelius-cchfv/data_output genes: [RdRp, GPC, NP] ``` ``` nucleotideSequences: - name: L nextclade_dataset_name: nextstrain/cchfv/linked/L nextclade_dataset_tag: <optional - was previously incorrectly placed on an organism level> nextclade_dataset_server: <optional overwrites nextclade_dataset_server for this seq> accepted_sort_matches: <optional, used for classify_with_nextclade_sort and require_nextclade_sort_match, if not given nextclade_dataset_name is used> gene_prefix: <optional, prefix to add to genes produced by nextclade run, e.g. nextclade labels genes as `AV1` but we expect `EV1_AV1`, here `EV1` would be the prefix > - name: M nextclade_dataset_name: nextstrain/cchfv/linked/M - name: S nextclade_dataset_name: nextstrain/cchfv/linked/S nextclade_dataset_server: https://raw.githubusercontent.com/nextstrain/nextclade_data/cornelius-cchfv/data_output ``` Note the templates now also generate the genes list from the merged config. ### PR Checklist - [ ] Update values.schema.json - [x] keep tests for alignment NONE case - [x] Create a minimizer for tests using: https://github.com/loculus-project/nextclade-sort-minimizer-creator - [x] Any manual testing that has been done is documented: submission of EVs from test folder were submitted with the same fastaHeader as the submissionId -> this succeeded, additionally the submission of CCHF with a fastaID column in the metadata was tested (also in folder above), additionally revision of a segment was tested - [x] Have preprocessing send back a segment: fastaHeader mapping ## Future Work - [ ] add integration testing for full EV submission user journey - [ ] improve CCHF minimizer (some segments are again not assigned) - [ ] discuss if the originalData dictionary should be migrated (persistent DB has segmentName as key, now we have fastaHeader as key) - [ ] update PPX docs with new multi-segment submission format 🚀 Preview: https://multi-segment-submission.loculus.org --------- Co-authored-by: Cornelius Roemer <cornelius.roemer@gmail.com>
|
Sorry, but did you just merge #5398 into this PR? My understanding was that we merge those PRs into the feature branch. Now we have one very large PR (I thought we actually wanted to avoid that). |
…s with correct fields (#5561) resolves #5572 Follow up PR with `fastaId` to `fastaIds` change: #5583 ### Screenshot fastaIds field added to template for CCHF: <img width="1672" height="1138" alt="image" src="https://github.com/user-attachments/assets/0dcc1be8-2f01-4205-a819-84ea8055fc5f" /> <img width="1836" height="520" alt="image" src="https://github.com/user-attachments/assets/b41678ca-17c5-41ef-a409-86288f18d124" /> Not added for ebola or EVs: <img width="1728" height="644" alt="image" src="https://github.com/user-attachments/assets/ebcfa88a-e64f-47b1-9fea-e3dfd6bd21a5" /> <img width="1836" height="520" alt="image" src="https://github.com/user-attachments/assets/a77e47ee-8f06-4a29-9eeb-68863ed3dbd0" /> ### PR Checklist - [ ] Make PR with same changes in PPX -> after docs are approved - ~[x] Add fastaIds to commonMetadata fields in config?~ -> fastaIds field does not exist for single segmented organisms and thus should not be added here as this breaks Loculus - [x] Ensure metadata template downloads are correct 🚀 Preview: https://multipath-docs.loculus.org --------- Co-authored-by: Theo Sanderson <theo@sndrsn.co.uk>
partially resolves #5576 ### Screenshot Have segment assignment by nextclade sort and segment assignment via nextclade align use the same code to assign segment for each entry after running nextclade on the batch. I also realized we dont have tests for the `require_nextclade_sort_match` config option - I added them here ### 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: https://multipath-updates.loculus.org
Co-authored-by: Fabian Engelniederhammer <92720311+fengelniederhammer@users.noreply.github.com> Co-authored-by: Cornelius Roemer <cornelius.roemer@gmail.com>
…5559) resolves #5558 🚀 Preview: https://backend-reject-gt1-seq.loculus.org --------- Co-authored-by: anna-parker <50943381+anna-parker@users.noreply.github.com> Co-authored-by: Fabian Engelniederhammer <92720311+fengelniederhammer@users.noreply.github.com> Co-authored-by: Theo Sanderson <theo@sndrsn.co.uk>
resolves #5571 Adds the new config structure to the kubernetes/loculus/values.schema.json. I also renamed `nucleotideSequences` to `nextcladeSequenceAndDatasets` to avoid confusion as preprocessing expects different items in this list of dictionaries. ### Screenshot <img width="1096" height="1398" alt="image" src="https://github.com/user-attachments/assets/02041fb7-2e92-4d14-9bc9-3d1feab0c112" /> ### 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
resolves # ### Screenshot When testing on PPX I see this error caused by #5552 that I sadly missed (string is converted to a list)- this should resolve the bug: ``` INFO:loculus_preprocessing.nextclade:Downloading Nextclade dataset: ['nextclade3', 'dataset', 'get', '--name=nextstrain/rsv/a/EPI_ISL_412866', '--server=https://data.clades.nextstrain.org/v3', '--output-dir=/tmp/tmp6frl2wtu/main', '-', '-', 't', 'a', 'g', '=', '2', '0', '2', '5', '-', '0', '8', '-', '2', '5', '-', '-', '0', '9', '-', '0', '0', '-', '3', '5', 'Z'] error: unexpected argument '-' found ``` ### 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
resolves these comments: #5382 (comment) #5382 (comment) #5382 (comment) 🚀 Preview: Add `preview` label to enable
… and preprocessing
…nomeLightweightSchema
bce2b8a to
2213cec
Compare
…ta field `fastaIds` (#5629) resolves #5627 The `fastaIds` object should contain a space separated list of fasta IDs. The website was constructing `fastaIds` for submissions via the edit page by concatenating full fasta headers of the form `>fastaId description` (and removing `>`) - but this lead to the backend thinking the description was another fastaId. Also adds a description to multiple fasta files in the integration tests to ensure in future such an issue cannot be missed ### 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: https://fastaheader.loculus.org --------- Co-authored-by: Cornelius Roemer <cornelius.roemer@gmail.com> Co-authored-by: Fabian Engelniederhammer <92720311+fengelniederhammer@users.noreply.github.com>
|
@codex review |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
resolves #4999 #4708, #4734, #5511
partially resolves #5392, #5185 (comment)
includes work done in #5398 and #5402
This PR additionally fixes submission, subtype assignment and search for EVs and other multi-path organisms.
BREAKING CHANGES
When users submit to multi-segmented organisms and want to group multiple segments under one metadata entry they are now required to add an additional
fastaIdscolumn with a space -separated list of thefastaIds (fasta header IDs) of the respective sequences. If nofastaIdscolumn is supplied thesubmissionIdwill be used instead and the backend will assume that (as in the single-segmented case) there is a one-to-one mapping of metadatasubmissionIdtofastaId.This new submission structure was voted for in microbioinfo: https://microbial-bioinfo.slack.com/archives/CB0HYT53M/p1760961465729399 and discussed in https://app.nuclino.com/Loculus/Development/2025-10-20-Weekly-6d5fe89f-8ded-4286-b892-d215e0a498f6 (and in other meetings)
Nextclade sort (uses a minimizer index for fast local alignment) or nextclade align (full sequence alignment to reference) will be used to assign segments/subtypes for all multi-segmented and multi-pathogen sequences (this is also done in ingest for grouping segments):
For organisms without a nextclade dataset we still allow the fasta headers to be used to determine the segment/subtype - entries must have the format
<submissionId>_<segmentName>(as in current set up).As preprocessing now assigns segments it will return a map from the segment (or subtype) to the fastaId in the processedData, the map is called:
sequenceNameToFastaId. This allows us to surface the segment assignment on the edit page.Nextclade Preprocessing pipeline config changes
Instead of having a dictionary for the nextclade datasets and servers we make
nucleotideSequencesa dictionary where each item includes all information required to run nextclade. I.e. we change from:to:
Ingest Pipeline Config changes
minimizer_indexis changed tominimizer_urlfor consistency (can be used in ingest and preprocessing and should both be the same)Optional additional Config changes
Limit the number of sequences the backend will accept per submission by using - should be added for multi-segmented organisms:
submissionDataTypes: &defaultSubmissionDataTypes consensusSequences: true maxSequencesPerEntry: 1Testing
You can use pathoplexus/example_data#16 and pathoplexus/dev_example_data#2 for testing.
PR Checklist
add integration testing for full EV submission user journey-> will be done in a later PR🚀 Preview: https://edit-page-anya.loculus.org