Remove the graphviz dependency from nf-core #1512

fabianegli · 2022-04-12T13:15:41Z

The current default DAG image format SVG requires Graphviz. Using the HTML version removes the Graphviz dependency.

It is now common to see the Warning about the missing Graphviz dependency when running nf-core pipelines. In my limited experience it is often the only warning and often not a very useful at that because the DAG is not a main product of the pipeline in most use cases.

To have the DAG image output as HTML should be fine, since most computers who can display a PNG will also have a browser that can render a HTML file.

Because the Graphviz dependency is on the host system rather than in a container, this means the hassle of installation is left with the user. A big part of Nextflow/nf-core is that it abstracts installations away from users. I consider the Graphviz dependency a clear anti-pattern for Nextflow and nf-core.

Because this makes life easier for users, no additional docs are needed.

This PR arouse out of me seeing

WARN: To render the execution DAG in the required format it is required to install Graphviz -- See http://www.graphviz.org/ for more info.

in a nextflow log on a LSF cluster and the discussion on Slack that followed.

An alternative solution might be to use a Graphviz DSL2 module to produce the PNG and remove the dependency on the host system in the most Nextflow way possible.

PR checklist

This comment contains a description of changes (with reason)
CHANGELOG.md is updated
If you've fixed a bug or added code that should be tested, add tests!
Documentation in docs is updated

ewels

Please add a mention of the change to the changelog.

ewels · 2022-04-12T14:26:32Z

Hah, ok apparently nf-core lint checks for this:

nextflow_config    dag.file did not end with .svg

So the linting needs to be updated too.. (Yay for CI tests that check this stuff!)

Code is here:

tools/nf_core/lint/nextflow_config.py

Lines 230 to 235 in f705faa

    
           # Check that the DAG filename ends in ``.svg`` 
        
           if "dag.file" in self.nf_config: 
        
               if self.nf_config["dag.file"].strip("'\"").endswith(".svg"): 
        
                   passed.append("Config ``dag.file`` ended with ``.svg``") 
        
               else: 
        
                   failed.append("Config ``dag.file`` did not end with ``.svg``")

fabianegli · 2022-04-12T18:12:57Z

I think the linting test should accept all documented file extensions. Any objections?

fabianegli · 2022-04-12T18:34:05Z

I am not entirely happy with the wording in my changes of the changelog, but am too tired to figure that out now. I also wonder if maybe the removal of the only dependency of the default pipeline template besides Java itself doesn't warrant a minor release instead of a patch release, i.e. 2.4.0 instead of 2.3.2. This however is up for discussion and https://semver.org/#what-should-i-do-if-i-update-my-own-dependencies-without-changing-the-public-api is not decisive on this issue.

fabianegli · 2022-04-12T19:46:48Z

So many bugs in so little code. Time to call it a day.

codecov · 2022-04-12T19:48:02Z

Codecov Report

Merging #1512 (e298fe9) into dev (821b34d) will increase coverage by 0.01%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##              dev    #1512      +/-   ##
==========================================
+ Coverage   64.91%   64.92%   +0.01%     
==========================================
  Files          52       52              
  Lines        6060     6062       +2     
==========================================
+ Hits         3934     3936       +2     
  Misses       2126     2126

