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.

Sign up for GitHub

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

Remove the graphviz dependency from nf-core #1512

Merged
merged 9 commits into from
Apr 12, 2022
Merged

Remove the graphviz dependency from nf-core #1512

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3567b07
Remove the graphviz dependency from nf-core
fabianegli Apr 12, 2022
8313a50
mention the Graphviz dependency removal
fabianegli Apr 12, 2022
ae2a057
allow all supported DAG file suffixes
fabianegli Apr 12, 2022
c68fb85
fix black complaint
fabianegli Apr 12, 2022
83fe29e
fix dag.file suffix extraction
fabianegli Apr 12, 2022
32da8e5
move changes in CHANGELOG into new dev section
fabianegli Apr 12, 2022
84e5db9
Merge branch 'dev' into fabianegli-remove-graphviz-dependency
fabianegli Apr 12, 2022
3d91fe0
describe new behavior and hot to retain status quo
fabianegli Apr 12, 2022
e298fe9
fix typo
fabianegli Apr 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,11 @@

## v2.4dev

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

maxulysse marked this conversation as resolved.
Show resolved Hide resolved
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
This patch release to removes the Graphviz dependency from default pipeline template.

I think info in the ## Template are enough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

ok for me then

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a sentence clarifying the change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

### 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.
- Fix bug in pipeline readme logo URL
- Fix Prettier formatting bug in completion email HTML template ([#1509](https://github.com/nf-core/tools/issues/1509))
- Removed retry strategy for AWS tests CI, as Nextflow now handles spot instance retries itself
Expand Down
10 changes: 7 additions & 3 deletions nf_core/lint/nextflow_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,10 +229,14 @@ def nextflow_config(self):

# 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``")
_, dag_file_suffix = os.path.splitext(self.nf_config["dag.file"].strip("'\""))
maxulysse marked this conversation as resolved.
Show resolved Hide resolved
allowed_dag_file_suffixes = [".dot", ".html", ".pdf", ".png", ".svg", ".gexf"]
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

if dag_file_suffix in allowed_dag_file_suffixes:
passed.append(f"Config ``dag.file`` ended with ``.{dag_file_suffix}``")
else:
failed.append("Config ``dag.file`` did not end with ``.svg``")
failed.append(
f"Config ``dag.file`` ended with ``.{dag_file_suffix}`` but needs to be one of {allowed_dag_file_suffixes!r}"
)

# Check that the minimum nextflowVersion is set properly
if "manifest.nextflowVersion" in self.nf_config:
Expand Down
2 changes: 1 addition & 1 deletion nf_core/pipeline-template/nextflow.config
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ trace {
}
dag {
enabled = true
file = "${params.tracedir}/pipeline_dag_${trace_timestamp}.svg"
file = "${params.tracedir}/pipeline_dag_${trace_timestamp}.html"
}

manifest {
Expand Down