Skip to content

Skip binning prep when no binning/aDNA#470

Merged
jfy133 merged 6 commits intodevfrom
skip-binning-prep
Jul 6, 2023
Merged

Skip binning prep when no binning/aDNA#470
jfy133 merged 6 commits intodevfrom
skip-binning-prep

Conversation

@jfy133
Copy link
Copy Markdown
Member

@jfy133 jfy133 commented Jul 5, 2023

As reported by @prototaxites on slack

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

@jfy133 jfy133 changed the base branch from master to dev July 5, 2023 08:26
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jul 5, 2023

This PR is against the master branch ❌

  • Do not close this PR
  • Click Edit and change the base to dev
  • This CI test will remain failed until you push a new commit

Hi @jfy133,

It looks like this pull-request is has been made against the nf-core/mag master branch.
The master branch on nf-core repositories should always contain code from the latest release.
Because of this, PRs to master are only allowed if they come from the nf-core/mag dev branch.

You do not need to close this PR, you can change the target branch to dev by clicking the "Edit" button at the top of this page.
Note that even after this, the test will continue to show as failing until you push a new commit.

Thanks again for your contribution!

jfy133
jfy133 commented Jul 5, 2023
View reviewed changes
Comment thread CHANGELOG.md Outdated
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jul 5, 2023 •
edited
Loading

nf-core lint overall result: Failed ❌

Posted for pipeline commit 54869cf

+| ✅ 149 tests passed       |+
#| ❔   1 tests were ignored |#
!| ❗   1 tests had warnings |!
-| ❌   7 tests failed       |-
Details

❌ Test failures:

  • nextflow_config - Config variable not found: params.validationShowHiddenParams
  • nextflow_config - Config variable not found: params.validationSchemaIgnoreParams
  • files_unchanged - .github/CONTRIBUTING.md does not match the template
  • files_unchanged - .github/ISSUE_TEMPLATE/bug_report.yml does not match the template
  • files_unchanged - assets/nf-core-mag_logo_light.png does not match the template
  • schema_params - Param schema_ignore_params from nextflow config not found in nextflow_schema.json
  • multiqc_config - 'assets/multiqc_config.yml' does not contain a matching 'report_comment'.

❗ 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.9
  • Run at 2023-07-06 09:22:33

prototaxites
prototaxites reviewed Jul 5, 2023
View reviewed changes
Comment thread workflows/mag.nf Outdated
@prototaxites
Copy link
Copy Markdown
Contributor

prototaxites commented Jul 5, 2023 •
edited
Loading

Github won't let me leave a comment there but line 948 in mag.nf:

    ch_multiqc_files = ch_multiqc_files.mix(BINNING_PREPARATION.out.bowtie2_assembly_multiqc.collect().ifEmpty([]))

Needs to be escaped as well as BINNING_PREPARATION.out doesn't exist if we skip it!

edit: also lines 956-958 need to include skip_binning:

    if (!params.skip_binqc && params.binqc_tool == 'busco'){
        ch_multiqc_files = ch_multiqc_files.mix(BUSCO_QC.out.multiqc.collect().ifEmpty([]))
    }

@prototaxites
Copy link
Copy Markdown
Contributor

prototaxites commented Jul 5, 2023

With those changes, binning entry mostly seems to run fine - but I'm getting a weird error with Centrifuge that's only occurring sporadically:

-[nf-core/mag] Pipeline completed with errors-
ERROR ~ Error executing process > 'NFCORE_MAG:MAG:CENTRIFUGE (test_minigut-minigut_cf)'

Caused by:
  java.util.NoSuchElementException -- Check script './workflows/../modules/local/centrifuge.nf' at line: 21

Source block:
  def input = meta.single_end ? "-U \"${reads}\"" :  "-1 \"${reads[0]}\" -2 \"${reads[1]}\""
  prefix = task.ext.prefix ?: "${meta.id}"
  """
      centrifuge -x "${db_name}" \
          -p ${task.cpus} \
          --report-file report.txt \
          -S results.txt \
          $input
      centrifuge-kreport -x "${db_name}" results.txt > ${prefix}.centrifuge_kreport.txt
      cat results.txt | cut -f 1,3 > results.krona
  
      cat <<-END_VERSIONS > versions.yml
      "${task.process}":
          centrifuge: \$(centrifuge --version | sed -n 1p | sed 's/^.*centrifuge-class version //')
      END_VERSIONS
      """

Tip: you can try to figure out what's wrong by changing to the process work dir and showing the script file named `.command.sh`

 -- Check '.nextflow.log' file for details

I can't figure out a way to reliably reproduce it, and foolishly lost .nextflow.log somewhere in my attempts to do so. Line 21 is the one that defines $prefix - perhaps there is some kind of race condition where if centrifuge at the wrong time, something has modified the meta map in an unsafe way?

prototaxites
prototaxites reviewed Jul 5, 2023
View reviewed changes
Comment thread workflows/mag.nf Outdated
@jfy133 jfy133 requested a review from prototaxites July 6, 2023 09:20
@jfy133
Copy link
Copy Markdown
Member Author

jfy133 commented Jul 6, 2023

@prototaxites failure is linting which will be resolved in #467

prototaxites
prototaxites approved these changes Jul 6, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@prototaxites prototaxites left a comment

Choose a reason for hiding this comment

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

LGTM!

@jfy133 jfy133 merged commit da775f8 into dev Jul 6, 2023
@jfy133 jfy133 deleted the skip-binning-prep branch July 6, 2023 13:34
This was referenced Jul 10, 2023
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@prototaxites prototaxites prototaxites approved these changes

@d4straub d4straub Awaiting requested review from d4straub d4straub is a code owner

@skrakau skrakau Awaiting requested review from skrakau

@maxibor maxibor Awaiting requested review from maxibor

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

@jfy133 @prototaxites