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

Added subworkflow fasta_gxf_busco_plot #7051

Merged
merged 9 commits into from
Nov 26, 2024
Merged

Added subworkflow fasta_gxf_busco_plot #7051

merged 9 commits into from
Nov 26, 2024

Conversation

GallVp
Copy link
Member

@GallVp GallVp commented Nov 21, 2024 •
edited
Loading

PR checklist

Closes #5584

  • Added subworkflow fasta_gxf_busco_plot. This sub-workflow is to be shared by assemblyqc, genomeqc and genepal. Please see linked issue: Using FASTA_GXF_BUSCO_PLOT sub workflow genomeqc#77 (comment)
  • 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

@GallVp
Copy link
Member Author

GallVp commented Nov 21, 2024

I do not think we need to run,

nf-core subworkflows lint fasta_gxf_busco_plot/meta.yml

Or am I missing something after taking a break from nf-core/modules?

@chriswyatt1
Copy link
Contributor

I am not sure either. But looks like you cannot run nf-core subworkflows lint on a path, so it should fail

@SPPearce
Copy link
Contributor

I do not think we need to run,

nf-core subworkflows lint fasta_gxf_busco_plot/meta.yml

Or am I missing something after taking a break from nf-core/modules?

Yes, there was an issue in the GitHub workflow action, I don't know if it has been fixed yet

@GallVp GallVp force-pushed the fasta_gxf_busco_plot branch from cfb6e32 to 5265979 Compare November 24, 2024 21:37
@GallVp
Copy link
Member Author

GallVp commented Nov 24, 2024

Yes, there was an issue in the GitHub workflow action, I don't know if it has been fixed yet

Thank you. Yes, it seems to have been fixed.

@GallVp
Copy link
Member Author

GallVp commented Nov 24, 2024 •
edited
Loading

Hi @FernandoDuarteF

I have applied your suggestions, fixed linting and updated the snapshots. I have also added you to the maintainers list. I hope you won't mind. Thank you!

@mahesh-panchal
Copy link
Member

Should we be following https://nf-co.re/docs/guidelines/components/modules#configuration-of-extargs-in-tests in subworkflows too?

@FernandoDuarteF
Copy link
Contributor

Awesome. Looks to me like everything is correct.

Thank you @GallVp!

@mahesh-panchal
Copy link
Member

Should we be following https://nf-co.re/docs/guidelines/components/modules#configuration-of-extargs-in-tests in subworkflows too?

Can you apply this guideline too to the subworkflow as well please?

@mahesh-panchal
Copy link
Member

FYI, we're formalizing the use of params to supply ext.args in subworkflows too now: https://github.com/nf-core/website/pull/2863/files

@GallVp
Copy link
Member Author

GallVp commented Nov 25, 2024

FYI, we're formalizing the use of params to supply ext.args in subworkflows too now: https://github.com/nf-core/website/pull/2863/files

Thank you @mahesh-panchal
In this case we only have a single nextflow.config and only a single process is configured. An additional layer of abstraction is perhaps unnecessary?

@mahesh-panchal
Copy link
Member

It's not necessary, but we do like uniformity. There's an issue to implement linting for this in tools. It'll make it easier to extend tests in future too ( perhaps Seqera AI could generate more comprehensive test coverage in future), and it means the args are transparent in the test file.

mahesh-panchal
mahesh-panchal approved these changes Nov 25, 2024
View reviewed changes
Copy link
Member

@mahesh-panchal mahesh-panchal left a comment

Choose a reason for hiding this comment

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

My only real quibble is the use of inject null. The others are suggestions if you like them better.

subworkflows/nf-core/fasta_gxf_busco_plot/main.nf Outdated
ch_versions = Channel.empty()
ch_db_path = val_busco_lineages_path
? Channel.of(file(val_busco_lineages_path, checkIfExists: true))
: Channel.of(null)
Copy link
Member

Choose a reason for hiding this comment

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

This is strange. We should never put null into a channel.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to Channel.of ( [ [] ] )

subworkflows/nf-core/fasta_gxf_busco_plot/main.nf Outdated
Comment on lines 61 to 62
ch_busco_assembly_inputs.map { meta, fasta, mode, lineage, db, config -> db ?: [] },
ch_busco_assembly_inputs.map { meta, fasta, mode, lineage, db, config -> config ?: [] }
Copy link
Member

Choose a reason for hiding this comment

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

If you swap the nulls above for empty lists (empty lists of lists though perhaps), you don't need these conditionals here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed the ?: business. Good idea!

