Light refactoring #1422

maxulysse · 2024-02-23T11:17:36Z

The linter is failing, but that is a bug in the linter which has been solved in the dev-version of the tool.

PR checklist

github-actions · 2024-02-23T11:20:17Z

`nf-core lint` overall result: Passed ✅ ⚠️

Posted for pipeline commit 100f9ea

+| ✅ 181 tests passed       |+
#| ❔  14 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:

files_exist - File is ignored: .github/workflows/awsfulltest.yml
files_exist - File is ignored: .github/workflows/awstest.yml
files_exist - File is ignored: conf/modules.config
files_exist - File is ignored: lib/WorkflowMain.groovy
files_exist - File is ignored: lib/NfcoreTemplate.groovy
files_exist - File is ignored: lib/WorkflowSarek.groovy
files_unchanged - File ignored due to lint config: .github/PULL_REQUEST_TEMPLATE.md
files_unchanged - File ignored due to lint config: assets/nf-core-sarek_logo_light.png
files_unchanged - File ignored due to lint config: docs/images/nf-core-sarek_logo_light.png
files_unchanged - File ignored due to lint config: docs/images/nf-core-sarek_logo_dark.png
files_unchanged - File ignored due to lint config: .gitignore or .prettierignore or pyproject.toml
actions_ci - actions_ci
actions_awstest - 'awstest.yml' workflow not found: /home/runner/work/sarek/sarek/.github/workflows/awstest.yml
template_strings - template_strings

✅ Tests passed:

