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.

Sign up for GitHub

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

Jump to bottom

Update of hifiasm version #41

Merged
merged 6 commits into from
May 1, 2024
Merged

Update of hifiasm version #41

merged 6 commits into from
May 1, 2024

Conversation

ksenia-krasheninnikova
Copy link
Contributor

@ksenia-krasheninnikova ksenia-krasheninnikova commented Apr 23, 2024 •
edited by gq1
Loading

The hifiasm patch facilitates piping in HiC data directly from CRAM using samtools fastq without creating intermediate FASTQ files.
This update is aimed at updating hifiasm to the latest version (0.19.8).
For this purpose wave-cli was used and a new wave container was generated.

Since all containers kept by the native wave infrastructure are deleted in a week - should it be moved from

wave.seqera.io/wt/73ac3caec075/wave/build:hifiasm-0.19.8_samtools-1.20--1f6824530f0d0ad5

to the private repo and in this case do we have a ToL repo for that?

Otherwise, there is a way of building a container with wave inside of the project with Dockerfile:
https://www.nextflow.io/docs/latest/wave.html#build-module-containers

UPD:
In the course of working on the PR it was revealed that nf-core wave functionality has not been fully implemented to be built into the workflow. The combined image of hifiasm and samtools was created with wave-cli and will be hosted for now at quay.io/sanger-tol.
The command used to build the image:
wave-1.3.0-linux-x86_64 --conda-package hifiasm=0.19.8 --conda-package samtools=1.20

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 pipeline conventions in the contribution docs
  • Make sure your code lints (nf-core lint).
  • Ensure the test suite passes (nextflow run . -profile test,docker --outdir <OUTDIR>).
  • Usage Documentation in docs/usage.md is updated.
  • Output Documentation in docs/output.md is updated.
  • CHANGELOG.md is updated.
  • README.md is updated (including new tool citations and authors/contributors).

@ksenia-krasheninnikova ksenia-krasheninnikova added the help wanted Extra attention is needed label Apr 23, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 23, 2024 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 59be6c8

+| ✅ 130 tests passed       |+
#| ❔  19 tests were ignored |#
!| ❗   3 tests had warnings |!

❗ Test warnings:

  • files_exist - File not found: conf/igenomes.config
  • pipeline_todos - TODO string in base.config: Check the defaults for all processes
  • pipeline_todos - TODO string in base.config: Customise requirements for specific processes.

❔ Tests ignored:

  • files_exist - File is ignored: assets/nf-core-genomeassembly_logo_light.png
  • files_exist - File is ignored: docs/images/nf-core-genomeassembly_logo_light.png
  • files_exist - File is ignored: docs/images/nf-core-genomeassembly_logo_dark.png
  • files_exist - File is ignored: .github/ISSUE_TEMPLATE/config.yml
  • files_exist - File is ignored: .github/workflows/awstest.yml
  • files_exist - File is ignored: .github/workflows/awsfulltest.yml
  • nextflow_config - Config variable ignored: manifest.name
  • nextflow_config - Config variable ignored: manifest.homePage
  • files_unchanged - File ignored due to lint config: LICENSE or LICENSE.md or LICENCE or LICENCE.md
  • files_unchanged - File ignored due to lint config: .github/CONTRIBUTING.md
  • files_unchanged - File ignored due to lint config: .github/ISSUE_TEMPLATE/bug_report.yml
  • files_unchanged - File ignored due to lint config: .github/workflows/linting.yml
  • files_unchanged - File ignored due to lint config: assets/sendmail_template.txt
  • files_unchanged - File does not exist: assets/nf-core-genomeassembly_logo_light.png
  • files_unchanged - File does not exist: docs/images/nf-core-genomeassembly_logo_light.png
  • files_unchanged - File does not exist: docs/images/nf-core-genomeassembly_logo_dark.png
  • files_unchanged - File ignored due to lint config: lib/NfcoreTemplate.groovy
  • files_unchanged - File ignored due to lint config: .gitignore or .prettierignore or pyproject.toml
  • actions_awstest - 'awstest.yml' workflow not found: /home/runner/work/genomeassembly/genomeassembly/.github/workflows/awstest.yml

✅ Tests passed:

Run details

  • nf-core/tools version 2.8
  • Run at 2024-05-01 10:42:29

@gq1
Copy link
Member

gq1 commented Apr 24, 2024

If you just want to use the latest version of hifiasm, I can see one here:
https://quay.io/repository/biocontainers/hifiasm?tab=tags&tag=latest

If you want to build your own version of hifiasm, you don't need to use wave.

Like I said yesterday in Slack, wave can build docker image on the fly using a Dockefile or just a conda package on the fly, you don't need to store them somewhere.

@ksenia-krasheninnikova
Copy link
Contributor Author

ksenia-krasheninnikova commented Apr 24, 2024 •
edited
Loading

Thanks @gq1!

The reason why we need a combined container is described in the PR description (see above) - we need samtools and hifiasm in the same package.

