Explain why CRAM reference base lookup failed - #1790
Merged
Conversation
When a reference source returned no bases for a sequence that is present in
the sequence dictionary, CRAMReferenceRegion threw
Failure getting reference bases for sequence chr1
which says what went wrong but nothing about why, or what to do. In practice
the overwhelmingly common cause is a reference fasta that has not been indexed,
so users hit this via GATK/Picard with a perfectly good reference and no
indication that a .fai is what's missing.
Say so: name the sequence and point at the companion .fai/.gzi/.dict files a
CRAM needs. This changes only the message text, not which inputs are accepted.
Note this does not address #998, which is a different misleading message raised
from ReferenceSource on a separate code path.
📝 WalkthroughWalkthrough
ChangesReference retrieval diagnostics
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/test/java/htsjdk/samtools/cram/build/CRAMReferenceRegionErrorMessageTest.java (1)
65-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the
.gziguidance in the test.The implementation now mentions
.gzifor bgzipped FASTA, but the test only protects the sequence,.fai, and.dictwording. Add an assertion for.gziso all advertised companion files are covered.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/htsjdk/samtools/cram/build/CRAMReferenceRegionErrorMessageTest.java` around lines 65 - 70, Update missing_bases_error_mentions_the_sequence_dictionary to also assert that the fetched error message contains “.gzi”, preserving the existing .dict assertion and message context.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/htsjdk/samtools/cram/build/CRAMReferenceRegion.java`:
- Around line 178-183: Update the IllegalArgumentException message in
CRAMReferenceRegion to include the actionable samtools faidx command for
generating the reference FASTA’s .fai index, while preserving the existing
sequence-name and .gzi/.dict guidance.
---
Nitpick comments:
In
`@src/test/java/htsjdk/samtools/cram/build/CRAMReferenceRegionErrorMessageTest.java`:
- Around line 65-70: Update missing_bases_error_mentions_the_sequence_dictionary
to also assert that the fetched error message contains “.gzi”, preserving the
existing .dict assertion and message context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 38d92f9f-3f97-4e30-b5fc-639d34205ec8
📒 Files selected for processing (2)
src/main/java/htsjdk/samtools/cram/build/CRAMReferenceRegion.javasrc/test/java/htsjdk/samtools/cram/build/CRAMReferenceRegionErrorMessageTest.java
Comment on lines
+178
to
+183
| throw new IllegalArgumentException(String.format( | ||
| "Failure getting reference bases for sequence '%s'. The reference source did not return any " | ||
| + "bases for this sequence. Verify that the reference fasta contains a sequence named " | ||
| + "'%s', and that it has been indexed -- a CRAM requires a fasta with a companion .fai " | ||
| + "index (plus a .gzi index if the fasta is bgzipped) and a .dict sequence dictionary.", | ||
| sequenceRecord.getSequenceName(), sequenceRecord.getSequenceName())); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Include the samtools faidx remediation.
The message identifies the likely missing indexes but does not provide the explicitly requested actionable command. Add guidance such as samtools faidx <reference.fasta> for creating the .fai index.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/htsjdk/samtools/cram/build/CRAMReferenceRegion.java` around
lines 178 - 183, Update the IllegalArgumentException message in
CRAMReferenceRegion to include the actionable samtools faidx command for
generating the reference FASTA’s .fai index, while preserving the existing
sequence-name and .gzi/.dict guidance.
This was referenced Jul 26, 2026
tfenne
added a commit
that referenced
this pull request
Jul 26, 2026
The whole-contig lookup reported
A reference must be supplied (reference sequence SAMSequenceRecord(name=1,
length=200,dict_index=0,assembly=null,alternate_names=[]) not found).
which is misleading in the common case: a reference *was* supplied, it just
had not been indexed. It also dumped the entire SAMSequenceRecord at the user.
This is the sibling of the by-region lookup fixed in #1790 for #1732. Both fail
for the same reason, so factor the diagnostic into one helper and use it from
both, rather than letting the two messages drift apart. The message names the
sequence and points at the companion .fai/.gzi/.dict files a CRAM needs.
Message text only; no change to which inputs are accepted.
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
1 participant
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.
Closes #1732.
When a reference source returns no bases for a sequence that is present in the sequence dictionary,
CRAMReferenceRegionthrewFailure getting reference bases for sequence chr1— which says what went wrong but nothing about why or what to do. In practice the overwhelmingly common cause is a reference fasta that has not been indexed, so users hit this via GATK/Picard with a perfectly good reference and no hint that a missing.faiis the problem.The message now names the sequence and points at the companion
.fai/.gzi/.dictfiles a CRAM needs. Message text only — no change to which inputs are accepted.Note this does not address #998, which is a different misleading message raised from
ReferenceSourceon a separate code path; that one stays open.Summary by CodeRabbit