files_exist - File found: .gitattributes
files_exist - File found: .gitignore
files_exist - File found: .nf-core.yml
files_exist - File found: .editorconfig
files_exist - File found: .prettierignore
files_exist - File found: .prettierrc.yml
files_exist - File found: CHANGELOG.md
files_exist - File found: CITATIONS.md
files_exist - File found: CODE_OF_CONDUCT.md
files_exist - File found: LICENSE or LICENSE.md or LICENCE or LICENCE.md
files_exist - File found: nextflow_schema.json
files_exist - File found: nextflow.config
files_exist - File found: README.md
files_exist - File found: .github/.dockstore.yml
files_exist - File found: .github/CONTRIBUTING.md
files_exist - File found: .github/ISSUE_TEMPLATE/bug_report.yml
files_exist - File found: .github/ISSUE_TEMPLATE/config.yml
files_exist - File found: .github/ISSUE_TEMPLATE/feature_request.yml
files_exist - File found: .github/PULL_REQUEST_TEMPLATE.md
files_exist - File found: .github/workflows/branch.yml
files_exist - File found: .github/workflows/ci.yml
files_exist - File found: .github/workflows/linting_comment.yml
files_exist - File found: .github/workflows/linting.yml
files_exist - File found: assets/email_template.html
files_exist - File found: assets/email_template.txt
files_exist - File found: assets/sendmail_template.txt
files_exist - File found: assets/nf-core-sarek_logo_light.png
files_exist - File found: conf/test.config
files_exist - File found: conf/test_full.config
files_exist - File found: docs/images/nf-core-sarek_logo_light.png
files_exist - File found: docs/images/nf-core-sarek_logo_dark.png
files_exist - File found: docs/output.md
files_exist - File found: docs/README.md
files_exist - File found: docs/README.md
files_exist - File found: docs/usage.md
files_exist - File found: main.nf
files_exist - File found: assets/multiqc_config.yml
files_exist - File found: conf/base.config
files_exist - File found: conf/igenomes.config
files_exist - File found: modules.json
files_exist - File found: pyproject.toml
files_exist - File not found check: Singularity
files_exist - File not found check: parameters.settings.json
files_exist - File not found check: pipeline_template.yml
files_exist - File not found check: .nf-core.yaml
files_exist - File not found check: bin/markdown_to_html.r
files_exist - File not found check: conf/aws.config
files_exist - File not found check: .github/workflows/push_dockerhub.yml
files_exist - File not found check: .github/ISSUE_TEMPLATE/bug_report.md
files_exist - File not found check: .github/ISSUE_TEMPLATE/feature_request.md
files_exist - File not found check: docs/images/nf-core-sarek_logo.png
files_exist - File not found check: .markdownlint.yml
files_exist - File not found check: .yamllint.yml
files_exist - File not found check: lib/Checks.groovy
files_exist - File not found check: lib/Completion.groovy
files_exist - File not found check: lib/Workflow.groovy
files_exist - File not found check: lib/Utils.groovy
files_exist - File not found check: lib/nfcore_external_java_deps.jar
files_exist - File not found check: .travis.yml
nextflow_config - Config variable found: manifest.name
nextflow_config - Config variable found: manifest.nextflowVersion
nextflow_config - Config variable found: manifest.description
nextflow_config - Config variable found: manifest.version
nextflow_config - Config variable found: manifest.homePage
nextflow_config - Config variable found: timeline.enabled
nextflow_config - Config variable found: trace.enabled
nextflow_config - Config variable found: report.enabled
nextflow_config - Config variable found: dag.enabled
nextflow_config - Config variable found: process.cpus
nextflow_config - Config variable found: process.memory
nextflow_config - Config variable found: process.time
nextflow_config - Config variable found: params.outdir
nextflow_config - Config variable found: params.input
nextflow_config - Config variable found: params.validationShowHiddenParams
nextflow_config - Config variable found: params.validationSchemaIgnoreParams
nextflow_config - Config variable found: manifest.mainScript
nextflow_config - Config variable found: timeline.file
nextflow_config - Config variable found: trace.file
nextflow_config - Config variable found: report.file
nextflow_config - Config variable found: dag.file
nextflow_config - Config variable (correctly) not found: params.nf_required_version
nextflow_config - Config variable (correctly) not found: params.container
nextflow_config - Config variable (correctly) not found: params.singleEnd
nextflow_config - Config variable (correctly) not found: params.igenomesIgnore
nextflow_config - Config variable (correctly) not found: params.name
nextflow_config - Config variable (correctly) not found: params.enable_conda
nextflow_config - Config timeline.enabled had correct value: true
nextflow_config - Config report.enabled had correct value: true
nextflow_config - Config trace.enabled had correct value: true
nextflow_config - Config dag.enabled had correct value: true
nextflow_config - Config manifest.name began with nf-core/
nextflow_config - Config variable manifest.homePage began with https://github.com/nf-core/
nextflow_config - Config dag.file ended with .html
nextflow_config - Config variable manifest.nextflowVersion started with >= or !>=
nextflow_config - Config manifest.version ends in dev: 3.5dev
nextflow_config - Config params.custom_config_version is set to master
nextflow_config - Config params.custom_config_base is set to https://raw.githubusercontent.com/nf-core/configs/master
nextflow_config - Lines for loading custom profiles found
nextflow_config - nextflow.config contains configuration profile test
nextflow_config - Config default value correct: params.step= mapping
nextflow_config - Config default value correct: params.split_fastq= 50000000
nextflow_config - Config default value correct: params.nucleotides_per_second= 200000
nextflow_config - Config default value correct: params.clip_r1= 0
nextflow_config - Config default value correct: params.clip_r2= 0
nextflow_config - Config default value correct: params.three_prime_clip_r1= 0
nextflow_config - Config default value correct: params.three_prime_clip_r2= 0
nextflow_config - Config default value correct: params.trim_nextseq= 0
nextflow_config - Config default value correct: params.group_by_umi_strategy= Adjacency
nextflow_config - Config default value correct: params.aligner= bwa-mem
nextflow_config - Config default value correct: params.ascat_min_base_qual= 20
nextflow_config - Config default value correct: params.ascat_min_counts= 10
nextflow_config - Config default value correct: params.ascat_min_map_qual= 35
nextflow_config - Config default value correct: params.cf_coeff= 0.05
nextflow_config - Config default value correct: params.cf_contamination= 0
nextflow_config - Config default value correct: params.cf_minqual= 0
nextflow_config - Config default value correct: params.cf_mincov= 0
nextflow_config - Config default value correct: params.cf_ploidy= 2
nextflow_config - Config default value correct: params.sentieon_haplotyper_emit_mode= variant
nextflow_config - Config default value correct: params.sentieon_dnascope_emit_mode= variant
nextflow_config - Config default value correct: params.sentieon_dnascope_pcr_indel_model= CONSERVATIVE
nextflow_config - Config default value correct: params.dbnsfp_fields= rs_dbSNP,HGVSc_VEP,HGVSp_VEP,1000Gp3_EAS_AF,1000Gp3_AMR_AF,LRT_score,GERP++_RS,gnomAD_exomes_AF
nextflow_config - Config default value correct: params.vep_custom_args= --everything --filter_common --per_gene --total_length --offline --format vcf
nextflow_config - Config default value correct: params.vep_version= 110.0-0
nextflow_config - Config default value correct: params.vep_out_format= vcf
nextflow_config - Config default value correct: params.genome= GATK.GRCh38
nextflow_config - Config default value correct: params.igenomes_base= s3://ngi-igenomes/igenomes/
nextflow_config - Config default value correct: params.vep_cache= s3://annotation-cache/vep_cache/
nextflow_config - Config default value correct: params.snpeff_cache= s3://annotation-cache/snpeff_cache/
nextflow_config - Config default value correct: params.custom_config_version= master
nextflow_config - Config default value correct: params.custom_config_base= https://raw.githubusercontent.com/nf-core/configs/master
nextflow_config - Config default value correct: params.test_data_base= https://raw.githubusercontent.com/nf-core/test-datasets/sarek3
nextflow_config - Config default value correct: params.seq_platform= ILLUMINA
nextflow_config - Config default value correct: params.max_cpus= 16
nextflow_config - Config default value correct: params.max_memory= 128.GB
nextflow_config - Config default value correct: params.max_time= 240.h
nextflow_config - Config default value correct: params.publish_dir_mode= copy
nextflow_config - Config default value correct: params.max_multiqc_email_size= 25.MB
nextflow_config - Config default value correct: params.validate_params= true
files_unchanged - .gitattributes matches the template
files_unchanged - .prettierrc.yml matches the template
files_unchanged - CODE_OF_CONDUCT.md matches the template
files_unchanged - LICENSE matches the template
files_unchanged - .github/.dockstore.yml matches the template
files_unchanged - .github/CONTRIBUTING.md matches the template
files_unchanged - .github/ISSUE_TEMPLATE/bug_report.yml matches the template
files_unchanged - .github/ISSUE_TEMPLATE/config.yml matches the template
files_unchanged - .github/ISSUE_TEMPLATE/feature_request.yml matches the template
files_unchanged - .github/workflows/branch.yml matches the template
files_unchanged - .github/workflows/linting_comment.yml matches the template
files_unchanged - .github/workflows/linting.yml matches the template
files_unchanged - assets/email_template.html matches the template
files_unchanged - assets/email_template.txt matches the template
files_unchanged - assets/sendmail_template.txt matches the template
files_unchanged - docs/README.md matches the template
readme - README Nextflow minimum version badge matched config. Badge: 23.04.0, Config: 23.04.0
readme - README Zenodo placeholder was replaced with DOI.
pipeline_name_conventions - Name adheres to nf-core convention
schema_lint - Schema lint passed
schema_lint - Schema title + description lint passed
schema_lint - Input mimetype lint passed: 'text/csv'
schema_params - Schema matched params returned from nextflow config
system_exit - No System.exit calls found
actions_schema_validation - Workflow validation passed: fix-linting.yml
actions_schema_validation - Workflow validation passed: linting_comment.yml
actions_schema_validation - Workflow validation passed: clean-up.yml
actions_schema_validation - Workflow validation passed: branch.yml
actions_schema_validation - Workflow validation passed: ci.yml
actions_schema_validation - Workflow validation passed: release-announcements.yml
actions_schema_validation - Workflow validation passed: cloudtest.yml
actions_schema_validation - Workflow validation passed: ncbench.yml
actions_schema_validation - Workflow validation passed: download_pipeline.yml
actions_schema_validation - Workflow validation passed: linting.yml
merge_markers - No merge markers found in pipeline files
modules_json - Only installed modules found in modules.json
multiqc_config - 'assets/multiqc_config.yml' contains report_section_order
multiqc_config - 'assets/multiqc_config.yml' contains export_plots
multiqc_config - 'assets/multiqc_config.yml' contains report_comment
multiqc_config - 'assets/multiqc_config.yml' follows the ordering scheme of the minimally required plugins.
multiqc_config - 'assets/multiqc_config.yml' contains a matching 'report_comment'.
multiqc_config - 'assets/multiqc_config.yml' contains 'export_plots: true'.
modules_structure - modules directory structure is correct 'modules/nf-core/TOOL/SUBTOOL'

