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

Update Hifiasm to 0.19.9 #6796

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Conversation

Schmytzi
Copy link
Contributor

@Schmytzi Schmytzi commented Oct 17, 2024

Bumped HifiAsm to 0.19.9. Also added tests.

PR checklist

  • This comment contains a description of changes (with reason).
  • If you've fixed a bug or added code that should be tested, add tests!
  • If you've added a new tool - have you followed the module conventions in the contribution docs
  • If necessary, include test data in your PR.
  • Remove all TODO statements.
  • Emit the versions.yml file.
  • Follow the naming conventions.
  • Follow the parameters requirements.
  • Follow the input/output options guidelines.
  • Add a resource label
  • Use BioConda and BioContainers if possible to fulfil software requirements.
  • Ensure that the test works with either Docker / Singularity. Conda CI tests can be quite flaky:
    • For modules:
      • nf-core modules test <MODULE> --profile docker
      • nf-core modules test <MODULE> --profile singularity
      • nf-core modules test <MODULE> --profile conda

@Schmytzi Schmytzi marked this pull request as draft October 17, 2024 13:53
@Schmytzi Schmytzi changed the title Update Hifiasm to 0.9.19 Update Hifiasm to 0.19.9 Oct 17, 2024
@Schmytzi Schmytzi marked this pull request as ready for review October 17, 2024 14:58
tag "modules_nfcore"
tag "hifiasm"

test("Test HifiAsm - fq [] [] [] []") {
Copy link
Contributor

@sateeshperi sateeshperi Oct 17, 2024

Choose a reason for hiding this comment

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

Suggested change
test("Test HifiAsm - fq [] [] [] []") {
test("homo_sapiens pacbio hifi [fastq]") {

Comment on lines 30 to 40
process.out.raw_unitigs,
process.out.processed_contigs,
process.out.processed_unitigs,
process.out.primary_contigs,
process.out.alternate_contigs,
process.out.paternal_contigs,
process.out.maternal_contigs,
process.out.versions
Copy link
Contributor

Choose a reason for hiding this comment

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

are you leaving out any channel or could all out channels be captured with process.out ?

Copy link
Contributor Author

@Schmytzi Schmytzi Oct 17, 2024

Choose a reason for hiding this comment

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

I'm leaving out the log and the binary files as those are not stable. The latter aren't documented as output files either but I figured some people might depend on them and I did not want to change the process definition.

Comment on lines 25 to 28
],
[

],
Copy link
Contributor

Choose a reason for hiding this comment

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

optional outputs ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, these are optional. Should I rather remove them from the test if we expect them to not be present?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes

@sateeshperi
Copy link
Contributor

u need to rm pytests and fix the nf-tests. reach on #nf-test channel if you are stuck

@GallVp
Copy link
Member

GallVp commented Oct 17, 2024

Thank you for the contribution.
The pytests are still running. Can you please delete the pytest files for hifiasm?

@Schmytzi Schmytzi requested a review from a team as a code owner October 18, 2024 07:31
Copy link
Contributor

@fellen31 fellen31 left a comment

Choose a reason for hiding this comment

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

This could be adressed in another PR, but since you are writing the tests:

The paternal and maternal kmer inputs (as well as the HiC inputs) are specific to the reads, and I think they should be input together, i.e.:

tuple val(meta), path(reads), path(paternal_kmer_dump), path(maternal_kmer_dump), path(hic_read1), path(hic_read2)

Otherwise, I think there's no way to ensure that the reads and kmers that belong together are input to the process together, so Sample1 could get Sample2's parental kmers.

@@ -2,4 +2,4 @@ channels:
- conda-forge
- bioconda
dependencies:
- bioconda::hifiasm=0.19.8
- bioconda::hifiasm=0.19.9
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we update this straight to 0.20.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think there's value in having version 0.19.9 in at least one version of nf-core/modules. In part because there appear to be some major changes in 0.20. We could combine the update to that version with your suggested change of the input channel.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll post it as a separate issue: #6799

Copy link
Contributor

Choose a reason for hiding this comment

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

You will need to pass -f0 with a nextflow.config to hifiasm to reduce the RAM requirements, in order for the tests to run on the GitHub runners.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the hint! I was going crazy over that 😄

@Schmytzi Schmytzi marked this pull request as draft October 18, 2024 08:30
@Schmytzi Schmytzi marked this pull request as ready for review October 18, 2024 09:36
@mahesh-panchal
Copy link
Member

Otherwise, I think there's no way to ensure that the reads and kmers that belong together are input to the process together, so Sample1 could get Sample2's parental kmers.

We usually use multiMap for this.

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.

5 participants