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

Allow closures in file/url for trace,report,timeline,dag,weblog #3651

Closed

Conversation

fbdtemme
Copy link
Contributor

This adds the option to pass a closure to the following configuration variables:

  • trace.file
  • report.file
  • timeline.file
  • dag.file
  • weblog.url

This closure will be resolved in the script execution context but additionally has access to workflow and nextflow.

Closes: #3626

Signed-off-by: Florian De Temmerman <florian.detemmerman@lizard.bio>
Signed-off-by: Florian De Temmerman <florian.detemmerman@lizard.bio>
Signed-off-by: Florian De Temmerman <florian.detemmerman@lizard.bio>
@fbdtemme fbdtemme changed the title Allow closures in file/uri for trace,report,timeline,dag,weblog Allow closures in file/url for trace,report,timeline,dag,weblog Feb 16, 2023
@bentsherman
Copy link
Member

@pditommaso what do you think about this idea? I think it would be useful to access variables like the run name in the config. I will need to update this PR but the changes appear to be straightforward.

@pditommaso
Copy link
Member

Not sure we should this, plain params can be used to change those values; along with this the workflow was never meant to accessed in the config file

@zhilizheng
Copy link

zhilizheng commented Aug 3, 2023

@pditommaso Thanks for the update. For analysis, we would run the same pipeline multiple times with differerent trials, this is very common in analysis (generate log run1, run2, run3..., the users would remember those runName in their note). The runName is the only unique identiy for the users to remember what they did and recall from nextflow command line. We would not like to change the name in the configuration file each time, especially for the trace or report. A way to automatic those input would be very useful, because users don't need to remember those, and can put it into the $HOME/.nextflow/config for routines.
I believe, the pipeline configuration shall be mininal for everyday usage. So, a common file would make the user forget those essentials, and keep the project very clean and straightforward for everyday try-and-error works. Or, we need to copy those configuration part again and again for each project, it increased the mental efforts.

@bentsherman
Copy link
Member

Paolo, the main point here is to use the run name in workflow outputs. Can't do that with a param. While I agree that workflow shouldn't be accessible in the config file in general, I see nothing wrong with using it in a closure in the config file. Many config options already support this, like the publishDir path. These report paths should have the same capabilities as publishDir path, since they are often published to the same location.

Signed-off-by: Ben Sherman <bentshermann@gmail.com>
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
@pditommaso
Copy link
Member

I'm concerned about the increasing complexity of the configuration file, tho it's true that dynamic values are already supported in the process directives configuration.

However, this would bring the inconsistency to yet another level. Why .file setting could be resolved dynamically and not all settings?

@bentsherman
Copy link
Member

Why .file setting could be resolved dynamically and not all settings?

In theory, yes every config setting could be dynamic. But because each dynamic setting must be configured manually to accept and evaluate a closure, such dynamic settings have been added on an as-needed basis. I don't think that is a reason against making these particular settings dynamic, unless you want to make some generic solution that makes all settings dynamic.

@bentsherman bentsherman self-requested a review August 4, 2023 17:18
@netlify
Copy link

netlify bot commented Aug 4, 2023

Deploy Preview for nextflow-docs-staging canceled.

Name Link
🔨 Latest commit 7b53394
🔍 Latest deploy log https://app.netlify.com/sites/nextflow-docs-staging/deploys/64d24813e0b12b0008cc9cd4

@bentsherman
Copy link
Member

bentsherman commented Aug 4, 2023

I added config.navigateDynamic() to fetch a config option and evaluate it if it's a closure. You just have to give it a binding. Should be a good way to support closures for config options outside of the process scope.

@bentsherman
Copy link
Member

This could also simplify slightly the convention that nf-core uses. Currently they have to fetch their own timestamp:

def trace_timestamp = new java.util.Date().format( 'yyyy-MM-dd_HH-mm-ss')
report.file = "${params.outdir}/pipeline_info/execution_report_${trace_timestamp}.html"

With this change they could use the timestamp that Nextflow already creates:

report.file = { "${params.outdir}/pipeline_info/execution_report_${workflow.start}.html" }

Not sure the best way to format an OffsetDateTime, but you get the idea. This way the timestamps are consistent.

Signed-off-by: Ben Sherman <bentshermann@gmail.com>
@pditommaso
Copy link
Member

This will introduce yet another exception in the resolution of the config file in which some parts are dynamic and others are not.

This use case can be addressed by passing a a custom parameter to give the file a different name. Closing without merging.

@pditommaso pditommaso closed this Sep 3, 2023
@bentsherman
Copy link
Member

No, it cannot be addressed by a param. They want to use the run name which is only available in the workflow metadata. Not wanting to make more settings dynamic is not a good enough reason to not do it IMO. Allowing closures here would enable something that is currently not possible.

@risoms
Copy link

risoms commented Nov 6, 2024

@fbdtemme @bentsherman Is there any plan on releasing this?

@bentsherman
Copy link
Member

I don't think so. I suggest using a param as Paolo suggests, even though it requires a bit more logic to set the run name and the param on the command line.

I would like to find a way to better control these report filenames, but I think we can find a better way than just using a closure

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