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

Unified async alignment reader/writer #286

Closed
mbhall88 opened this issue Jul 23, 2024 · 3 comments · Fixed by #288 or #292
Closed

Unified async alignment reader/writer #286

mbhall88 opened this issue Jul 23, 2024 · 3 comments · Fixed by #288 or #292
Assignees
Labels

Comments

@mbhall88
Copy link
Contributor

I use the noodles_util::alignment readers and writers a lot. How easy would it be to implement an aync version of this transparent reader/writer?

If this is something you think would be reasonably straightforward I would be willing to draft a PR with some guidance on where to start.

@zaeleus zaeleus added the util label Jul 23, 2024
@zaeleus
Copy link
Owner

zaeleus commented Jul 24, 2024

If this is something you think would be reasonably straightforward I would be willing to draft a PR with some guidance on where to start.

Yes, give it a go! It's a loaded issue, consisting of four parts:

I recommend focusing on each individually, submitting a patch for review per part. It is a fair amount of work/code, but don't feel obligated to complete it all.

For the first part, I normalized the alignment format async reader methods, so you shouldn't have any problems outside of util. I also added an async variant format reader and builder to use as a reference.

Here are some instructions on how to proceed with the reader:

  1. Add the alignment format dependencies (SAM, BAM, and CRAM) to the async feature in noodles-util/Cargo.toml. These use the optional dependency feature syntax.

    async = [
    "dep:futures",
    "dep:tokio",
    "noodles-bcf?/async",
    "noodles-bgzf?/async",
    "noodles-vcf?/async",

  2. Add the modules.

    • noodles-util/src/alignment/async.rs (example)
    • noodles-util/src/alignment/async/io.rs (example)
    • noodles-util/src/alignment/async/io/reader.rs (example)
  3. Implement Reader<R>. This will not use the I/O trait approach like the blocking version. Since well know all the format variants, use an enum instead. Feel free to start with the following scaffold.

//! Async alignment reader.

/// An async alignment reader.
pub enum Reader<R> {
    /// SAM.
    Sam(sam::r#async::io::Reader<R>),
    /// BAM.
    Bam(bam::r#async::io::Reader<R>),
    /// CRAM.
    Cram(cram::r#async::io::Reader<R>),
}

impl<R> Reader<R>
where
    R: AsyncBufRead + Unpin,
{
    /// Reads the SAM header.
    pub async fn read_header(&mut self) -> io::Result<sam::Header> {
        todo!()
    }

    /// Returns an iterator over records starting from the current stream position.
    pub fn records<'r, 'h: 'r>(
        &'r mut self,
        header: &'h sam::Header,
    ) -> impl Stream<Item = io::Result<Box<dyn sam::alignment::Record>>> + 'r {
        todo!()
    }
}

Let me know if you have any further questions or issues.

@mbhall88 mbhall88 mentioned this issue Aug 7, 2024
4 tasks
@zaeleus zaeleus linked a pull request Aug 7, 2024 that will close this issue
4 tasks
@zaeleus zaeleus reopened this Aug 15, 2024
@zaeleus zaeleus linked a pull request Aug 22, 2024 that will close this issue
4 tasks
@mbhall88
Copy link
Contributor Author

mbhall88 commented Aug 23, 2024

I wonder if it also makes sense to add an asynchronous version of an indexed reader? Given the implementations on SAM/BAM/CRAM are !Send I guess we probably need that auto trait before we could do that? And I hope I understand this correctly, we might also want Sync as currently it is !Sync, though my understanding is it doesn't have to be Sync to be used in async code correct?

@zaeleus
Copy link
Owner

zaeleus commented Aug 26, 2024

I wonder if it also makes sense to add an asynchronous version of an indexed reader?

Tracked in #296.

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 a pull request may close this issue.

2 participants