Run details

nf-core/tools version 2.13.1
Run at 2024-04-03 17:05:37

asp8200 · 2024-02-23T11:30:25Z

So far this PR seems like it is adding some stuff and not refactoring 😄 Anyways, linter says

"Param modules_testdata_base_path from nextflow config not found in nextflow_schema.json"

maxulysse · 2024-02-23T11:37:28Z

sorry, this is an ongoing PR, I should have put it as a draft

FriederikeHanssen

lgtm 🚀

kenibrewer

LGTM too

maxulysse · 2024-03-11T15:00:11Z

sorry still need to fix tumor_normal_pair test

…ath(params.fasta_fai)

kenibrewer · 2024-03-12T13:35:04Z

There's a nf-core linting failure that needs to be addressed before review:

nextflow_config - Config default value incorrect: params.aligner is set as bwa-mem in nextflow_schema.json but is null in nextflow.config.

main.nf

asp8200 · 2024-03-21T08:21:08Z

In 4575cba, then full-test is still not running on awsbatch :-/ I still get:

ERROR ~ Error executing process > 'NFCORE_SAREK:PREPARE_INTERVALS:CREATE_INTERVALS_BED (wgs_calling_regions.hg38.bed)'

Caused by:
  The user value contains invalid characters. Enter a value that matches the pattern ^([a-z0-9_][a-z0-9_-]{0,30})$

