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

Merging template updates 2.9 and workflow update #86

Merged
merged 22 commits into from
Sep 20, 2023
Merged

Merging template updates 2.9 and workflow update #86

merged 22 commits into from
Sep 20, 2023

Conversation

Daniel-VM
Copy link
Contributor

@Daniel-VM Daniel-VM commented Sep 11, 2023 •
edited
Loading

This PR merges the template update v2.9 (#84). The new implementations ensure that the nf-core/bacass workflow is compatible with the latest nf-core version.

Taking advantage of the template update, several local modules and processes listed in #61 have been refactored into nf-core modules, along with a version update.

Other implementations have been included in accordance with the task list in #85.

Note: Appropriate test data for running Canu, PycoQC, or Nanopolish is still unavailable (as mentioned in #56 ).

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
    • If necessary, also make a PR on the nf-core/bacass branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core lint).
  • Ensure the test suite passes (nextflow run . -profile test,docker).
  • 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).

Specific check list

  • Merge template update 2.9 and fix conflicts.
  • Update local modules to nf-core modules and update versions (See CHANGELOG.md): Canu, minimap2, Miniasm, Racon.
  • Update existing nf-core modules: FastQC, Samtools, Kraken2, Quast, Prokka, MultiQC.
  • Refactor local modules that couldn't be updated into nf-core/modules (See CHANGELOG.md): Dfast, Medaka, Nanoplot, Nanopolish, Pycoqc, Unicycler.
  • Replace Skewer with FastP wrapping it with FastQC by using subworkflows/nf-core/fastq_trim_fastp_fastqc.
  • Replace the local module get_software_versions.nf with nf-core/custom/dumpsoftwareversions.nf.
  • Refactor channels and ensure that they are are functioning correctly.
  • Refactor config files into nf-core v2.9 format.
  • Update nextflow_schema.json by adding some extra params.
  • Make all existing test datasets compatible with this template version update.

nf-core-bot and others added 16 commits December 14, 2021 16:42
@github-actions
Copy link

github-actions bot commented Sep 12, 2023 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 5c6fbee

+| ✅ 156 tests passed       |+
#| ❔   1 tests were ignored |#
!| ❗   4 tests had warnings |!

❗ Test warnings:

  • pipeline_todos - TODO string in README.md: Describe the minimum required steps to execute the pipeline, e.g. how to prepare samplesheets.
  • pipeline_todos - TODO string in README.md: update the following command to include all required parameters for a minimal example
  • pipeline_todos - TODO string in methods_description_template.yml: #Update the HTML below to your preferred methods description, e.g. add publication citation for this pipeline
  • pipeline_todos - TODO string in awsfulltest.yml: You can customise AWS full pipeline tests as required

❔ Tests ignored:

✅ Tests passed:

Run details

  • nf-core/tools version 2.9
  • Run at 2023-09-19 20:49:29

@d4straub
Copy link
Collaborator

d4straub commented Sep 12, 2023 •
edited
Loading

Wow, thats a large PR. I didnt check in detail yet, but only initiated starting the automatic tests. All test failed with Missing required parameter: --outdir.
I assume adding in .github/workflows/ci.yml line 42 & 68 --outdir results should fix the issue.

@Daniel-VM
Copy link
Contributor Author

I'm afraid it is... My bad 😵‍💫. but there were a lot of files to update since the previous template update. Hope this commit fix the tests.

@d4straub
Copy link
Collaborator

d4straub commented Sep 12, 2023 •
edited
Loading

No worries, thats a process ;)
Current problems are failing container downloads, such as
docker: Error response from daemon: manifest for biocontainers/fastp:0.23.4--h5f740d0_0 not found: manifest unknown: manifest unknown.
It looks to me that a crucial snippet got lost in nextflow.config:
Lines 173 - 179 in nextflow.config in https://github.com/nf-core/bacass/pull/84/files#diff-ce7465a8e67b3b9c95696db1146ca7e6328f075e08a3d64062b87aa8608fc4ea
That are

