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

pipeline: links and examples update for 1.x #1584

Merged
merged 19 commits into from
Jul 29, 2020

Conversation

utkarshsingh99
Copy link
Contributor

@utkarshsingh99 utkarshsingh99 commented Jul 17, 2020


Line 169 in repro.md links to /doc/tutorial/dags and previously linked to /doc/tutorial/pipelines. Both these links just redirect to doc/start. Maybe they should have a different link (/doc/start/data-pipelines)?

@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Jul 17, 2020

Continuation of Merged PR #1383

Continuation why? Is there a pending review there? Please provide exact context.

One of the Next Priorities in DVC 1.x discrepancies. #1255

Which one? Please copy the checkbox over...

@utkarshsingh99
Copy link
Contributor Author

Also, regarding the sub-point in #1255 Next Priorities:

Also push/pull examples.

There was a certain bug in one of the examples (private remote) in dvc pull as I had mentioned on Discord.
Message link.
Once that example is resolved, I might be in a better position to work on that.
I hope I can commit those changes in a separate PR?

@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Jul 19, 2020

There was a certain bug in one of the examples (private remote) in dvc pull as I had mentioned on Discord

I can't open that link. Please open a proper bug report in this repo. Never mind just needs updating in general. It's already part of #1255

Why does that stop us from updating the dvc pipeline examples on those pages? Just change "dvc pipeline" for "dvc dag" even if the output isn't 100% correct for now.

@jorgeorpinel
Copy link
Contributor

In fact please find any examples using "dvc pipeline" and update them to "dvc dag" as best as you can @utkarshsingh99. Thanks

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.

So far so good. Just missing updating dvc pipeline from examples all throughout docs.

Please search for "pipeline" everywhere and see if there are any other instances that may need changing to "dag". Thanks

content/docs/command-reference/pull.md Outdated Show resolved Hide resolved
content/docs/command-reference/push.md Outdated Show resolved Hide resolved
$ dvc dag src/evaluate.py.dvc
+---------------------+
| src/evaluate.py.dvc |
+---------------------+
```
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changing the output here because it's a relatively less incorrect output than the previous one.

Unfortunately, the remote still isn't working. I cannot find the output of evaluate.dvc since it doesn't exist. Also, files in src/ are Git-tracked files, so I had to remove them from git. Even after adding all files (dvc add) in src/, dvc dag evaluate.py.dvc does not show the linked files either. I suppose I am missing something and having the remote changed would result in an accurate output. This output is the output I'm currently getting.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that's totally wrong 😋 please read the cmd ref at https://dvc.org/doc/command-reference/dag and notice the https://github.com/iterative/example-get-started project has been updated for 1.x already. There are no .dvc stage files, it's all in dvc.yaml. Have you followed the new Get Started at https://dvc.org/doc/start?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also you don't need to use a target in dvc dag, just the plan command should be enough... But it will print a diagram which may be too long. If it seems to (too long) just use cat dvc.yaml here as well. Thanks

Copy link
Contributor

@jorgeorpinel jorgeorpinel Jul 22, 2020

Choose a reason for hiding this comment

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

Never mind. cat dvc.yaml will bee way too long here also. Let's try dvc dag with the right target please. Lmk if you need help... But do read those refs I shared, hopefully you'll get it from that. Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I've updated it with dvc dag evaluate, but it still gave the entire diagram as output.
Because the deps files link all the way back to prepare and featurize.

Copy link
Contributor

@jorgeorpinel jorgeorpinel Jul 24, 2020

Choose a reason for hiding this comment

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

OK. Yeah I contradicted myself, sorry... You don't need to use a target in dvc dag. The plan command should show everything by default.

But I think we're gonna have to go with a simple list of stage names like in push, because this diagram is too huge.

The rest of the example will also need updating here, so it makes sense (similar to the changes in https://github.com/iterative/dvc.org/pull/1591/files). Please try to make sure the entire context makes sense and works. Thanks

This comment was marked as resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh okay, I got your point. But a problem arises as stated in #1584(comment).

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see the issue. It's just a matter of updating the rest of the example please.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is the other missing point but we can extract into a separate issue. Are you having trouble understanding what I meant here about updating the rest of the example @utkarshsingh99 ? Thanks

@utkarshsingh99

This comment has been minimized.

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.

Getting there.

@jorgeorpinel jorgeorpinel mentioned this pull request Jul 22, 2020
1 task
@jorgeorpinel jorgeorpinel changed the title pipeline links changed pipeline: links and examples update for 1.x Jul 22, 2020
@utkarshsingh99
Copy link
Contributor Author

A problem in pull.md if I simply list stage names in one line:

With the first dvc pull we specified a stage in the middle of this pipeline (featurize.dvc) while using --with-deps

I have listed the stage names just as a one-line list without the code block, instead of bulleted points. This might not exactly define an 'order' to say that featurize is in the middle of the pipeline in pull.md. The same problem arises for push.md.

@utkarshsingh99

This comment has been minimized.

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.

Almost.... 🤞

@@ -195,7 +184,7 @@ One could do a simple `dvc pull` to get all the data, but what if you only want
to retrieve part of the data?

```dvc
$ dvc pull --with-deps featurize.dvc
$ dvc pull --with-deps featurize
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, I missed this change last time.
For now, I don't see any other change. The dvc status command doesn't need change (in my opinion).
I hope I have updated all examples. @jorgeorpinel

@jorgeorpinel
Copy link
Contributor

The dvc status command doesn't need change

Yeah, why status? It doesn't mention dag.

updated all examples

Just in push and pull, as described in this PR's description/scope.

@jorgeorpinel
Copy link
Contributor

Merging even with this strange CI checks error:

image

From https://app.circleci.com/pipelines/github/iterative/dvc.org/4859/workflows/ae8a7d2c-2aa4-4d53-bafc-3c9246ebd621/jobs/4913

https://www.amazon.com/DevOps-Handbook-World-Class-Reliability-Organizations-ebook/dp/B01M9ASFQ3 is nowhere in these changes.

@jorgeorpinel jorgeorpinel merged commit fe91492 into iterative:master Jul 29, 2020
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.

2 participants