Command executed:

  awk -vFS="    " '{
      t = $5  # runtime estimate
      if (t == "") {
          # no runtime estimate in this row, assume default value
          t = ($3 - $2) / 200000
      }
      if (name == "" || (chunk > 600 && (chunk + t) > longest * 1.05)) {
          # start a new chunk
          name = sprintf("%s_%d-%d.bed", $1, $2+1, $3)
          chunk = 0
          longest = 0
      }
      if (t > longest)
          longest = t
      chunk += t
      print $0 > name
  }' wgs_calling_regions.hg38.bed

  cat <<-END_VERSIONS > versions.yml
  "NFCORE_SAREK:PREPARE_INTERVALS:CREATE_INTERVALS_BED":
      gawk: $(awk -Wversion | sed '1!d; s/.*Awk //; s/,.*//')
  END_VERSIONS

Command exit status:
  -

Command output:
  (empty)

(The full-tests also fails no the dev-branch, but due to other bugs which have now been fixed in this PR.)

The "small" test-profile also fail on 4575cba with a similar error but from another process:

ERROR ~ Error executing process > 'NFCORE_SAREK:PREPARE_INTERVALS:TABIX_BGZIPTABIX_INTERVAL_COMBINED (wgs_calling_regions.hg38)'

Caused by:
  The user value contains invalid characters. Enter a value that matches the pattern ^([a-z0-9_][a-z0-9_-]{0,30})$

Command executed:

  bgzip  --threads 1 -c  wgs_calling_regions.hg38.bed > wgs_calling_regions.hg38.bed.gz
  tabix  wgs_calling_regions.hg38.bed.gz

  cat <<-END_VERSIONS > versions.yml
  "NFCORE_SAREK:PREPARE_INTERVALS:TABIX_BGZIPTABIX_INTERVAL_COMBINED":
      tabix: $(echo $(tabix -h 2>&1) | sed 's/^.*Version: //; s/ .*$//')
  END_VERSIONS

Command exit status:
  -

Command output:
  (empty)