subworkflows/nf-core/fasta_gxf_busco_plot/main.nf Outdated
// MODULE: BUSCO_GENERATEPLOT as PLOT_ASSEMBLY
ch_assembly_plot_summary = ch_assembly_short_summaries_txt
| map { meta, txt ->
def lineage_name = meta.lineage.split('_odb')[0]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def lineage_name = meta.lineage.split('_odb')[0]
def lineage_name = meta.lineage - '_odb'

I think this also works.

Copy link
Member Author

Choose a reason for hiding this comment

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

Applied this in a single commit.

subworkflows/nf-core/fasta_gxf_busco_plot/main.nf Show resolved Hide resolved
subworkflows/nf-core/fasta_gxf_busco_plot/main.nf Show resolved Hide resolved
subworkflows/nf-core/fasta_gxf_busco_plot/main.nf Outdated
ch_versions = ch_versions.mix(PLOT_ASSEMBLY.out.versions)

// MODULE: GFFREAD as EXTRACT_PROTEINS
ch_gffread_inputs = ! ( val_mode == 'geno' || val_mode == 'genome' )
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ch_gffread_inputs = ! ( val_mode == 'geno' || val_mode == 'genome' )
ch_gffread_inputs = ! ( val_mode in [ 'geno', 'genome' ] )

!in should also work, but I think the language server complains ( or it did the last time I used it ).

Copy link
Member Author

Choose a reason for hiding this comment

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

Switched to val_mode !in [ 'geno', 'genome' ] and the server did not complain.

@GallVp
Copy link
Member Author

GallVp commented Nov 25, 2024

It's not necessary, but we do like uniformity. There's an issue to implement linting for this in tools. It'll make it easier to extend tests in future too ( perhaps Seqera AI could generate more comprehensive test coverage in future), and it means the args are transparent in the test file.

Good idea. I have applied this now.

@GallVp GallVp force-pushed the fasta_gxf_busco_plot branch from 5265979 to 9f1764f Compare November 25, 2024 21:35
@GallVp
Copy link
Member Author

GallVp commented Nov 25, 2024

Hi @mahesh-panchal , @FernandoDuarteF

Thank you for approving the swf. I have applied your suggestions. Can you kindly quickly skim through the code once more to make sure everything is as expect.

mahesh-panchal
mahesh-panchal approved these changes Nov 26, 2024
View reviewed changes
Copy link
Member

@mahesh-panchal mahesh-panchal left a comment

Choose a reason for hiding this comment

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

🚀

@GallVp GallVp added this pull request to the merge queue Nov 26, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 26, 2024
@GallVp GallVp enabled auto-merge November 26, 2024 09:12
@FernandoDuarteF
Copy link
Contributor

Looks all good to me.

@GallVp GallVp added this pull request to the merge queue Nov 26, 2024
Merged via the queue into nf-core:master with commit 3628d82 Nov 26, 2024
22 checks passed
@GallVp GallVp deleted the fasta_gxf_busco_plot branch November 26, 2024 21:23
LouisLeNezet pushed a commit to LouisLeNezet/modules that referenced this pull request Dec 4, 2024
@GallVp @FernandoDuarteF
Added subworkflow fasta_gxf_busco_plot (nf-core#7051)
be838cd 
* Added subworkflow fasta_gxf_busco_plot

* Update subworkflows/nf-core/fasta_gxf_busco_plot/main.nf

Co-authored-by: Fernando Duarte <[email protected]>

* Update subworkflows/nf-core/fasta_gxf_busco_plot/main.nf

Co-authored-by: Fernando Duarte <[email protected]>

* Update subworkflows/nf-core/fasta_gxf_busco_plot/main.nf

Co-authored-by: Fernando Duarte <[email protected]>

* Update subworkflows/nf-core/fasta_gxf_busco_plot/main.nf

Co-authored-by: Fernando Duarte <[email protected]>

* Fixed linting and updated snapshot

* Applied suggestions

---------

Co-authored-by: Fernando Duarte <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FernandoDuarteF FernandoDuarteF FernandoDuarteF approved these changes

@mahesh-panchal mahesh-panchal mahesh-panchal approved these changes

@chriswyatt1 chriswyatt1 Awaiting requested review from chriswyatt1

@SPPearce SPPearce Awaiting requested review from SPPearce

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

Successfully merging this pull request may close these issues.

new subworkflow: FASTA_GXF_BUSCO_PLOT
5 participants
@GallVp @chriswyatt1 @SPPearce @mahesh-panchal @FernandoDuarteF