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

cmd: update push deps example #1591

Merged
merged 5 commits into from
Jul 29, 2020
Merged

Conversation

imhardikj
Copy link
Contributor

@imhardikj imhardikj commented Jul 18, 2020

Close #1445 by addressing the last pending checkbox:

  • push cmd ref.

@imhardikj imhardikj marked this pull request as draft July 18, 2020 22:15
@imhardikj imhardikj marked this pull request as ready for review July 18, 2020 22:37
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.

☝️

@imhardikj imhardikj changed the title Removing term Dvcfile in push cmdref push cmdref: removing term Dvcfile and updating example Jul 21, 2020
@imhardikj
Copy link
Contributor Author

@jorgeorpinel I have removed example-get started repo and instead set up a project for original example using dummy data and files.
I have used $cat dvc.yaml here currently but the dvc dag visualization of this dummy project can be viewed in this db08e42 commit.

@imhardikj imhardikj requested a review from jorgeorpinel July 21, 2020 21:14
@jorgeorpinel jorgeorpinel changed the title push cmdref: removing term Dvcfile and updating example cmd: update push example example Jul 22, 2020
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.

@imhardikj
Copy link
Contributor Author

@jorgeorpinel updated the example

@imhardikj imhardikj requested a review from jorgeorpinel July 22, 2020 14:09
@jorgeorpinel jorgeorpinel mentioned this pull request Jul 23, 2020
3 tasks
@jorgeorpinel jorgeorpinel changed the title cmd: update push example example cmd: update push deps example Jul 23, 2020
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.

Approved, but we need to wait until #1584 (comment) is merged before merging this one.

@shcheklein
Copy link
Member

hey, thanks @imhardikj , but I still see Dvcfile in the push.md - I think we should get rid of it completely to close this ticket?

@@ -181,11 +181,11 @@ One could do a simple `dvc push` to share all the data, but what if you only
want to upload part of the data?

```dvc
$ dvc push --with-deps matrix-train.p.dvc
$ dvc push --with-deps test-posts
Copy link
Member

Choose a reason for hiding this comment

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

we need to rewrite the example above, the project structure for this command to make sense?

Copy link
Member

Choose a reason for hiding this comment

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

also, why do change the stage name here, intentional?

Copy link
Contributor

Choose a reason for hiding this comment

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

rewrite the example above, the project structure for this command to make sense

Yes! It was also done in the complementary PR. Both are merged, please check in https://dvc.org/doc/command-reference/push#example-with-dependencies

why do change the stage name here, intentional?

Intentional. I came up with these new names. But now that you mention it matrix-train would've been the obvious choice here 🤦

Copy link
Contributor

@jorgeorpinel jorgeorpinel Jul 30, 2020

Choose a reason for hiding this comment

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

Wait no, that't the name of the next stage. That's why I came up with test-posts.


... Do some work based on the partial update

$ dvc push --with-deps model.p.dvc
$ dvc push --with-deps matrix-train
Copy link
Member

Choose a reason for hiding this comment

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

same here - why do we need to change the stage name?

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 think model is a good stage name.

@jorgeorpinel
Copy link
Contributor

thanks @imhardikj , but I still see Dvcfile in the push.md

Sorry that wsa confusing but I set it up that way: someone else was working on that part of the code so I asked Hardik to leave it alone. Now both PRs are merged and Dvcfile is gone from the code base 🙂

@imhardikj imhardikj deleted the pushcmd branch July 30, 2020 12:20
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.

term: stop using Dvcfile
3 participants