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

term: remove Dvcfile from repro cmd ref. #1504

Merged
merged 8 commits into from
Jul 5, 2020

Conversation

sarthakforwet
Copy link
Contributor

@sarthakforwet sarthakforwet commented Jun 27, 2020

The following PR completes one subtask of #1445 by removing/updating Dvcfile from repro.md.
This issue has also been referenced under #1255 .

Also remove Dvcfile

Updations:

  • Changed the output of dvc repro to the new one.
  • Updated the usage of dvc run command in Examples.

@sarthakforwet
Copy link
Contributor Author

@jorgeorpinel I guess the -c and -R options in repro.md also needs to be updated as they rely on specifying a target which is in dvc.yaml file now. What's your opinion over that?

@sarthakforwet

This comment has been minimized.

@jorgeorpinel
Copy link
Contributor

Hi @sarthakforwet this PR does not resolve #1445 completely so please don't use that special term in the description because it would cause #1445 to be closed if this is merged.

Comment on lines 27 to 28
`dvc repro` relies on the DAG definition that it reads from `dvc.yaml`, and uses
`dvc.lock` to determine what exactly needs to be run.
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks but this is out of scope for this PR. The whole cmd ref. needs to be rewritten so no use in adding a small update here, probably. Unless it was needed to explain the Dvcfile removal but I don' think that's the case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok Sure! will add it later when updating command reference.

Copy link
Contributor

Choose a reason for hiding this comment

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

See #1572

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.

Good overall but I left some specific comments.


I would recommend that after this PR is merged you try to help fully updating the whole dvc repro command reference for 1.0. That means the description and options, since it seems you already updated the examples accordingly 🙂

@sarthakforwet
Copy link
Contributor Author

sarthakforwet commented Jul 1, 2020

this PR does not resolve #1445 completely

@jorgeorpinel I intend to completely solve that issue here.
Please can you tell me what about Vim plugin I have to look at?
Also working on updating other files in the documentation regarding the use of Dvcfile.

@sarthakforwet
Copy link
Contributor Author

you try to help fully updating the whole dvc repro command reference for 1.0

Yeah sure! would love to do that 😁

@sarthakforwet sarthakforwet marked this pull request as draft July 2, 2020 13:17
@jorgeorpinel jorgeorpinel changed the title term: stop using Dvcfile term: remove Dvcfile from repro cmd ref. Jul 4, 2020
@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Jul 4, 2020

I intend to completely solve that issue here.

Let's not do this @sarthakforwet. It's a best practice to make PRs small (easy to review), get them merged, and then work on the remaining work in other PRs. I changed the title of this PR to reflect it's scope.

So again, please don't use that special term in the description because it would cause #1445 to be closed. Thanks

P/s but it would be much more useful if you help entirely updating the dvc repro cmd ref. first, TBH. You don't even have to finish #1445 (or update repro) if you don't have time for it soon. This is all just an exercise to see how we would collaborate under GSoD.

@jorgeorpinel jorgeorpinel mentioned this pull request Jul 4, 2020
3 tasks
@jorgeorpinel jorgeorpinel marked this pull request as ready for review July 4, 2020 01:39
@sarthakforwet
Copy link
Contributor Author

if you help entirely updating the dvc repro cmd ref. first

Sure, can you guide me with the relevant issues or should I open a new PR dedicated to completely update repro.md ?

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.

LGTM 🙂

@jorgeorpinel jorgeorpinel merged commit b00ef75 into iterative:master Jul 5, 2020
@jorgeorpinel
Copy link
Contributor

should I open a new PR dedicated to completely update repro.md

@sarthakforwet yes, this would be useful definitely. But again, no pressure in these any more. I think we've established good collaboration. I recommend you focus on polishing your GSoD application and share it with me for feedback if you haven't. We'll have the rest of July for collaborating on Gtihub.

@jorgeorpinel
Copy link
Contributor

Thanks for your contributions BTW!

@sarthakforwet
Copy link
Contributor Author

I recommend you focus on polishing your GSoD application and share it with me for feedback if you haven't.

yeah sure! Thanks!

@sarthakforwet
Copy link
Contributor Author

Thanks for your contributions BTW!

Pleasure! and thanks for support.

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