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 template 2.13.1 #713

Merged
merged 16 commits into from
Mar 20, 2024
Merged

Update template 2.13.1 #713

merged 16 commits into from
Mar 20, 2024

Conversation

d4straub
Copy link
Collaborator

@d4straub d4straub commented Mar 18, 2024 •
edited
Loading

Closes #712

I had to change test_iontorrent & test_pacbio_its to not use dada2 with unite but sintax (were cancelled because of too high memory requirements). That meant unfortunately disabling addsh which is now not tested anywhere, I think.

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/ampliseq 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 --outdir <OUTDIR>).
  • Check for unexpected warnings in debug mode (nextflow run . -profile debug,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).

@d4straub d4straub mentioned this pull request Mar 18, 2024
Closed
@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 19, 2024 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 7f3e4b6

+| ✅ 191 tests passed       |+
#| ❔   6 tests were ignored |#
!| ❗   1 tests had warnings |!

❗ Test warnings:

  • readme - README did not have a Nextflow minimum version badge.

❔ Tests ignored:

✅ Tests passed:

Run details

  • nf-core/tools version 2.13.1
  • Run at 2024-03-20 09:18:19

@d4straub
Copy link
Collaborator Author

d4straub commented Mar 19, 2024 •
edited
Loading

Ah, almost everything is working again! What a journey!

test_iontorrent & test_pacbio_its both fail with

Process `NFCORE_AMPLISEQ:AMPLISEQ:DADA2_TAXONOMY_WF:DADA2_TAXONOMY (ASV_ITS_seqs.full.fasta,assignTaxonomy.fna)` terminated with an error exit status (137)

indicating that memory requirements are too high. This is curious because it passed in the past.
Both tests use dada_ref_taxonomy = "unite-fungi=8.3". unite-fungi=8.3 leads to a file with 18.8MB, 8.2 to 14.5MB, so testing the smaller one now.

edit: nope, doesnt work! Still

   Command exit status:
    137

edit2: running the test locally returns a memory usage of 6.045Gb, which is slightly over the limit of 6GB...

edit3: using sintax requires much less memory, so switching to sintax...

edit4: this requires unfortunately to disable addsh testing

@d4straub d4straub marked this pull request as ready for review March 19, 2024 15:06
erikrikarddaniel
erikrikarddaniel approved these changes Mar 20, 2024
View reviewed changes
Copy link
Member

@erikrikarddaniel erikrikarddaniel left a comment

Choose a reason for hiding this comment

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

Looks good!

mashehu
mashehu approved these changes Mar 20, 2024
View reviewed changes
assets/multiqc_config.yml
disable_version_detection: true

# MultiQC version 1.20 & 1.21 need that flag to produce plots!
# TODO: Remove with version greater than 1.21!
Copy link
Contributor

Choose a reason for hiding this comment

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

I am curious, is this a general issue with multiqc? if so we should bring it up in the #pipline-maintainers channel on slack and add it to the template as well

Copy link
Collaborator Author

@d4straub d4straub Mar 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is a bug in multiqc, yes, I reported it and will be fixed in next release. I highly doubt that many use that folder as I do and care that its empty ;)
edit: "this folder" means multiqc_plots
edit2: link to slack convo: https://nfcore.slack.com/archives/C05663US2B0/p1710846896142419

main.nf Outdated Show resolved Hide resolved
nextflow_schema.json Outdated Show resolved Hide resolved
nextflow_schema.json Show resolved Hide resolved
nextflow_schema.json Show resolved Hide resolved
@d4straub @mashehu
Apply suggestions from code review
7f3e4b6 
Co-authored-by: Matthias Hörtenhuber <mashehu@users.noreply.github.com>
@d4straub
Copy link
Collaborator Author

d4straub commented Mar 20, 2024 •
edited
Loading

Thanks you two for your reviews, will merge it after tests passed.

@d4straub d4straub merged commit d05e42e into nf-core:dev Mar 20, 2024
17 checks passed
@d4straub d4straub deleted the update-template-2.13.1 branch March 20, 2024 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erikrikarddaniel erikrikarddaniel erikrikarddaniel approved these changes

@mashehu mashehu mashehu 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
@d4straub @erikrikarddaniel @mashehu