-
Notifications
You must be signed in to change notification settings - Fork 252
Add manifest.diagram to the pipeline template, with linting
#4460
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
base: dev
Are you sure you want to change the base?
Changes from all commits
ed40821
7ebec57
ef0fe43
1430666
1eec0f8
b6e2d5b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -313,6 +313,8 @@ manifest { | |
| nextflowVersion = '!>=25.10.4' | ||
| version = '{{ version }}' | ||
| doi = '' | ||
| // TODO nf-core: Make a metro map (nf-metro or drawn) and add relative path to the SVG below | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should be a link to our docs https://nf-co.re/docs/community/brand/workflow-schematics which need to be updated with nf-metro) |
||
| // diagram = 'docs/images/metro_map.svg' | ||
| } | ||
|
|
||
| {% if nf_schema -%} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,9 @@ | |
|
|
||
| log = logging.getLogger(__name__) | ||
|
|
||
| # Image formats that Nextflow accepts for `manifest.diagram` | ||
| DIAGRAM_EXTENSIONS = {".svg", ".png", ".jpg", ".jpeg", ".gif", ".webp"} | ||
|
|
||
|
|
||
| def nextflow_config(self) -> dict[str, list[str]]: | ||
| """Checks the pipeline configuration for required variables. | ||
|
|
@@ -68,11 +71,27 @@ def nextflow_config(self) -> dict[str, list[str]]: | |
| **The following variables throw warnings if missing:** | ||
|
|
||
| * ``manifest.mainScript``: The filename of the main pipeline script (should be ``main.nf``) | ||
| * ``manifest.diagram`` | ||
|
|
||
| * A relative path to a workflow diagram (metro map) for the pipeline, eg. ``docs/images/metro_map.svg`` | ||
| * Any SVG works, including hand-drawn ones. `nf-metro <https://seqeralabs.github.io/nf-metro/latest/>`_ | ||
| can generate one from a config file, if you'd like a hand. | ||
|
pinin4fjords marked this conversation as resolved.
|
||
| * Requires Nextflow ``26.10.0`` or later, but is safe to set for older versions - they ignore it. | ||
| * If set, the value must be valid or the test **fails** (see below) | ||
|
|
||
| * ``timeline.file``, ``trace.file``, ``report.file``, ``dag.file`` | ||
|
|
||
| * Default filenames for the timeline, trace and report | ||
| * The DAG file path should end with ``.svg`` (If Graphviz is not installed, Nextflow will generate a ``.dot`` file instead) | ||
|
|
||
| **The following variables fail the test if they are set to an invalid value:** | ||
|
|
||
| * ``manifest.diagram`` | ||
|
|
||
| * Must be a relative path, not a URL | ||
| * Must be one of the image formats that Nextflow accepts: ``.svg``, ``.png``, ``.jpg``, ``.jpeg``, ``.gif``, ``.webp`` | ||
| * Must point at a file that exists in the pipeline | ||
|
|
||
| **The following variables are depreciated and fail the test if they are still present:** | ||
|
|
||
| * ``params.version``: The old method for specifying the pipeline version. Replaced by ``manifest.version`` | ||
|
|
@@ -150,6 +169,7 @@ def nextflow_config(self) -> dict[str, list[str]]: | |
| # Throw a warning if these are missing | ||
| config_warn = [ | ||
| ["manifest.mainScript"], | ||
| ["manifest.diagram"], | ||
| ["timeline.file"], | ||
| ["trace.file"], | ||
| ["report.file"], | ||
|
|
@@ -272,6 +292,21 @@ def _config_has_key(key: str) -> bool: | |
| else: | ||
| failed.append(f"Config ``dag.file`` did not end with ``{default_dag_format}``") | ||
|
|
||
| # Check that manifest.diagram points at a file that exists in the pipeline | ||
| diagram = manifest.get("diagram", "") | ||
| if diagram and "manifest.diagram" not in ignore_configs: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. are the |
||
| if re.match(r"^\w+://", diagram): | ||
| failed.append(f"Config ``manifest.diagram`` should be a relative path, not a URL: ``{diagram}``") | ||
| elif Path(diagram).suffix.lower() not in DIAGRAM_EXTENSIONS: | ||
| failed.append( | ||
| f"Config ``manifest.diagram`` is not a supported image format " | ||
| f"({', '.join(sorted(DIAGRAM_EXTENSIONS))}): ``{diagram}``" | ||
| ) | ||
| elif (Path(self.wf_path) / diagram).is_file(): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. might be very minor: if the diagram is an absolute path it could override |
||
| passed.append(f"Config ``manifest.diagram`` file found: ``{diagram}``") | ||
| else: | ||
| failed.append(f"Config ``manifest.diagram`` file not found: ``{diagram}``") | ||
|
|
||
| # Check that the minimum nextflowVersion is set properly | ||
| nextflow_version = manifest.get("nextflowVersion", "") | ||
| if nextflow_version: | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -7,6 +7,9 @@ | |||||
|
|
||||||
| from ..test_lint import TestLint | ||||||
|
|
||||||
| # The template ships `manifest.diagram` commented out, so a new pipeline always warns about it | ||||||
| MISSING_DIAGRAM_WARNING = "Config variable not found: `manifest.diagram`" | ||||||
|
|
||||||
|
|
||||||
| class TestLintNextflowConfig(TestLint): | ||||||
| def setUp(self) -> None: | ||||||
|
|
@@ -18,15 +21,15 @@ def test_nextflow_config_example_pass(self): | |||||
| self.lint_obj.load_pipeline_config() | ||||||
| result = self.lint_obj.nextflow_config() | ||||||
| assert len(result["failed"]) == 0 | ||||||
| assert len(result["warned"]) == 0 | ||||||
| assert result["warned"] == [MISSING_DIAGRAM_WARNING] | ||||||
|
|
||||||
| def test_default_values_match(self): | ||||||
| """Test that the default values in nextflow.config match the default values defined in the nextflow_schema.json.""" | ||||||
| lint_obj = nf_core.pipelines.lint.PipelineLint(self.new_pipeline) | ||||||
| lint_obj.load_pipeline_config() | ||||||
| result = lint_obj.nextflow_config() | ||||||
| assert len(result["failed"]) == 0 | ||||||
| assert len(result["warned"]) == 0 | ||||||
| assert result["warned"] == [MISSING_DIAGRAM_WARNING] | ||||||
| assert "Config default value correct: params.validate_params" in str(result["passed"]) | ||||||
|
|
||||||
| def test_nextflow_config_bad_name_fail(self): | ||||||
|
|
@@ -37,7 +40,7 @@ def test_nextflow_config_bad_name_fail(self): | |||||
| lint_obj.nf_config["manifest"]["name"] = "bad_name" | ||||||
| result = lint_obj.nextflow_config() | ||||||
| assert len(result["failed"]) > 0 | ||||||
| assert len(result["warned"]) == 0 | ||||||
| assert result["warned"] == [MISSING_DIAGRAM_WARNING] | ||||||
|
|
||||||
| def test_nextflow_config_dev_in_release_mode_failed(self): | ||||||
| """Tests that config variable existence test fails with dev version in release mode""" | ||||||
|
|
@@ -48,7 +51,7 @@ def test_nextflow_config_dev_in_release_mode_failed(self): | |||||
| lint_obj.nf_config["manifest"]["version"] = "dev_is_bad_name" | ||||||
| result = lint_obj.nextflow_config() | ||||||
| assert len(result["failed"]) > 0 | ||||||
| assert len(result["warned"]) == 0 | ||||||
| assert result["warned"] == [MISSING_DIAGRAM_WARNING] | ||||||
|
|
||||||
| def test_nextflow_config_missing_test_profile_failed(self): | ||||||
| """Test failure if config file does not contain `test` profile.""" | ||||||
|
|
@@ -64,7 +67,7 @@ def test_nextflow_config_missing_test_profile_failed(self): | |||||
| lint_obj.load_pipeline_config() | ||||||
| result = lint_obj.nextflow_config() | ||||||
| assert len(result["failed"]) > 0 | ||||||
| assert len(result["warned"]) == 0 | ||||||
| assert result["warned"] == [MISSING_DIAGRAM_WARNING] | ||||||
|
|
||||||
| def test_default_values_fail(self): | ||||||
| """Test linting fails if the default values in nextflow.config do not match the ones defined in the nextflow_schema.json.""" | ||||||
|
|
@@ -181,7 +184,7 @@ def test_default_values_float(self): | |||||
| lint_obj.load_pipeline_config() | ||||||
| result = lint_obj.nextflow_config() | ||||||
| assert len(result["failed"]) == 0 | ||||||
| assert len(result["warned"]) == 0 | ||||||
| assert result["warned"] == [MISSING_DIAGRAM_WARNING] | ||||||
| assert "Config default value correct: params.dummy" in str(result["passed"]) | ||||||
|
|
||||||
| def test_default_values_float_fail(self): | ||||||
|
|
@@ -214,5 +217,57 @@ def test_default_values_float_fail(self): | |||||
| result = lint_obj.nextflow_config() | ||||||
|
|
||||||
| assert len(result["failed"]) == 1 | ||||||
| assert len(result["warned"]) == 0 | ||||||
| assert result["warned"] == [MISSING_DIAGRAM_WARNING] | ||||||
| assert "Config default value incorrect: `params.dummy" in str(result["failed"]) | ||||||
|
|
||||||
| def test_manifest_diagram_pass(self): | ||||||
| """Test that a `manifest.diagram` pointing at an existing file passes.""" | ||||||
| diagram = Path(self.new_pipeline) / "docs" / "images" / "metro_map.svg" | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
is already path |
||||||
| diagram.parent.mkdir(parents=True, exist_ok=True) | ||||||
| diagram.write_text("<svg></svg>") | ||||||
| self._set_manifest_diagram("docs/images/metro_map.svg") | ||||||
|
|
||||||
| result = self._lint_new_pipeline() | ||||||
| assert len(result["failed"]) == 0 | ||||||
| assert len(result["warned"]) == 0 | ||||||
| assert "Config ``manifest.diagram`` file found: ``docs/images/metro_map.svg``" in result["passed"] | ||||||
|
|
||||||
| def test_manifest_diagram_missing_file_fail(self): | ||||||
| """Test that a `manifest.diagram` pointing at a non-existent file fails.""" | ||||||
| self._set_manifest_diagram("docs/images/metro_map.svg") | ||||||
|
|
||||||
| result = self._lint_new_pipeline() | ||||||
| assert "Config ``manifest.diagram`` file not found: ``docs/images/metro_map.svg``" in result["failed"] | ||||||
|
|
||||||
| def test_manifest_diagram_bad_format_fail(self): | ||||||
| """Test that a `manifest.diagram` that is not a supported image format fails.""" | ||||||
| diagram = Path(self.new_pipeline) / "docs" / "images" / "metro_map.pdf" | ||||||
| diagram.parent.mkdir(parents=True, exist_ok=True) | ||||||
| diagram.write_text("not an image") | ||||||
| self._set_manifest_diagram("docs/images/metro_map.pdf") | ||||||
|
|
||||||
| result = self._lint_new_pipeline() | ||||||
| assert ( | ||||||
| "Config ``manifest.diagram`` is not a supported image format " | ||||||
| "(.gif, .jpeg, .jpg, .png, .svg, .webp): ``docs/images/metro_map.pdf``" in result["failed"] | ||||||
| ) | ||||||
|
|
||||||
| def test_manifest_diagram_url_fail(self): | ||||||
| """Test that a `manifest.diagram` set to a URL fails - it should be a relative path.""" | ||||||
| url = "https://example.com/metro_map.svg" | ||||||
| self._set_manifest_diagram(url) | ||||||
|
|
||||||
| result = self._lint_new_pipeline() | ||||||
| assert f"Config ``manifest.diagram`` should be a relative path, not a URL: ``{url}``" in result["failed"] | ||||||
|
|
||||||
| def _set_manifest_diagram(self, value: str) -> None: | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think we usually have helper functions higher up |
||||||
| """Uncomment the templated `manifest.diagram` line and set it to `value`.""" | ||||||
| nf_conf_file = Path(self.new_pipeline) / "nextflow.config" | ||||||
| content = nf_conf_file.read_text() | ||||||
| assert "// diagram" in content | ||||||
| nf_conf_file.write_text(content.replace("// diagram", f"diagram = '{value}' //")) | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. bit of a weird replace here because it leaves the placeholder string as a comment, but maybe the cleanest option |
||||||
|
|
||||||
| def _lint_new_pipeline(self) -> dict: | ||||||
| lint_obj = nf_core.pipelines.lint.PipelineLint(self.new_pipeline) | ||||||
| lint_obj.load_pipeline_config() | ||||||
| return lint_obj.nextflow_config() | ||||||
Uh oh!
There was an error while loading. Please reload this page.