fix: LIBRARY_LAYOUT now PAIRED again when 2+ fastq files supplied - #25
Open
corneliusroemer wants to merge 2 commits into
Open
fix: LIBRARY_LAYOUT now PAIRED again when 2+ fastq files supplied#25corneliusroemer wants to merge 2 commits into
corneliusroemer wants to merge 2 commits into
Conversation
added 2 commits
July 26, 2026 16:03
…onResponse ReadsValidator created an AtomicBoolean paired, never passed it to runValidatorWrapper, and then read it back into the response - so isPaired() was always false and ReadsXmlWriter emitted LIBRARY_LAYOUT SINGLE for every reads submission, including two-file paired FASTQ. INSERT_SIZE was silently dropped with it, since NOMINAL_LENGTH is only written inside the PAIRED element. ValidatorWrapper.isPaired() already computed the correct value and had no callers. Threading the AtomicBoolean through mirrors the pre-2024 shape and restores webin-cli 7.0.1 semantics exactly. Regression introduced in eaa11b6 (2024-02-01) by the v1->v2 validator migration; shipped from webin-cli 7.1.0 onwards. Adds ReadsValidatorTest covering both directions. The single-file case asserts isPaired() is false, pinning ENA-3407's intent so it is not 'fixed' by accident.
The single-file assertion previously said it pinned 'ENA-3407's intent'. ENA-3407 is an internal EBI JIRA id that nobody outside EBI can read, so that claimed knowledge we do not have. Reference 7517a26 / PR enasequence#11 and what their diff actually does instead - publicly checkable, and honest about being an inference from the commit rather than from the ticket.
There was a problem hiding this comment.
Pull request overview
This PR fixes propagation of paired-read detection from the underlying validator so that ReadsValidationResponse.isPaired() reflects paired FASTQ submissions (enabling webin-cli to emit LIBRARY_LAYOUT=PAIRED again when appropriate).
Changes:
- Propagate
ValidatorWrapper.isPaired()intoReadsValidationResponse.setPaired(...). - Add unit tests asserting pairedness for two-FASTQ submissions and non-pairedness for single-FASTQ submissions.
- Introduce a small test helper for validating FASTQ resources via
ReadsValidator.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/main/java/uk/ac/ebi/ena/readtools/validator/ReadsValidator.java | Wires paired-read detection from ValidatorWrapper into the public validation response. |
| src/test/java/uk/ac/ebi/ena/readtools/validator/ReadsValidatorTest.java | Adds regression tests covering paired vs single FASTQ pairedness reporting. |
Comments suppressed due to low confidence (1)
src/main/java/uk/ac/ebi/ena/readtools/validator/ReadsValidator.java:116
pairedis only set whenvalidatorWrapper.run()completes successfully. Ifrun()throws after detecting pairing (e.g., paired FASTQs failing the pairing-threshold check), the response will incorrectly reportpaired=falsebecause the setter is skipped in the catch path. Setting it in afinallyblock preserves the detected pairing state even when validation fails.
ValidatorWrapper validatorWrapper =
new ValidatorWrapper(files, fileFormat, isQuick ? QUICK_READ_LIMIT : EXTENDED_READ_LIMIT);
try {
validatorWrapper.run();
fileQualityStats = validatorWrapper.getFileQualityStats();
paired.set(validatorWrapper.isPaired());
} catch (ReadsValidationException e) {
result.add(ValidationMessage.error(e.getMessage()));
e.printStackTrace();
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+93
to
+95
| Path workDir = Files.createTempDirectory("rvt"); | ||
| Path reportFile = Files.createTempFile(workDir, "", ".report"); | ||
|
|
corneliusroemer
added a commit
to corneliusroemer/webin-cli
that referenced
this pull request
Jul 26, 2026
…NGLE This PR makes the tests fail the way they should. Since readtools version 2.3.0 all reads submissions are classified as LIBRARY_LAYOUT=SINGLE even if they are paired (paired fastq, bam, cram) The test that should have caught this lacked an assertion hence silently passed. For tests to pass, the fix at enasequence/readtools#25 or similar has to be merged, released and webin-cli has to point at the new readtools release. Wrongly classified experiments could potentially be fixed. The bug is live since webin-cli version 7.1.0 (2024-03-14) Related to enasequence/readtools#26 Related to enasequence/readtools#27
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Since commit eaa11b6 (2024-02-01 first released in 2.3.0)
LIBRARY_LAYOUTis always set toSINGLEfor any FASTQs submission, even when there are 2 or more files supplied and the reads are paired.Claude noticed this when reading back the XML that got posted by webin-cli from a test-submission with paired FASTQs and I got it to verify the bug.
Tests verify that this should now be resolved - but I don't know the codebase so there might be gotchas. I'm pretty confident the bug is real though.
The bug landed in webin-cli in version 7.1.0 (2024-03-14) - so all paired reads submitted with webin-cli of that version or higher will likely wrongly have SINGLE not PAIRED. Not sure if this can be fixed retrospectively.
The bug (but not this fix here) is possibly broader in that it extends to BAM/CRAM as well: those appear to also always produce SINGLE.