Seemingly similar issue reported here, but the fix proposed by @bentsherman didn't solve it.

clue: The "small" test-profile can run with awsbatch on the dev-branch (at least in one mamba-env), which seem to indicate that it is something in this PR that is causing the problem with awsbatch.

kenibrewer · 2024-03-21T13:40:46Z

@asp8200 Could the problem maybe be happening when we try to assign a tag to the job? Maybe AWS batch only supports tags of a certain length, or the files for the AWS Batch dataset are too long / contain illegal characters.

nextflow.config

asp8200

Not sure why stuff is being moved around, but if it works and isn't more confusing than before, then OK. By the way, the error reported from the linter can be ignored. It is a know bug in the linter.

The test and test_full now pass locally. (But not over awsbatch. However, we now know what the problem with that is, and we have at least one possible fix.)

kenibrewer · 2024-03-27T19:54:24Z

@maxulysse What still remains to be done on this PR? This is a pretty substantial bit of refactoring, and I think it would be good to get this merged back into dev sooner rather than later.

Also, as general practice for nf-core development, is it acceptable for major rewrites like this one to merge a mostly correct set of changes into dev and then address the remaining failures as PRs to that branch?

…nto refactor_light

maxulysse · 2024-04-03T16:33:33Z

@maxulysse What still remains to be done on this PR? This is a pretty substantial bit of refactoring, and I think it would be good to get this merged back into dev sooner rather than later.

Also, as general practice for nf-core development, is it acceptable for major rewrites like this one to merge a mostly correct set of changes into dev and then address the remaining failures as PRs to that branch?

I agree with you @kenibrewer,
We got a bit sidetracked, especially me, due to hackathon + team retreat.
Let's merge that ASAP and release when we can.

asp8200

LGTM. However, we still need to fix the runOptions/containerOptions-issue so that the pipeline can run over awsbatch. We do have a fix for that but it isn't great:

https://nextflow.slack.com/archives/C02T98A23U7/p1711040612294759?thread_ts=1711013350.318419&cid=C02T98A23U7

https://github.com/nf-core/sarek/pull/1430/files

maxulysse · 2024-04-04T07:26:57Z

LGTM. However, we still need to fix the runOptions/containerOptions-issue so that the pipeline can run over awsbatch. We do have a fix for that but it isn't great:

https://nextflow.slack.com/archives/C02T98A23U7/p1711040612294759?thread_ts=1711013350.318419&cid=C02T98A23U7

https://github.com/nf-core/sarek/pull/1430/files

yeah, but that's another issue

initialize params from the genome map

e8464fb

maxulysse requested a review from FriederikeHanssen as a code owner February 23, 2024 11:17

maxulysse marked this pull request as draft February 23, 2024 11:37

maxulysse added 12 commits February 23, 2024 12:41

add modules_testdata_base_path

1f3235d

proper usage of logo

9b362ec

Merge branch 'dev' into refactor_light

e6b5ec5

exclude tests

b31e299

sort tests exclusion

396f026

Merge branch 'dev' into refactor_light

64927c9

why is this file still there

9dacebe

remove more lib

cadfd86

fix duplication

d4d8eae

Merge branch 'dev' into refactor_light

9a1e0f2

small reorganization

501c5cb

all should work

76fc410

maxulysse marked this pull request as ready for review March 8, 2024 10:43

maxulysse added 2 commits March 8, 2024 12:20

small code polish

9c3fd40

update CHANGELOG

cf2411c

FriederikeHanssen previously approved these changes Mar 8, 2024

View reviewed changes

kenibrewer previously approved these changes Mar 8, 2024

View reviewed changes

Merge branch 'dev' into refactor_light

1bf5222

maxulysse dismissed stale reviews from kenibrewer and FriederikeHanssen via 1bf5222 March 11, 2024 11:03

kenibrewer previously approved these changes Mar 11, 2024

View reviewed changes

Adding .collect() to Channel.fromPath(params.fasta) and Channel.fromP…

62b4ea4

…ath(params.fasta_fai)

asp8200 dismissed kenibrewer’s stale review via 62b4ea4 March 12, 2024 13:08

