Skip to content
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

Add async alignment reader #288

Merged
merged 14 commits into from
Aug 15, 2024
Merged

Add async alignment reader #288

merged 14 commits into from
Aug 15, 2024

Conversation

mbhall88
Copy link
Contributor

@mbhall88 mbhall88 commented Aug 7, 2024

See #286 for more details.

Creating this as a draft so if I am too slow and you want to work on this sooner you can use where I got to (if you want).

Checklist

  • async Reader
    • Builder
    • Documentation examples
    • examples/ entry

@mbhall88 mbhall88 changed the title Add async alignment reader/writer Add async alignment reader Aug 7, 2024
@mbhall88 mbhall88 changed the title Add async alignment reader Add async alignment reader/writer Aug 7, 2024
@mbhall88
Copy link
Contributor Author

mbhall88 commented Aug 7, 2024

I think the async Reader is complete. I haven't added doc examples yet as these will be easier to do once I have implemented the Builder for the Reader.

I'll move on to the Builder after you're happy with everything up to this point @zaeleus. Also, I figured doing all of this in one PR, with reviews at each stage on the checklist, would be best. Though I guess we could do separate PRs for the Reader and Writer. Up to you.

@zaeleus zaeleus added the util label Aug 7, 2024
@zaeleus zaeleus linked an issue Aug 7, 2024 that may be closed by this pull request
Copy link
Owner

@zaeleus zaeleus left a comment

Choose a reason for hiding this comment

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

so if I am too slow and you want to work on this sooner you can use where I got to (if you want).

No rush and no worries.

I haven't added doc examples yet as these will be easier to do once I have implemented the Builder for the Reader.

Examples are not dependent on the builder. The enum variants are public, and one can be selected for the test, e.g., this example written before the builder was available:

/// Reads the VCF header.
///
/// # Examples
///
/// ```
/// # #[tokio::main]
/// # async fn main() -> tokio::io::Result<()> {
/// use noodles_util::variant::r#async::io::Reader;
/// use noodles_vcf as vcf;
///
/// let data = b"##fileformat=VCFv4.5
/// #CHROM\tPOS\tID\tREF\tALT\tQUAL\tFILTER\tINFO
/// ";
///
/// let mut reader = Reader::Vcf(vcf::r#async::io::Reader::new(&data[..]));
/// let _header = reader.read_header().await?;
/// # Ok(())
/// # }
/// ```

Though I guess we could do separate PRs for the Reader and Writer.

I recommend splitting the reader and writer into two PRs. The features are independent, and smaller patches are always easier to reason.

@mbhall88 mbhall88 changed the title Add async alignment reader/writer Add async alignment reader Aug 7, 2024
@mbhall88
Copy link
Contributor Author

I've nearly added the Builder. There is one error when trying to build. The compiler is being ever so patient with me but I seem to be too thick to understand how to resolve the problem

error[E0308]: mismatched types
   --> noodles-util/src/alignment/async/io/reader/builder.rs:168:40
    |
168 |             Format::Bam => Reader::Bam(bam::r#async::io::Reader::new(reader)),
    |                            ----------- ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `AsyncReader<Box<dyn AsyncBufRead + Unpin>>`, found `AsyncReader<AsyncReader<Box<dyn AsyncBufRead + Unpin>>>`
    |                            |
    |                            arguments to this enum variant are incorrect
    |
    = note: expected struct `noodles_bam::AsyncReader<Box<dyn tokio::io::AsyncBufRead + Unpin>>`
               found struct `noodles_bam::AsyncReader<noodles_bgzf::AsyncReader<Box<dyn tokio::io::AsyncBufRead + Unpin>>>`
note: tuple variant defined here
   --> noodles-util/src/alignment/async/io/reader.rs:20:5
    |
20  |     Bam(bam::r#async::io::Reader<R>),
    |     ^^^

I can see that this seems to be coming from having an AsyncReader inside another AsyncReader, and I think it might be coming from the bgzf::AsyncReader? Not really sure how to get around this.

No doubt you'll see a simple solution that I am to naive to think of myself (as always) 😅

@mbhall88 mbhall88 marked this pull request as ready for review August 14, 2024 01:10
Copy link
Owner

@zaeleus zaeleus left a comment

Choose a reason for hiding this comment

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

Great work, thanks!

@zaeleus zaeleus merged commit bba0ee5 into zaeleus:master Aug 15, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unified async alignment reader/writer
2 participants