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

Updated channels #398

Merged
merged 12 commits into from
Sep 3, 2024
Merged

Updated channels #398

merged 12 commits into from
Sep 3, 2024

Conversation

nschcolnicov
Copy link
Contributor

Addressing: #390

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/smrnaseq branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core lint).
  • Ensure the test suite passes (nf-test test main.nf.test -profile test,docker).
  • 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).

@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 29, 2024 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit c9c47d4

+| ✅ 214 tests passed       |+
#| ❔   1 tests were ignored |#
!| ❗   3 tests had warnings |!

❗ Test warnings:

  • pipeline_todos - TODO string in main.nf: Optionally add in-text citation tools to this list.
  • pipeline_todos - TODO string in main.nf: Optionally add bibliographic entries to this list.
  • pipeline_todos - TODO string in main.nf: Only uncomment below if logic in toolCitationText/toolBibliographyText has been filled!

❔ Tests ignored:

  • nextflow_config - Config default ignored: params.fastp_known_mirna_adapters

✅ Tests passed:

Run details

  • nf-core/tools version 2.14.1
  • Run at 2024-09-03 12:24:08

@nschcolnicov
Copy link
Contributor Author

nschcolnicov commented Aug 29, 2024 •
edited
Loading

nf-test observations

For the test profiles using bowtie_index such as test_umi and test_skipfastp, I observed that the dev branch is not running the BAM_SORT_STATS_SAMTOOLS subworkflow on all samples.

Issue Details

This issue arises because when the workflow uses the indexing process to generate bowtie_index, it creates an output channel that contains the bowtie_index. However, when the bowtie_index is provided from params, it is handled as a Groovy variable. This variable gets consumed on the first run of the process and is then not available for the remaining samples.

Missing Processes

While running test_skipfastp in both dev and this branch, I was noted that in dev, the following processes were missing:

NFCORE_SMRNASEQ:GENOME_QUANT:BAM_SORT_STATS_SAMTOOLS:SAMTOOLS_INDEX (Clone1_N1_mature_hairpin_genome)
NFCORE_SMRNASEQ:GENOME_QUANT:BAM_SORT_STATS_SAMTOOLS:SAMTOOLS_SORT (Clone1_N1_mature_hairpin_genome)
NFCORE_SMRNASEQ:GENOME_QUANT:BAM_SORT_STATS_SAMTOOLS:BAM_STATS_SAMTOOLS:SAMTOOLS_STATS (Clone1_N1_mature_hairpin_genome)
PREPARE_GENOME:UNTAR_BOWTIE_INDEX (bowtie_index.tar.gz)
NFCORE_SMRNASEQ:GENOME_QUANT:BAM_SORT_STATS_SAMTOOLS:BAM_STATS_SAMTOOLS:SAMTOOLS_IDXSTATS (Clone1_N1_mature_hairpin_genome)
NFCORE_SMRNASEQ:GENOME_QUANT:BAM_SORT_STATS_SAMTOOLS:BAM_STATS_SAMTOOLS:SAMTOOLS_FLAGSTAT (Clone1_N1_mature_hairpin_genome)

This issue was also present when running master using the test_skipfastp profile.

Conclusion

These issues are the reasons why the tests are failing. However, these are not issues introduced by these changes. Instead, these changes highlight some pre-existing issues. I will proceed to update the tests with this branch that contains the fixes.

Regarding all of the observed nf-test errors raised in CI

test_skipfastp

  • UNTAR_BOWTIE_INDEX is not in the versions snapshot anymore because it now runs outside of the workflow in PREPARE_GENOME.
  • dev is missing running BAM_SORT_STATS_SAMTOOLS on some samples as explained above.

test_mirgenedb

  • Ingested and changed the name of the mirgenedb mature and hairpin FASTA files to work with Channel.fromPath. Previously, it was using Groovy's file() function, which works with odd paths, but not with Channel.fromPath.
  • UNTAR_BOWTIE_INDEX is not in the versions snapshot anymore because it now runs outside of the workflow in PREPARE_GENOME.
  • BAM files changed md5sum because they contain the command used to run, and I changed the name of the mature and hairpin FASTA files.

test_umi

  • UNTAR_BOWTIE_INDEX is not in the versions snapshot anymore because it now runs outside of the workflow in PREPARE_GENOME.
  • dev is missing running BAM_SORT_STATS_SAMTOOLS on some samples as explained above.

** You might also see another failed assertion about "genome.1.ebwt not found." but I already fixed this in my local branch, it had to do with the naming of the folder where the .ebwt file was stored, when I added metadata to the bowtie_index channel, the untar module has a way of processing where it decides on the folder name based on whether if it finds a meta.id value in the input metadata or not. **

@nschcolnicov nschcolnicov mentioned this pull request Aug 30, 2024
Merged
10 tasks
@nschcolnicov nschcolnicov marked this pull request as ready for review August 30, 2024 20:00
atrigila
atrigila approved these changes Sep 2, 2024
View reviewed changes
subworkflows/local/contaminant_filter.nf Outdated Show resolved Hide resolved
subworkflows/local/genome_quant.nf Outdated Show resolved Hide resolved
subworkflows/local/mirna_quant.nf Outdated Show resolved Hide resolved
subworkflows/local/mirna_quant.nf Outdated Show resolved Hide resolved
subworkflows/local/mirna_quant.nf Outdated Show resolved Hide resolved
workflows/smrnaseq.nf Outdated Show resolved Hide resolved
subworkflows/local/prepare_genome.nf Outdated Show resolved Hide resolved
workflows/smrnaseq.nf Outdated Show resolved Hide resolved
workflows/smrnaseq.nf Outdated Show resolved Hide resolved
workflows/smrnaseq.nf Show resolved Hide resolved
@nschcolnicov nschcolnicov merged commit ba86791 into nf-core:dev Sep 3, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atrigila atrigila atrigila approved these changes

@apeltzer apeltzer Awaiting requested review from apeltzer

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

Successfully merging this pull request may close these issues.
2 participants
@nschcolnicov @atrigila