The on-the-fly functionality hasn't been implemented yet:
nf-core/modules#4940

We can wait until it's implemented or we can host the image built with wave-cli and use it in the 'container' section.

@muffato
Copy link
Member

muffato commented Apr 25, 2024

Hi both,
I think the current recommendation is to use mulled containers: they're exactly there for combining multiple Conda packages, and they've been used many times.

Like you said, @ksenia-krasheninnikova , integrating wave into nf-core is still work-in-progress on their side (that may resume soon, cf nf-core/modules#4080 (comment) )
If we want to manually build containers via wave and store them somewhere without waiting for the nf-core implementation, we'd need a bit of tooling to simplify that. We also need to be clear on when doing that vs when building a custom container in our docker-images repo. For instance, it feels like the cramfilter_bwamem2_minimap2_samtools_perl container could be managed by Wave as well. @gq1 : think about that, the pros and cons, etc, and we can discuss when I'm back

@gq1
Copy link
Member

gq1 commented Apr 25, 2024

Both nf-core PRs only try to pre-build the containers using Conda or Spack packages for modules?
nf-core/modules#4940
nf-core/modules#4080

I didn't try, maybe it just works if we have a local or modified nf-core modules when they only have conda environment or Spack file with wave?

@ksenia-krasheninnikova
Copy link
Contributor Author

ksenia-krasheninnikova commented Apr 26, 2024 •
edited
Loading

According to this discussion on slack it should be possible to add a “conda” directive and -with-wave and it should just work

@gq1
Copy link
Member

gq1 commented Apr 26, 2024

Not sure how you created the wave image currently, manually using conda and wave?

The CI job failed when converting Docker image to SIF? You can try to run with Docker profile first?
FATAL: While making image from oci registry: error fetching image to cache: while building SIF from layers: conveyor failed to get: no descriptor found for reference

@muffato
Copy link
Member

muffato commented Apr 26, 2024

It may be because the Wave container registry only keeps images for 1 week and it's now been deleted.

@ksenia-krasheninnikova
Copy link
Contributor Author

I've just created the wave image this week. The pipeline works fine for me locally with singularity profile. With docker docker/24.0.2-c1 hifiasm module fails for me locally on one of the farm nodes with this error:

Command error:
  WARNING: Your kernel does not support swap limit capabilities or the cgroup is not mounted. Memory limited without swap.
  touch: cannot touch '.command.trace': Read-only file system

@ksenia-krasheninnikova
Copy link
Contributor Author

@gq1 The image was created with this:
wave-1.3.0-linux-x86_64 --conda-package hifiasm=0.19.8 --conda-package samtools=1.20

@gq1
Copy link
Member

gq1 commented Apr 26, 2024

You have a similar yaml file like this?
https://github.com/nf-core/modules/blob/25f2b00a02ae84e44644e4ff94ec56130f984f79/modules/nf-core/bowtie/align/environment.yml
Then only conda directive here:
https://github.com/nf-core/modules/blob/25f2b00a02ae84e44644e4ff94ec56130f984f79/modules/nf-core/bowtie/align/main.nf#L5

gq1
gq1 reviewed Apr 26, 2024
View reviewed changes
modules/nf-core/hifiasm/main.nf
@@ -6,9 +6,7 @@ process HIFIASM {
exit 1, "This version of HIFIASM module does not support Conda. Please use Docker / Singularity / Podman instead."
Copy link
Member

Choose a reason for hiding this comment

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

no conda here

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, thanks for spotting. Something went wrong with adding files in the previous commit.

gq1
gq1 reviewed May 1, 2024
View reviewed changes
modules/nf-core/hifiasm/environment.yml Outdated
@@ -0,0 +1,8 @@
name: hifiasm
Copy link
Member

Choose a reason for hiding this comment

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

This file can be removed from here.

@gq1
Copy link
Member

gq1 commented May 1, 2024

Can you update the description of this PR?

For this purpose wave-cli was used and a new wave container was generated.

Since all containers kept by the native wave infrastructure are deleted in a week - should it be moved from

wave.seqera.io/wt/73ac3caec075/wave/build:hifiasm-0.19.8_samtools-1.20--1f6824530f0d0ad5

to the private repo and in this case do we have a ToL repo for that?

Otherwise, there is a way of building a container with wave inside of the project with Dockerfile:
https://www.nextflow.io/docs/latest/wave.html#build-module-containers

@ksenia-krasheninnikova
Delete modules/nf-core/hifiasm/environment.yml
59be6c8 
This file was deleted as conda is not supported by current module implementation
@ksenia-krasheninnikova ksenia-krasheninnikova removed the help wanted Extra attention is needed label May 1, 2024
@ksenia-krasheninnikova ksenia-krasheninnikova merged commit 271885e into dev May 1, 2024
6 checks passed
@muffato muffato deleted the hifiasm_wave branch May 29, 2024 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gq1 gq1 gq1 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ksenia-krasheninnikova @gq1 @muffato