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

Add option to ignore process selector warnings #2700

Closed
drpatelh opened this issue Mar 5, 2022 · 13 comments
Closed

Add option to ignore process selector warnings #2700

drpatelh opened this issue Mar 5, 2022 · 13 comments

Comments

@drpatelh
Copy link
Contributor

drpatelh commented Mar 5, 2022

Warnings are great to let developers know that they are using the correct process selectors e.g. here. However, if we are using if/else type logic in the main workflow like here then we have to replicate this in any configuration too like here. This becomes quite painful to maintain and ideally we should have an option to turn off the warnings via some sort of configuration option.

Possible suggestions:

  • An option that takes a list of process names to ignore but this could get very long for larger pipelines e.g. ignore_selector_warnings = ['KUNG', 'FOO', 'BAR']
  • A configuration option that can be set within the process scope for all processes or individual ones:
process {
    ignore_selector_warnings = true

    withName: 'FOO' {
        ignore_selector_warnings = true
    }
}
@pditommaso
Copy link
Member

Think it should be possible to detect the name of the process without adding a special notation, but only one without nested notation. I mean for example FOO but not NF-CORE:SOMETHING:FOO.

Could that be a solution for you ?

Set<String> getProcessNames() {
if( NF.dsl1 )
return new HashSet<String>(getDsl1ProcessNames())
def result = new HashSet(definitions.size() + imports.size())
// local definitions
for( def item : definitions.values() ) {
if( item instanceof ProcessDef )
result.add(item.name)
}
// processes from imports
for( def item: imports.values() ) {
if( item instanceof ProcessDef )
result.add(item.name)
}
return result
}

@drpatelh
Copy link
Contributor Author

drpatelh commented Mar 7, 2022

Do you mean by changing this line in the pipeline configuration from withName: '.*:FASTQC_UMITOOLS_TRIMGALORE:FASTQC' to withName: 'FASTQC'?

The nested notation is quite important when re-using modules in the pipeline and applying different parameters / publishing options. Be good to factor this in when ignoring the warnings.

@pditommaso
Copy link
Member

Yeah, the main problem is that it can be detected the process name even if they are within a branch not executed, but not for the fully qualified name, which requires the evaluation of that piece of code.

@drpatelh
Copy link
Contributor Author

drpatelh commented Mar 7, 2022

Hmmm...I see your point 🤔

@drpatelh
Copy link
Contributor Author

drpatelh commented Mar 7, 2022

The fully qualified names are where these warnings come in most useful e.g. if you change the name of a sub-workflow or some nesting then the warnings gets you back on the right track. If we use just the process name and change a fully qualified path to a module then we won't get a warning anymore?

@pditommaso
Copy link
Member

what about adding an option to switch it off this check. At the end is only meant to be useful for development

@drpatelh
Copy link
Contributor Author

drpatelh commented Mar 14, 2022

Sounds good in principle but sometimes users may use the pipeline with a certain set of params which show the warning that may have been missed by the developers. How do you propose adding it and would it be off by default? Having an on/off option would definitely be a good start but maybe not the best longer term option?

@mahesh-panchal
Copy link
Contributor

It's also useful for users who mis-spell selector names when providing a custom config to override settings. It would be preferable not to have the warnings disabled.

Although I do agree that the need for if's in the config is unpleasant.

Could those particular warnings be shifted to the log only as long the components ( between : ) of the fully qualified name all belong to either process or workflow namespace?

@pditommaso
Copy link
Member

I've committed 814a260 that adds the ability to disable the config process names adding in the config

nextflow.enable.configProcessNamesValidation = false

Think this can be a quick solution.

Could those particular warnings be shifted to the log only as long the components ( between : ) of the fully qualified name all belong to either process or workflow namespace?

Not sure to fully understand the logic. If you can provide an example it may help

@mahesh-panchal
Copy link
Contributor

Could those particular warnings be shifted to the log only as long the components ( between : ) of the fully qualified name all belong to either process or workflow namespace?

Not sure to fully understand the logic. If you can provide an example it may help

My initial thought was something like:

selector.tokenize(':') in namespace

where namespace is the list of process and workflow names.

Now I realize that it won't work though because the selector is a pattern, so one needs to deal with stuff like

NF_CORE:.*:PROCESS
.*:FOO(BAR|BAZ)

etc.

@stale
Copy link

stale bot commented Jan 16, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jan 16, 2023
@pditommaso
Copy link
Member

@drpatelh is this still relevant?

@stale stale bot removed the stale label Jan 16, 2023
@drpatelh
Copy link
Contributor Author

Nope. I think the config option you added should suffice given that the configuration is going to be updated in future iterations of the DSL anyway.

Just done a little testing and we could switch this off by default in the top-level of the nextflow.config in nf-core pipelines and then have a specific profile called devel that allows developers to see the warning during pipeline development.

nextflow.enable.configProcessNamesValidation = false

profiles {
    devel { nextflow.enable.configProcessNamesValidation = true }
}

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

No branches or pull requests

4 participants