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

docs: replace dvc pipeline with dvc dag #1383

Merged
merged 8 commits into from
Jun 24, 2020
Merged

docs: replace dvc pipeline with dvc dag #1383

merged 8 commits into from
Jun 24, 2020

Conversation

efiop
Copy link
Contributor

@efiop efiop commented May 31, 2020

You may disregard these recommendations if you used the Edit on GitHub button from dvc.org to improve a doc in place.

❗ Please read the guidelines in the Contributing to the Documentation list if you make any substantial changes to the documentation or JS engine.

🐛 Please make sure to mention Fix #issue (if applicable) in the description of the PR. This causes GitHub to close it automatically when the PR is merged.

Please choose to allow us to edit your branch when creating the PR.

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

@shcheklein shcheklein temporarily deployed to dvc-landing-pipeline-da-no2vu4 May 31, 2020 23:18 Inactive
@shcheklein
Copy link
Member

will be merging after 1.0 release probably

@calibre-analytics
Copy link

You are out of tests

Choose a plan to resume monitoring your Sites and Pull Requests. If you need help, check the Manage Your Plan and Test Usage guide.

@shcheklein
Copy link
Member

@jorgeorpinel one more thing to the discrepancies list - we probably use dvc pipeline in a few places ... need to check content and replace

@efiop
Copy link
Contributor Author

efiop commented May 31, 2020

@shcheklein We do, in a lot of places. I've decided not to touch those because the mapping is not direct and there are a lot of discrepancies with 1.0. For now it will at least redirect to the new command, but we definitely have a lot of stuff to revisit :(

@jorgeorpinel jorgeorpinel mentioned this pull request Jun 1, 2020
20 tasks
Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

A few minor issues. I can probably take this over when released (just tricky to keep track of open PRs sometimes). Also missing:

p.s. I did add a checkbox about that in #1255 @shcheklein, for the record.

content/docs/command-reference/dag.md Outdated Show resolved Hide resolved
Comment on lines +19 to +20
`dvc dag` displays the stages of a pipeline up to the target stage. If `target`
is omitted, it will show the full project DAG.
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Is it possible to have more than one DAG?
  • Should we call it always "pipeline" or always "DAG" or always "dependency graph"? We currently have a mix of all these everywhere 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, 2 DAGs together is 1 DAG too, just with weakly connected components :)

Pipeline is a DAG in which all stages are somehow connected with each other, so it is not quite that. We could call it pipelineS, but dvc add foo is not quite a pipeline strictly speaking. dependency graph sounds as the most correct term, but DAG is a synonym, hence the https://dagshub.com/ :) So we could and probably should use them interchangeably.

This doc is adapted from the old dvc pipeline show so it does suffer from some legacy sentence structure :(

content/docs/command-reference/dag.md Outdated Show resolved Hide resolved
content/docs/command-reference/dag.md Outdated Show resolved Hide resolved
content/docs/command-reference/dag.md Outdated Show resolved Hide resolved
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-pipeline-da-no2vu4 June 1, 2020 18:11 Inactive
@shcheklein shcheklein temporarily deployed to dvc-landing-pipeline-da-no2vu4 June 3, 2020 01:24 Inactive
efiop added a commit to iterative/dvc that referenced this pull request Jun 3, 2020
@shcheklein shcheklein temporarily deployed to dvc-landing-pipeline-da-no2vu4 June 3, 2020 01:28 Inactive
@efiop efiop self-assigned this Jun 8, 2020
@jorgeorpinel jorgeorpinel assigned jorgeorpinel and unassigned efiop Jun 20, 2020
@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Jun 20, 2020

OK solved the conflicts in this branch and grep replaced pipeline->dag where needed. Turning this back to you @efiop, please git pull 🙂

@jorgeorpinel jorgeorpinel assigned efiop and unassigned jorgeorpinel Jun 20, 2020
@jorgeorpinel jorgeorpinel mentioned this pull request Jun 20, 2020
17 tasks
@shcheklein
Copy link
Member

@jorgeorpinel @efiop what is the status of this?

@efiop

This comment has been minimized.

@efiop
Copy link
Contributor Author

efiop commented Jun 23, 2020

The conflicts are crazy 🙁

@efiop
Copy link
Contributor Author

efiop commented Jun 23, 2020

Will try to get back to it soon. People are asking for it on discord https://discordapp.com/channels/485586884165107732/563406153334128681/725054982629621832

efiop added a commit that referenced this pull request Jun 23, 2020
@efiop
Copy link
Contributor Author

efiop commented Jun 23, 2020

@jorgeorpinel @shcheklein Resolved the conflicts, addressed review comments. Please take a look when you'll have time. 🙏

@shcheklein
Copy link
Member

Looks good to me, but I see some other mentions of the dvc pipeline across docs. Please, do git grep "dvc pipeline" and fix that are obvious for you, and we should create a checkbox in the DVC 1.0 for everything that is left.

@efiop
Copy link
Contributor Author

efiop commented Jun 23, 2020

@shcheklein Did that when initially submitted the PR: #1383 (comment) . But a lot has change since then, taking another look...

@efiop
Copy link
Contributor Author

efiop commented Jun 23, 2020

@shcheklein Indeed, now we have a lot less to update. But still some examples are out-of-date (e.g. push/pull). So I've left them be, but updated others. Edited the checkbox in that epic. We have pipeline -> dag redirects, so we should be fine for now.

@shcheklein
Copy link
Member

thanks @efiop !

@shcheklein shcheklein merged commit ccaecda into master Jun 24, 2020
@efiop efiop deleted the pipeline_dag branch June 24, 2020 01:05
Comment on lines -73 to +74
as pager for the output of `dvc pipeline show`. Windows doesn't have the `less`
command available however. Fortunately, there is a easy way of installing it via
as pager for the output of `dvc dag`. Windows doesn't have the less command
available however. Fortunately, there is a easy way of installing `less` via
Copy link
Contributor

Choose a reason for hiding this comment

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

`less` first, "it" second, please.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see this PR is merged. OK I'll do this somewhere else.

Comment on lines 34 to +38
"^/doc/command-reference/lock$ /doc/command-reference/freeze",
"^/doc/command-reference/unlock$ /doc/command-reference/unfreeze",
"^/doc/command-reference/pipeline$ /doc/command-reference/dag",
"^/doc/command-reference/pipeline/show$ /doc/command-reference/dag",
"^/doc/command-reference/pipeline/list$ /doc/command-reference/dag",
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: I'd probably put these above lock/unlock

Comment on lines +35 to +36
`dvc dag` displays the stages of a pipeline up to the target stage. If `target`
is omitted, it will show the full project DAG.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we shuold start the description with this paragraph though. Since it's now a specific command after all 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

Never mind, the short intro is enough. That one needs updating though... Will do.

@jorgeorpinel
Copy link
Contributor

Continued in #1496

shcheklein pushed a commit that referenced this pull request Jun 26, 2020
* cmd ref: more updates on new dag command
per #1383 (review)
et al.

* Update content/docs/command-reference/dag.md

Co-authored-by: Ruslan Kuprieiev <[email protected]>

* cmd: a few more impros to dag
per #1496 (review)
and #1496 (review)

* cmd: graph -> DAG{linked} in dag intro
per #1496 (review)

* cmd: DAGs -> graph(s) in dag intro
per #1496 (comment)

Co-authored-by: Ruslan Kuprieiev <[email protected]>
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.

3 participants