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

dag: Add --mermaid and --md option. #7383

Merged
merged 1 commit into from
Apr 21, 2022
Merged

dag: Add --mermaid and --md option. #7383

merged 1 commit into from
Apr 21, 2022

Conversation

daavoo
Copy link
Contributor

@daavoo daavoo commented Feb 15, 2022


Now that mermaid can be embedded in github md:

dvc dag --md >> dag.md
cml send-comment dag.md

@daavoo daavoo force-pushed the dag-md branch 2 times, most recently from 0d8ff38 to f8b4d4c Compare February 15, 2022 10:45
@daavoo daavoo requested a review from skshetry February 15, 2022 10:59
@daavoo daavoo marked this pull request as ready for review February 15, 2022 10:59
@daavoo daavoo requested a review from a team as a code owner February 15, 2022 10:59
skshetry
skshetry previously approved these changes Feb 15, 2022
Copy link
Member

@skshetry skshetry left a comment

Choose a reason for hiding this comment

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

πŸ”₯

@skshetry skshetry added A: pipelines Related to the pipelines feature feature is a feature labels Feb 15, 2022
@skshetry
Copy link
Member

skshetry commented Feb 15, 2022

We could extend this with subgraphs to show individual pipelines in the future.

flowchart TD
	node1[data/data.xml.dvc]
	node2[evaluate]
	node3[featurize]
	node4[prepare]
	node5[train]
	node6[data2.dvc]

	subgraph P1
    node6
    end

    subgraph P2
	node1-->node4
	node3-->node2
	node3-->node5
	node4-->node3
	node5-->node2
    end
flowchart TD
	node1[data/data.xml.dvc]
	node2[evaluate]
	node3[featurize]
	node4[prepare]
	node5[train]
	node6[data2.dvc]

	subgraph P1
    node6
    end

    subgraph P2
	node1-->node4
	node3-->node2
	node3-->node5
	node4-->node3
	node5-->node2
    end
Loading

@daavoo
Copy link
Contributor Author

daavoo commented Feb 15, 2022

We could extend this with subgraphs to show individual pipelines in the future.

flowchart TD
	node1[data/data.xml.dvc]
	node2[evaluate]
	node3[featurize]
	node4[prepare]
	node5[train]
	node6[data2.dvc]

	subgraph P1
    node6
    end

    subgraph P2
	node1-->node4
	node3-->node2
	node3-->node5
	node4-->node3
	node5-->node2
    end

That was a doubt I had because I couldn't find an example test.

When it's possible for get_pipelines to return more than 1 pipeline @skshetry ?

@daavoo daavoo changed the title dag: Add --mermaid option. dag: Add --mermaid and --md option. Feb 15, 2022
@daavoo daavoo requested a review from skshetry February 15, 2022 19:06
@skshetry
Copy link
Member

This is now inconsistent with other --md flags, in that they never add ``` (backticks).

@daavoo
Copy link
Contributor Author

daavoo commented Feb 16, 2022

This is now inconsistent with other --md flags, in that they never add ``` (backticks).

I see your point. However, the way I understand it is that --md outputs a self-contained MarkDown "component".

On the one hand, the other --md flags output a MarkDown table, because it's the component that best suits the data to be shown.

On the other, this command would generate a MarkDown codeblock, which is also a self-contained MarkDown component, because it's suitable for the data to be shown.

I see inconsistency in the fact that this is the first --md option not outputting a table, but I see consistency on all outputting self-contained MarkDown components.

@casperdcl
Copy link
Contributor

casperdcl commented Mar 7, 2022

Definitely prefer "self-contained markdown component with backticks"

  • it's --md not (just) --mermaid. In fact --md may even imply --mermaid.
  • it works well with CML reports

@0x2b3bfa0

This comment was marked as outdated.

@0x2b3bfa0

This comment was marked as outdated.

@casperdcl
Copy link
Contributor

@daavoo as @0x2b3bfa0 said, maybe --mermaid (without --md) should not have fencing.

@0x2b3bfa0
Copy link
Member

```
🀺
```

@dberenbaum
Copy link
Collaborator

What's the status of this PR?

@daavoo
Copy link
Contributor Author

daavoo commented Apr 20, 2022

@daavoo as @0x2b3bfa0 said, maybe --mermaid (without --md) should not have fencing.

This is already the current status

@daavoo daavoo enabled auto-merge (rebase) April 20, 2022 08:12
@daavoo daavoo disabled auto-merge April 20, 2022 08:52
@daavoo daavoo enabled auto-merge (rebase) April 20, 2022 08:52
@daavoo daavoo requested review from skshetry and removed request for skshetry April 20, 2022 09:27
dvc/commands/dag.py Outdated Show resolved Hide resolved
dvc/commands/dag.py Outdated Show resolved Hide resolved
@skshetry skshetry disabled auto-merge April 21, 2022 08:18
@skshetry skshetry merged commit f20d6a7 into main Apr 21, 2022
@skshetry skshetry deleted the dag-md branch April 21, 2022 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: pipelines Related to the pipelines feature feature is a feature
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants