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.

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

Dev -> Master for 2.2 release #1353

Merged
merged 360 commits into from
Dec 14, 2021
Merged

Dev -> Master for 2.2 release #1353

merged 360 commits into from
Dec 14, 2021

Conversation

drpatelh
Copy link
Member

@drpatelh drpatelh commented Dec 9, 2021

No description provided.

drpatelh and others added 30 commits September 30, 2021 09:08
 Update modules in pipeline template with new versions syntax
Update template version of the dumpsoftwareversions module
Passing auth parameters with requests.get
Fix sha query param in get_module_git_log to get log from private branches
passing repo_name information while searching for local modules info
suppress 'DeprecationWarning: invalid escape sequence \.' by using patterns explicitely
tests/test_download.py::DownloadTest::test_wf_use_local_configs was failing forever after first attempt since the first attempt cached a configuration file which is reloaded in all the following cases. Now we disable caching in $HOME/.nextflow/nf-core while during tests
Fixed bug in `nf-core list` when `NXF_HOME` is set
change out-dated `/software/` paths to `/modules/
Copy link
Member

@JoseEspinosa JoseEspinosa left a comment

Choose a reason for hiding this comment

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

LGTM! Just made some small suggestions!

.github/workflows/create-test-wf.yml Outdated Show resolved Hide resolved
nf_core/lint/files_exist.py Show resolved Hide resolved
nf_core/pipeline-template/lib/NfcoreTemplate.groovy Outdated Show resolved Hide resolved
@@ -26,22 +26,19 @@ params {
// Boilerplate options
outdir = './results'
tracedir = "${params.outdir}/pipeline_info"
publish_dir_mode = 'copy'
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 more a comment than a change suggestion (I also remembered this was discussed somewhere else but can't find where). After starting the conversion to DSL2 v2.0 of the chipseq pipeline, I was wondering whether maintaining a default value would be good in case someone wants to completely change the behavior of all the processes, otherwise, it will need to modify all the process modes 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, you might be right. But I suspect you may be able to use the power of NF to provide a generic override for that too? 🤔 Not sure exactly what the syntax would be but something like this:

process {
    publishDir = [
        path: { "${params.outdir}/${task.process.tokenize(':')[-1].tokenize('_')[0].toLowerCase()}" },
        mode: 'symlink',
        saveAs: { filename -> filename.equals('versions.yml') ? null : filename }
    ]
}

Copy link
Member

Choose a reason for hiding this comment

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

The problem is that this won't override all the named process selectors where the mode is harcoded, so then you will have to copy all the rest of the ext.args from the original modules.config, I'd say

Copy link
Member Author

Choose a reason for hiding this comment

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

It should do. The settings above if provided via -c custom.config should override any mode set in the pipeline for all processes. Maybe @mahesh-panchal can confirm.

Copy link
Member

@JoseEspinosa JoseEspinosa Dec 14, 2021

Choose a reason for hiding this comment

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

OK, then probably that's the part I missed, fair enough 👍
Will keep testing and I find something not working as expected then we can discuss it further.

Copy link
Member

Choose a reason for hiding this comment

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

Confirmed:
main.nf:

nextflow.enable.dsl = 2

workflow {
    FOO()
}

process FOO {

    output:
    path "table.txt", emit: txt

    script:
    """
    touch table.txt
    """
}

nextflow.config:

params.outdir = 'results'
process {
    withName: FOO {
        publishDir = [
            path: { "${params.outdir}/${task.process.tokenize(':')[-1].tokenize('_')[0].toLowerCase()}" },
            mode: 'copy',
            saveAs: { filename -> filename.equals('versions.yml') ? null : filename }
        ]
    }
}

custom.config:

params.outdir = 'results'
process {
    publishDir = [
        path: { "${params.outdir}/${task.process.tokenize(':')[-1].tokenize('_')[0].toLowerCase()}" },
        mode: 'symlink',
        saveAs: { filename -> filename.equals('versions.yml') ? null : filename }
    ]
    withName: FOO {
       publishDir = "foo"
    }
}

Output:

$ nextflow run main.nf -c custom.config 
N E X T F L O W  ~  version 21.10.0
Launching `main.nf` [focused_swirles] - revision: de5020ba1d
executor >  local (1)
[25/cdff54] process > FOO [100%] 1 of 1 ✔

$ tree -a
.
├── .nextflow
│   ├── cache
│   │   ├── aeca00af-ea5f-47fc-afe6-de3c6f46a563
│   │   │   ├── db
│   │   │   │   ├── 000003.log
│   │   │   │   ├── CURRENT
│   │   │   │   ├── LOCK
│   │   │   │   └── MANIFEST-000002
│   │   │   └── index.sleepy_mayer
│   │   └── cb644109-0e7f-4657-91fc-3ef9d15345a1
│   │       ├── db
│   │       │   ├── 000003.log
│   │       │   ├── CURRENT
│   │       │   ├── LOCK
│   │       │   └── MANIFEST-000002
│   │       └── index.focused_swirles
│   ├── history
│   └── plr
├── .nextflow.log
├── .nextflow.log.1
├── custom.config
├── foo
│   └── table.txt -> /Users/mahpa906/Downloads/Nextflow_sandbox/test_publish_mode/work/25/cdff5407da55c04b0929d82216982e/table.txt
├── main.nf
├── nextflow.config
├── results
│   └── foo
│       └── table.txt
└── work
    ├── 25
    │   └── cdff5407da55c04b0929d82216982e
    │       ├── .command.begin
    │       ├── .command.err
    │       ├── .command.log
    │       ├── .command.out
    │       ├── .command.run
    │       ├── .command.sh
    │       ├── .exitcode
    │       └── table.txt
    └── bf
        └── fef5f5ebf87254c920c447d82648ff
            ├── .command.begin
            ├── .command.err
            ├── .command.log
            ├── .command.out
            ├── .command.run
            ├── .command.sh
            ├── .exitcode
            └── table.txt

Copy link
Member

Choose a reason for hiding this comment

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

I just realised my test didn't confirm mode inheritance. It's using the default setting each time.
If you set it process wide to 'copy', but then override it for a specific instance, then the default mode 'symlink' is still used.

tests/lint/actions_ci.py Show resolved Hide resolved
@JoseEspinosa JoseEspinosa self-requested a review December 14, 2021 16:02
Warn instead of fail if module is different from remote
@drpatelh drpatelh merged commit d0ab20e into master Dec 14, 2021
@@ -78,6 +77,8 @@ def files_exist(self):
bin/markdown_to_html.r
conf/aws.config
.github/workflows/push_dockerhub.yml
.github/ISSUE_TEMPLATE/bug_report.md
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
.github/ISSUE_TEMPLATE/bug_report.md
.github/ISSUE_TEMPLATE/bug_report.md
.github/ISSUE_TEMPLATE/feature_request.md

@@ -24,5 +24,5 @@ jobs:
{
"outdir": "s3://{% raw %}${{ secrets.AWS_S3_BUCKET }}{% endraw %}/{{ short_name }}/{% raw %}results-${{ github.sha }}{% endraw %}"
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
"outdir": "s3://{% raw %}${{ secrets.AWS_S3_BUCKET }}{% endraw %}/{{ short_name }}/{% raw %}results-${{ github.sha }}{% endraw %}"
"outdir": "s3://{% raw %}${{ secrets.AWS_S3_BUCKET }}{% endraw %}/{{ short_name }}/{% raw %}results-small-tests-${{ github.sha }}{% endraw %}"

Copy link
Member

Choose a reason for hiding this comment

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

Just thought about this now, that avoids conflicts with big tests commit sha!

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, crap! So they would be put in the same directory if we don't change that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet