Skip to content

Merging template updates 2.7.2#373

Merged
skrakau merged 13 commits into
nf-core:devfrom
skrakau:template-update-2.7.2-new
Jan 12, 2023
Merged

Merging template updates 2.7.2#373
skrakau merged 13 commits into
nf-core:devfrom
skrakau:template-update-2.7.2-new

Conversation

@skrakau

@skrakau skrakau commented Dec 20, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Template update 2.7.2

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/mag 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>).
  • 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).

@github-actions

github-actions Bot commented Dec 20, 2022 •
edited
Loading

Copy link
Copy Markdown

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 5d30fed

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

❗ Test warnings:

  • pipeline_todos - TODO string in methods_description_template.yml: #Update the HTML below to your prefered methods description, e.g. add publication citation for this pipeline

❔ Tests ignored:

  • files_unchanged - File ignored due to lint config: lib/NfcoreTemplate.groovy

✅ Tests passed:

Run details

  • nf-core/tools version 2.7.2
  • Run at 2023-01-11 08:45:10

d4straub
d4straub reviewed Dec 20, 2022
View reviewed changes
Comment thread modules/nf-core/multiqc/main.nf
label 'process_single'

conda (params.enable_conda ? 'bioconda::multiqc=1.13' : null)
conda "bioconda::multiqc=1.13"

@d4straub d4straub Dec 20, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hm, does that mean that this change should be made for all modules, i.e. all the local modules?

@jfy133 jfy133 Dec 20, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes it should q

@skrakau skrakau Dec 21, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ups, will do

@skrakau skrakau marked this pull request as draft January 4, 2023 16:47
jfy133
jfy133 reviewed Jan 5, 2023
View reviewed changes

@jfy133 jfy133 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Overall looks straightforward to me, I'm not sure about the AR2 output channel refactor though - something to check

Comment thread workflows/mag.nf

// pair1 and 2 come for PE data from separate output channels, so bring
// this back together again here
ch_adapterremoval_pe_out = Channel.empty()

@jfy133 jfy133 Jan 5, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This deletion may not work, as if you do merging of PE data you won't have the truncated output channel from the AR module (reviewed from my phone, so I'm not 💯 sure)

@skrakau skrakau Jan 5, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks, will check

@skrakau skrakau Jan 10, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Hi @jfy133 ,
isn't the same true for ADAPTERREMOVAL_SE.out.singles_truncated, i.e. that there is no such output if there are no single-end reads? So I think if there are no single-end reads it doesn't cause a problem, the output channel exists, it's just empty. Or am I missing something?

@jfy133 jfy133 Jan 12, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Actually ignore me, I confused myself - you don't want to do merging in this pipeline 😅 so it's irrelevent! You're correct , the output channel is just empty because the module wouldn't get executed.

@skrakau

skrakau commented Jan 5, 2023

Copy link
Copy Markdown
Member Author

Hi @jfy133, the test_adapterremoval CI test fails in a non-reproducible way, see https://github.com/nf-core/mag/actions/runs/3846034900/jobs/6550873713. I couldn't reproduce it yet locally, but it seems to be a problem with the input file.

Did you observe this before or is this new?

@skrakau skrakau marked this pull request as ready for review January 11, 2023 08:51
@skrakau

skrakau commented Jan 11, 2023

Copy link
Copy Markdown
Member Author

I hope I didn't miss anything :D

Maybe worth mentioning:

  • updated nf-core modules with tool version updates:
    • BCFtools 1.14 -> 1.16
    • Freebayes 1.3.5 -> 1.3.6
    • SAMtools 1.15 -> 1.16.1

And I saw that adapterremoval says it's not deterministic when run with multiple threads, even when using the --seed parameter. Maybe it would be worth adding an option to run this in a deterministic manner.

@skrakau skrakau requested a review from d4straub January 11, 2023 09:00
d4straub
d4straub approved these changes Jan 11, 2023
View reviewed changes

@d4straub d4straub left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good to me. Not sure about the channel problem above though.

@skrakau skrakau requested a review from jfy133 January 11, 2023 10:06
@jfy133

jfy133 commented Jan 12, 2023

Copy link
Copy Markdown
Member

And I saw that adapterremoval says it's not deterministic when run with multiple threads, even when using the --seed parameter. Maybe it would be worth adding an option to run this in a deterministic manner.

This is only when you're doing read-merging (collapsing), which is not performed in the pipeline so we should be fine :)

@skrakau skrakau merged commit 4e5c4ca into nf-core:dev Jan 12, 2023
@jfy133 jfy133 mentioned this pull request Feb 27, 2023
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jfy133 jfy133 jfy133 approved these changes

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

4 participants

@skrakau @jfy133 @d4straub @nf-core-bot