// Set default registry for Apptainer, Docker, Podman and Singularity independent of -profile
// Will not be used unless Apptainer / Docker / Podman / Singularity are enabled
// Set to your registry if you have a mirror of containers
apptainer.registry   = 'quay.io'
docker.registry      = 'quay.io'
podman.registry      = 'quay.io'
singularity.registry = 'quay.io'

@Daniel-VM
Copy link
Contributor Author

Oops, I am going to fix and test it locally again using docker...

@Daniel-VM
Copy link
Contributor Author

Hello!, I have locally tested -profile docker and it works now (I haven't pushed yet). However, quay.io/biocontainers/biocontainers:v1.2.0_cv1 (process: KRAKEN2_DB_PREPARATION) requires to sign-in in quay.io.
Docker error:

docker: Error response from daemon: unauthorized: access to the requested resource is not authorized.

After sing-up into quay.io my account was blocked (I dunno why). Therefore, I can't login to quay.io and the error message above arise when the wf tries to access to quay.io/biocontainers/biocontainers:v1.2.0_cv1. Does the CI test in GitHub take into account that it will require an user account in quay.io/ to be able to download the image?.

@d4straub
Copy link
Collaborator

d4straub commented Sep 13, 2023 •
edited
Loading

There shouldnt be any sign in needed, never.
'quay.io/biocontainers/biocontainers:v1.2.0_cv1' seems indeed wrong, replace 'biocontainers/biocontainers:v1.2.0_cv1' with 'docker.io/biocontainers/biocontainers:v1.2.0_cv1' and you should be good.

@Daniel-VM
Copy link
Contributor Author

In fact, that was the solution! 🙌🏾

@d4straub
Copy link
Collaborator

Sorry for the delay, I will try to review on Monday.

d4straub
d4straub reviewed Sep 18, 2023
View reviewed changes
Copy link
Collaborator

@d4straub d4straub left a comment

Choose a reason for hiding this comment

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

Nice work!

I think there was a little unused code added from the template such as igenomes.config and sample_check.py etc which could be removed.
Also the samplesheet problem with supplying a csv that is tab separated (-> tsv) could be solved.
Other than that it looks fine to me.

I havent run the pipeline but looked only at the code. Aiming to run the pipeline today, but I dont expect major new insight.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CITATIONS.md Outdated Show resolved Hide resolved
modules/local/samplesheet_check.nf Outdated Show resolved Hide resolved
nextflow.config Show resolved Hide resolved
nextflow.config Outdated Show resolved Hide resolved
nextflow.config Show resolved Hide resolved
nextflow_schema.json Show resolved Hide resolved
@d4straub
Copy link
Collaborator

I did run the pipeline with nextflow run Daniel-VM/bacass -r dev -profile test,singularity --outdir results, the output was fine, didnt see any problem, but
WARN: The operator first is useless when applied to a value channel which returns a single value by definition. Would be good to avoid the warning.

@Daniel-VM
Copy link
Contributor Author

Test are running but I totally forgot to remove the WARN: The operator first is useless when applied to a value channel which returns a single value by definition 😱. Lets see the CI test result and I will add another commit to this PR avoiding the warn message before this branch gets merged.

@Daniel-VM
Copy link
Contributor Author

Warning messages removed ;). Thank you so much for the review !

d4straub
d4straub approved these changes Sep 20, 2023
View reviewed changes
Copy link
Collaborator

@d4straub d4straub left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Great!

nf-validation on sample sheet and/or tsv/csv documentation and parsing will need adjustments, but the template update is fine as is I think.

Now we only need to have the branch protection rules adjusted to reflect the new tests. I already attempted to get someone to do it but wasnt successful yet.

@Daniel-VM Daniel-VM merged commit ba415ef into nf-core:dev Sep 20, 2023
10 checks passed
@Daniel-VM
Copy link
Contributor Author

Awesome! Thanks again 👍🏾 .

This was referenced Sep 20, 2023
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@d4straub d4straub d4straub 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.

None yet
3 participants
@Daniel-VM @d4straub @nf-core-bot