Skip to content

Skip ';' comment lines before the first FASTA record - #122

Open
adamjohnwright wants to merge 1 commit into
onecodex:masterfrom
adamjohnwright:fasta-leading-comments
Open

adamjohnwright wants to merge 1 commit into
onecodex:masterfrom
adamjohnwright:fasta-leading-comments

Conversation

@adamjohnwright

Copy link
Copy Markdown

parse_fastx_reader rejects a FASTA file whose first lines start with ; (the original Pearson FASTA comment convention):

Expected '@' or '>' at the start of the file but found ';'.

This currently breaks tools built on needletail. For example, salmon 2.8.0 salmon index -t ref.fa fails with exactly this error, via piscem-rs.

This PR:

  • get_fastx_reader handles a leading ; by skipping ; lines, then returning a FastaReader for the rest.
  • Comment lines followed by a FASTQ record are still UnknownFormat, since FASTQ has no comment lines.
  • A file with only comment lines is EmptyFile.
  • Files starting with > or @ take exactly the same path as before.

One side effect to be aware of: for files with a comment block, the reader's own byte and line positions count from the first > rather than from the start of the file.

Tests: four new tests in parser::test:

  • skipped comments with two records;
  • a gzipped file;
  • a comment before a FASTQ record, which is an error;
  • a file with only comments, which is an empty-file error.

cargo test --all-features passes (Rust 1.99), and cargo fmt and cargo clippy --all-features are clean.

Context: we maintain the FAIR Header Reference genome (FHR) metadata standard (https://doi.org/10.1093/bib/bbae122). It stores provenance as ;~ lines before the first record. The same narrow rule is proposed for htslib, htsjdk, seqkit and others.

This change was prepared with an AI coding assistant (Claude Code) and reviewed by me.

parse_fastx_reader rejected a FASTA file whose first lines start with
';' (the original FASTA comment convention): "Expected '@' or '>' at
the start of the file but found ';'". This breaks e.g. salmon index on
such references. Skip those lines before the first '>' record; they
are still an error before a FASTQ record.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@audy

audy commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Hi @adamjohnwright. Supporting comments at the beginning of the file (and possibly followed by blank lines) seems like a low-risk addition to needletail but I'm not sure if that is the complete spec for Pearson-style FASTA files as there seem to be examples of other parsers and files that have comments in-between records, empty lines between comments/records, and whatever this is on Wikipedia (the LCBO example). I'd like to avoid creating yet another subtle variation of FASTA parsing by only implementing part of the Pearson style comment "spec". I also can't find any examples in that repo with comments (the LCBO example in that repo seems to have had its comments stripped https://github.com/wrpearson/fasta2/blob/master/lcbo.aa) 😭 .

If the goal is to maintain backwards compatibility with Salmon pre-Rust rewrite, then I think needletail would need to skip everything before the first record: https://github.com/COMBINE-lab/salmon/blob/a2f6912b3f9f9af91e3a4b0d74adcb3bdc4c9a32/include/kseq++.hpp#L572 and inter-record comments would get included as part of the sequence. I believe htslib behaves the same way. This might be okay with the current implementation (it preserves the most backwards-compatibility, and skipping lines at the beginning of the file won't significantly impact performance).

So I'm not really quite sure what the best option is here. FASTA format is a can of worms and the probably easiest way to maintain a performant parser is to limit support to a strict subset. I'm open to ideas though.

@adamjohnwright

Copy link
Copy Markdown
Author

Thanks @audy, that's a really helpful analysis. Skipping everything before the first record would work well for us, and it avoids defining a partial Pearson variant: it's the existing kseq behaviour (old salmon, and in our testing BWA, minimap2, seqtk, kallisto, samtools dict and pysam's FastxFile all do this). Our headers are only ever placed before the first record, so we don't need comments between records. If that approach works for you, I'm happy to update this PR to skip all lines before the first >/@ instead of only ; lines, with tests.

@jmarshall

Copy link
Copy Markdown

To be clear, the existing kseq.h behaviour is to start parsing FASTA/Q records from the first > or @ character — even if that character is in the middle of what a human reader would interpret as a preceding comment line.

@adamjohnwright

Copy link
Copy Markdown
Author

Thanks @jmarshall, that's an important detail. To be precise about what we'd propose for needletail: skip whole lines until the first line that starts with > (or @), rather than kseq's character scan, so a > or @ inside a leading line can't start a record. @audy, if you prefer that approach, I'll update the PR to do that, with tests covering exactly that case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants