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

Save built-in reports as Tower reports #4760

Merged
merged 2 commits into from Mar 12, 2024

Conversation

bentsherman
Copy link
Member

Close #3460

This PR saves the built-in reports as Tower reports by inspecting the Nextflow config. If a report is enabled, it will be eagerly sent to the reports file on workflow completion.

Note that if a report fails to create for some reason, it will be displayed in the platform run page but will not load since the file doesn't exist. In this case, the failure will have been logged as a warning to the console output.

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

netlify bot commented Feb 20, 2024

Deploy Preview for nextflow-docs-staging ready!

Name Link
🔨 Latest commit a8607b5
🔍 Latest deploy log https://app.netlify.com/sites/nextflow-docs-staging/deploys/65d892c56c87090008fc5387
😎 Deploy Preview https://deploy-preview-4760--nextflow-docs-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@mbosio85
Copy link

Hi! how are the reports saved? I mean the names.
If there are user-defined reports, do the nextflow reports fall in the middle of the alphabetically sorted list or are they at the top/bottom, separated from the rest?

@bentsherman
Copy link
Member Author

They will be lumped in with the user-defined reports. For this reason I made the display name "Nextflow execution report", "Nextflow timeline report", etc so that the Nextflow reports are at least grouped together

As a second iteration, we could have the platform reports tab display "runtime" reports separately from "user" reports, and Nextflow could denote this type when it sends the report information to the platform

@mbosio85
Copy link

thanks

Copy link
Collaborator

@jordeu jordeu left a comment

Choose a reason for hiding this comment

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

The implementation seems correct.

My only concern is that it might be better to make this configurable somewhere. With this approach, if the report/timeline/trace/dag are enabled, they will always be added as Tower reports, and there is no way to keep generating them without adding them as Tower reports.

This should be optional. Many users will still want to generate the report/timeline/trace/dag files without showing them on the Tower reports tab.

@bentsherman
Copy link
Member Author

I was thinking we could enable it only if the report path is included in the tower.yml. They would have to specify the report path twice but I guess that's already true for everything else in tower.yml

Signed-off-by: Ben Sherman <bentshermann@gmail.com>
Comment on lines +231 to +248
void publishRuntimeReports() {
final config = session.config
final files = []

if( config.navigate('report.enabled') )
files << config.navigate('report.file', ReportObserver.DEF_FILE_NAME)

if( config.navigate('timeline.enabled') )
files << config.navigate('timeline.file', TimelineObserver.DEF_FILE_NAME)

if( config.navigate('trace.enabled') )
files << config.navigate('trace.file', TraceFileObserver.DEF_FILE_NAME)

if( config.navigate('dag.enabled') )
files << config.navigate('dag.file', GraphObserver.DEF_FILE_NAME)

for( def file : files )
filePublish( (file as Path).complete() )
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Why is the logs handler saving the timeline report but not the others?

Copy link
Member

Choose a reason for hiding this comment

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

Because that was the requirement

Copy link
Member Author

Choose a reason for hiding this comment

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

What requirement? 😆

In any case, the logs handler is saving the timeline file whereas this PR is adding the built-in reports to the Tower reports list. So there is no overlap

Copy link
Member

Choose a reason for hiding this comment

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

Nearly the timeline is on both

https://github.com/nextflow-io/nextflow/blob/3460-publish-nextflow-reports/plugins/nf-tower/src/main/io/seqera/tower/plugin/LogsHandler.groovy#L65-L70

Therefore if the user provides a custom path it breaks the download in the platform frontend.

But I agree is not a problem of this PR. It should be solved in the platform

@pditommaso pditommaso merged commit b710d92 into master Mar 12, 2024
21 checks passed
@pditommaso pditommaso deleted the 3460-publish-nextflow-reports branch March 12, 2024 15:34
@ewels
Copy link
Member

ewels commented Mar 12, 2024

Should we document this somewhere?

@bentsherman
Copy link
Member Author

Probably in the platform docs: https://docs.seqera.io/platform/23.4.0/reports/overview

nschan pushed a commit to nschan/nextflow that referenced this pull request Apr 3, 2024
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
Signed-off-by: Niklas Schandry <niklas@bio.lmu.de>
nschan pushed a commit to nschan/nextflow that referenced this pull request Apr 3, 2024
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
Signed-off-by: Niklas Schandry <niklas@bio.lmu.de>
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.

Allow to publish timeline, trace and DAG files as a Tower report
5 participants