asp8200 requested review from kenibrewer and FriederikeHanssen March 12, 2024 13:23

fix getGenomeAttribute

cbb2a62

asp8200 reviewed Mar 12, 2024

View reviewed changes

main.nf Show resolved Hide resolved

remove duplication

f9d1723

asp8200 mentioned this pull request Mar 15, 2024

Release v2.1.1 nf-core/bamtofastq#83

Merged

10 tasks

asp8200 added 3 commits March 21, 2024 07:37

Adding .collect() in the construction of the channels dict and fasta_fai

5ac371d

Adding ascat_loci_gc in input definition for SAMPLESHEET_TO_CHANNEL

5bd9856

Adding params.ascat_loci_gc in call to SAMPLESHEET_TO_CHANNEL

4575cba

asp8200 reviewed Mar 24, 2024

View reviewed changes

nextflow.config Outdated Show resolved Hide resolved

asp8200 previously approved these changes Mar 24, 2024

View reviewed changes

maxulysse added 2 commits April 3, 2024 18:28

refactor: chmod +x on main.nf

0e86cc2

Merge branch 'refactor_light' of github.com:maxulysse/nf-core_sarek i…

853f7d1

…nto refactor_light

maxulysse dismissed asp8200’s stale review via 853f7d1 April 3, 2024 16:28

refactor: no need for params to be ignored

ce5b800

maxulysse added 2 commits April 3, 2024 19:00

fix: restore proper aligner usage

b065df0

prettier

100f9ea

asp8200 approved these changes Apr 3, 2024

View reviewed changes

maxulysse mentioned this pull request Apr 4, 2024

Parabricks alignment #1444

Closed

10 tasks

maxulysse merged commit b14d243 into nf-core:dev Apr 4, 2024
23 checks passed

maxulysse deleted the refactor_light branch April 4, 2024 10:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Light refactoring #1422

Light refactoring #1422

maxulysse commented Feb 23, 2024 •

edited by asp8200

Loading

github-actions bot commented Feb 23, 2024 •

edited

Loading

❗ Test warnings:

❔ Tests ignored:

✅ Tests passed:

Run details

asp8200 commented Feb 23, 2024

maxulysse commented Feb 23, 2024

FriederikeHanssen left a comment

kenibrewer left a comment

maxulysse commented Mar 11, 2024

kenibrewer commented Mar 12, 2024

asp8200 commented Mar 21, 2024 •

edited

Loading

kenibrewer commented Mar 21, 2024

asp8200 left a comment •

edited

Loading

kenibrewer commented Mar 27, 2024

maxulysse commented Apr 3, 2024

asp8200 left a comment

maxulysse commented Apr 4, 2024

Light refactoring #1422

Light refactoring #1422

Conversation

maxulysse commented Feb 23, 2024 • edited by asp8200 Loading

PR checklist

github-actions bot commented Feb 23, 2024 • edited Loading

nf-core lint overall result: Passed ✅ ⚠️

❗ Test warnings:

❔ Tests ignored:

✅ Tests passed:

Run details

asp8200 commented Feb 23, 2024

maxulysse commented Feb 23, 2024

FriederikeHanssen left a comment

Choose a reason for hiding this comment

kenibrewer left a comment

Choose a reason for hiding this comment

maxulysse commented Mar 11, 2024

kenibrewer commented Mar 12, 2024

asp8200 commented Mar 21, 2024 • edited Loading

kenibrewer commented Mar 21, 2024

asp8200 left a comment • edited Loading

Choose a reason for hiding this comment

kenibrewer commented Mar 27, 2024

maxulysse commented Apr 3, 2024

asp8200 left a comment

Choose a reason for hiding this comment

maxulysse commented Apr 4, 2024

maxulysse commented Feb 23, 2024 •

edited by asp8200

Loading

github-actions bot commented Feb 23, 2024 •

edited

Loading

`nf-core lint` overall result: Passed ✅ ⚠️

asp8200 commented Mar 21, 2024 •

edited

Loading

asp8200 left a comment •

edited

Loading