Impacted Files	Coverage Δ
nf_core/lint/nextflow_config.py	`77.08% <80.00%> (+0.48%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 821b34d...e298fe9. Read the comment docs.

CHANGELOG.md

nf_core/lint/nextflow_config.py

maxulysse · 2022-04-12T20:12:42Z

I am not entirely happy with the wording in my changes of the changelog, but am too tired to figure that out now. I also wonder if maybe the removal of the only dependency of the default pipeline template besides Java itself doesn't warrant a minor release instead of a patch release, i.e. 2.4.0 instead of 2.3.2. This however is up for discussion and https://semver.org/#what-should-i-do-if-i-update-my-own-dependencies-without-changing-the-public-api is not decisive on this issue.

I think we'll decide the the type of release when we'll do it, so no need to worry about that yourself (for now)

maxulysse · 2022-04-12T20:51:00Z

CHANGELOG.md

+This patch release to removes the Graphviz dependency from default pipeline template.
+


Suggested change

This patch release to removes the Graphviz dependency from default pipeline template.

I think info in the ## Template are enough

In the current form it is quite redundant, but this change might need some visibility. Not just because it removes the dependency, but because it should probably mention in the more prominent sentence that the default DAG graph format has changed and that user configs need to change to keep getting the SVG output.

ok for me then

I added a sentence clarifying the change.

Please double check if it makes sense. I am not yet familiar enough with nextflow to judge what I am writing about configs ...

After that, if you think it's ready to merge, I am all for it.

fabianegli · 2022-04-12T20:53:00Z

Should this PR be hashtagged in the CHANGELOG.md?

maxulysse · 2022-04-12T20:59:00Z

Should this PR be hashtagged in the CHANGELOG.md?

If you want to, yes, go for it, otherwise, we can merge

* add link to PR

maxulysse · 2022-04-12T21:20:47Z

Congrats on the PR @fabianegli

fabianegli · 2022-04-12T21:24:08Z

Thank you. That was very quick. I am very happy right now. 🥳

Thank you for the feedback, help and review @ewels and @maxulysse!

ewels

Sorry, only just got to this and it's already been merged. I think we should revert the change in behaviour about accepting additional file formats though sorry 😞

ewels · 2022-04-12T22:03:51Z

CHANGELOG.md

@@ -2,8 +2,11 @@

 ## v2.4dev

+This patch release to removes the Graphviz dependency from default pipeline template by setting the defaul DAG format to HTML. To keep the previous behaviour, the "dag.file" file extension needs to be changed to ".svg" in the nextflow.config.


It's not a patch release for just this PR 😉 This is just one change in the release. The bullet point below was sufficient, I think you can remove this paragraph.

ewels · 2022-04-12T22:04:01Z

CHANGELOG.md

 ### Template

+- Set the default DAG graphic output to HTML to have a default that does not depend on Graphviz being installed on the host system ([#1512](https://github.com/nf-core/tools/pull/1512)).


ewels · 2022-04-12T22:06:29Z

nf_core/lint/nextflow_config.py

-        if self.nf_config["dag.file"].strip("'\"").endswith(".svg"):
-            passed.append("Config ``dag.file`` ended with ``.svg``")
+        _, dag_file_suffix = os.path.splitext(self.nf_config["dag.file"].strip("'\""))
+        allowed_dag_file_suffixes = [".dot", ".html", ".pdf", ".png", ".svg", ".gexf"]


Too late to the party, but I think we need to revert this sorry.

Whilst it's perfectly valid to accept all, we specifically want to enforce a single standard for nf-core pipelines. People can override in their own configs and even ignore this linting test if it's a problem.

If we allow all formats, then there's nothing to stop people from (probably accidentally) missing this change and continuing with .svg and the problems that this PR is trying to solve. We explicitly want to force people to change to the format that we think is best for nf-core pipelines.

I am fine with reverting this PR, allowing html only and removing the first sentence in the changelog. I think it is/was mostly necessary because of the multiple allowed formats.

fabianegli · 2022-04-13T05:32:46Z

The feedback is already implemented on the previous branch.

I am not familiar with the GitHub web interface for the revert. May I try?

Remove the graphviz dependency from nf-core

3567b07

ewels approved these changes Apr 12, 2022

View reviewed changes

mention the Graphviz dependency removal

8313a50

fabianegli added 3 commits April 12, 2022 21:33

allow all supported DAG file suffixes

ae2a057

fix black complaint

c68fb85

fix dag.file suffix extraction

83fe29e

fabianegli requested a review from ewels April 12, 2022 19:50

maxulysse reviewed Apr 12, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

maxulysse reviewed Apr 12, 2022

View reviewed changes

nf_core/lint/nextflow_config.py Show resolved Hide resolved

fabianegli added 2 commits April 12, 2022 22:46

move changes in CHANGELOG into new dev section

32da8e5

Merge branch 'dev' into fabianegli-remove-graphviz-dependency

84e5db9

maxulysse reviewed Apr 12, 2022

View reviewed changes

fabianegli added 2 commits April 12, 2022 23:09

describe new behavior and hot to retain status quo

3d91fe0

* add link to PR

fix typo

e298fe9

maxulysse approved these changes Apr 12, 2022

View reviewed changes

maxulysse merged commit 2a1d1be into nf-core:dev Apr 12, 2022

fabianegli deleted the fabianegli-remove-graphviz-dependency branch April 12, 2022 21:28

ewels reviewed Apr 12, 2022

View reviewed changes

fabianegli restored the fabianegli-remove-graphviz-dependency branch April 13, 2022 04:49

This was referenced Apr 14, 2022

revert to single dag graph format allowance when linting #1516

Closed

revert to single dag.file file format for linting #1517

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove the graphviz dependency from nf-core #1512

Remove the graphviz dependency from nf-core #1512

fabianegli commented Apr 12, 2022 •

edited

Loading

ewels left a comment

ewels commented Apr 12, 2022

fabianegli commented Apr 12, 2022

fabianegli commented Apr 12, 2022 •

edited

Loading

fabianegli commented Apr 12, 2022

codecov bot commented Apr 12, 2022 •

edited

Loading

maxulysse commented Apr 12, 2022

maxulysse Apr 12, 2022

fabianegli Apr 12, 2022

maxulysse Apr 12, 2022

fabianegli Apr 12, 2022

fabianegli Apr 12, 2022

fabianegli Apr 12, 2022

fabianegli commented Apr 12, 2022

maxulysse commented Apr 12, 2022

maxulysse commented Apr 12, 2022

fabianegli commented Apr 12, 2022

ewels left a comment

ewels Apr 12, 2022

ewels Apr 12, 2022

ewels Apr 12, 2022

fabianegli Apr 13, 2022

fabianegli commented Apr 13, 2022

		This patch release to removes the Graphviz dependency from default pipeline template.

		@@ -2,8 +2,11 @@

		## v2.4dev

		This patch release to removes the Graphviz dependency from default pipeline template by setting the defaul DAG format to HTML. To keep the previous behaviour, the "dag.file" file extension needs to be changed to ".svg" in the nextflow.config.

		### Template

		- Set the default DAG graphic output to HTML to have a default that does not depend on Graphviz being installed on the host system ([#1512](https://github.com/nf-core/tools/pull/1512)).

Remove the graphviz dependency from nf-core #1512

Remove the graphviz dependency from nf-core #1512

Conversation

fabianegli commented Apr 12, 2022 • edited Loading

PR checklist

ewels left a comment

Choose a reason for hiding this comment

ewels commented Apr 12, 2022

fabianegli commented Apr 12, 2022

fabianegli commented Apr 12, 2022 • edited Loading

fabianegli commented Apr 12, 2022

codecov bot commented Apr 12, 2022 • edited Loading

Codecov Report

maxulysse commented Apr 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabianegli commented Apr 12, 2022

maxulysse commented Apr 12, 2022

maxulysse commented Apr 12, 2022

fabianegli commented Apr 12, 2022

ewels left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabianegli commented Apr 13, 2022

fabianegli commented Apr 12, 2022 •

edited

Loading

fabianegli commented Apr 12, 2022 •

edited

Loading

codecov bot commented Apr 12, 2022 •

edited

Loading