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

Skip binning prep when no binning/aDNA #470

Merged
merged 6 commits into from
Jul 6, 2023
Merged

Skip binning prep when no binning/aDNA #470

merged 6 commits into from
Jul 6, 2023

Conversation

jfy133
Copy link
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

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
CHANGELOG.md Outdated Show resolved Hide resolved
@github-actions
Copy link

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

❌ 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
workflows/mag.nf Outdated Show resolved Hide resolved
@prototaxites
Copy link
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
Contributor

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
workflows/mag.nf Outdated Show resolved Hide resolved
@jfy133 jfy133 requested a review from prototaxites July 6, 2023 09:20
@jfy133
Copy link
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
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

@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