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

Add motus prep_long #6590

Merged
merged 30 commits into from
Oct 4, 2024
Merged

Add motus prep_long #6590

merged 30 commits into from
Oct 4, 2024

Conversation

sofstam
Copy link
Contributor

@sofstam sofstam commented Sep 5, 2024

PR checklist

Closes #6589

  • 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
    • For subworkflows:
      • nf-core subworkflows test <SUBWORKFLOW> --profile docker
      • nf-core subworkflows test <SUBWORKFLOW> --profile singularity
      • nf-core subworkflows test <SUBWORKFLOW> --profile conda

@sofstam sofstam requested a review from a team as a code owner September 5, 2024 14:09
@sofstam sofstam requested review from Joon-Klaps and removed request for a team September 5, 2024 14:09
fellen31
fellen31 reviewed Sep 5, 2024
View reviewed changes
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.

Tests should be fixed with echo '' | gzip > I think!

modules/nf-core/motus/preplong/main.nf Outdated Show resolved Hide resolved
modules/nf-core/motus/preplong/main.nf Show resolved Hide resolved
modules/nf-core/motus/preplong/main.nf Outdated Show resolved Hide resolved
modules/nf-core/motus/preplong/meta.yml Outdated Show resolved Hide resolved
modules/nf-core/motus/preplong/meta.yml Outdated Show resolved Hide resolved
modules/nf-core/motus/preplong/tests/main.nf.test Outdated Show resolved Hide resolved
modules/nf-core/motus/preplong/tests/main.nf.test Outdated Show resolved Hide resolved
modules/nf-core/motus/preplong/tests/main.nf.test Outdated Show resolved Hide resolved
sofstam and others added 12 commits September 5, 2024 16:37
@sofstam
Update meta
b881c2e
@sofstam
Merge branch 'master' into motus-prep_long
7adbdd4
@sofstam @fellen31
Update modules/nf-core/motus/preplong/main.nf
cf78dcd 
Co-authored-by: Felix Lenner <[email protected]>
@sofstam @fellen31
Update modules/nf-core/motus/preplong/main.nf
d0bc778 
Co-authored-by: Felix Lenner <[email protected]>
@sofstam
Merge branch 'master' into motus-prep_long
11969c9
@sofstam @fellen31
Update modules/nf-core/motus/preplong/tests/main.nf.test
ddc3cf7 
Co-authored-by: Felix Lenner <[email protected]>
@sofstam @fellen31
Update modules/nf-core/motus/preplong/tests/main.nf.test
6d6586d 
Co-authored-by: Felix Lenner <[email protected]>
@sofstam @fellen31
Update modules/nf-core/motus/preplong/meta.yml
5af72e3 
Co-authored-by: Felix Lenner <[email protected]>
@sofstam @fellen31
Update modules/nf-core/motus/preplong/tests/main.nf.test
c0c725e 
Co-authored-by: Felix Lenner <[email protected]>
@sofstam
Update snap file
9d66e88
@sofstam
Fix linting
b4971df
@sofstam
Merge branch 'master' into motus-prep_long
226543b
SPPearce
SPPearce reviewed Sep 10, 2024
View reviewed changes
modules/nf-core/motus/preplong/meta.yml Outdated Show resolved Hide resolved
modules/nf-core/motus/preplong/tests/main.nf.test
process {
"""
input[0] = Channel.of([
file('https://raw.githubusercontent.com/motu-tool/mOTUs/master/motus/downloadDB.py')
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems strange, why is the input a python script?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In order to run the module, we need to run the download of the motus db first.This is how we have been doing with the motus profile as well.

https://github.com/nf-core/modules/blob/master/modules/nf-core/motus/profile/tests/main.nf.test#L20

Copy link
Contributor

Choose a reason for hiding this comment

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

But the script could just be part of the module?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you please explain what you mean? We need the script to run the test.

Copy link
Member

@jfy133 jfy133 Sep 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think @SPPearce means we could just copy and paste teh code from the mOTUs repository and dump it straight into the module itself.

I think we did this @SPPearce as the script occasioanlly changes to account for changes in where the the database gets downloaded from or something, so it was easier to just download on the fly.

So any copied-code could potentially break.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that is what I meant, to include the script itself with the module for the download module.

Copy link
Contributor

Choose a reason for hiding this comment

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

But if there is a reason behind it then that is fine. It isn't relevant for this module anyway.

SPPearce
SPPearce approved these changes Sep 17, 2024
View reviewed changes
modules/nf-core/motus/preplong/main.nf Outdated Show resolved Hide resolved
modules/nf-core/motus/preplong/main.nf Show resolved Hide resolved
modules/nf-core/motus/preplong/tests/main.nf.test
process {
"""
input[0] = Channel.of([
file('https://raw.githubusercontent.com/motu-tool/mOTUs/master/motus/downloadDB.py')
Copy link
Contributor

Choose a reason for hiding this comment

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

But if there is a reason behind it then that is fine. It isn't relevant for this module anyway.

@sofstam sofstam added this pull request to the merge queue Oct 4, 2024
Merged via the queue into nf-core:master with commit 11680c3 Oct 4, 2024
12 checks passed
@sofstam sofstam deleted the motus-prep_long branch October 4, 2024 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jfy133 jfy133 jfy133 left review comments

@fellen31 fellen31 fellen31 left review comments

@SPPearce SPPearce SPPearce approved these changes

@Joon-Klaps Joon-Klaps Awaiting requested review from Joon-Klaps Joon-Klaps was automatically assigned from nf-core/nf-test

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

Successfully merging this pull request may close these issues.

new module: motus/prep_long
4 participants
@sofstam @jfy133 @SPPearce @fellen31