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.

Sign up for GitHub

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

Jump to bottom

ref: reduce overlap between repro and stage add #4026

Merged
merged 4 commits into from
Mar 13, 2023
Merged

ref: reduce overlap between repro and stage add #4026

merged 4 commits into from
Mar 13, 2023

Conversation

jorgeorpinel
Copy link
Contributor

@jorgeorpinel jorgeorpinel commented Oct 7, 2022 •
edited
Loading

@jorgeorpinel
ref: changes and simplifications ...
d6342f5 
around the pipeliens/ DAG concepts
@jorgeorpinel jorgeorpinel added A: docs Area: user documentation (gatsby-theme-iterative) C: ref Content of /doc/*-reference labels Oct 7, 2022
@shcheklein shcheklein temporarily deployed to dvc-org-ref-simplify-pi-08rtdu October 7, 2022 03:52 Inactive
@jorgeorpinel jorgeorpinel changed the title ref: simplifications around "pipelines"/"DAG" concepts ref: simplifify repro, run, stage add (overlaps) Oct 7, 2022
@jorgeorpinel jorgeorpinel marked this pull request as ready for review October 7, 2022 03:53
@github-actions
Copy link
Contributor

github-actions bot commented Oct 7, 2022

d6342f5

Link Check Report

All 17 links passed!

CML watermark

@jorgeorpinel jorgeorpinel added the p2-nice-to-have Less of a priority at the moment. We don't usually deal with this immediately. label Oct 7, 2022
@shcheklein
Copy link
Member

@jorgeorpinel please resolve the conflicts, please add some summary on what is going on here and where should we pay attention to, thanks!

@shcheklein shcheklein temporarily deployed to dvc-org-ref-simplify-pi-wzm3mj December 22, 2022 09:46 Inactive
@jorgeorpinel jorgeorpinel changed the title ref: simplifify repro, run, stage add (overlaps) ref: reduce overlap between repro, run, and stage add Dec 22, 2022
@github-actions
Copy link
Contributor

Link Check Report

All 17 links passed!

@jorgeorpinel
Copy link
Contributor Author

jorgeorpinel commented Dec 22, 2022 •
edited
Loading

@shcheklein I think this was meant to optimize similar explanations across the 3 related cmd refs mentioned in the title.

Due to the messy diff in this one, it may be easier to compare the existing cmd ref pages vs. the new ones (listed in the OP).

This is p2 though, so I'm finishing other things first. We can always close this PR (temporarily or permanently) or ask the eng team to review/approve/merge it when they can (since it's purely changes to the cmd ref).

@jorgeorpinel jorgeorpinel requested a review from a team January 26, 2023 16:56
@jorgeorpinel jorgeorpinel self-assigned this Feb 20, 2023
@shcheklein shcheklein temporarily deployed to dvc-org-ref-simplify-pi-rxz0nu February 23, 2023 20:24 Inactive
@jorgeorpinel jorgeorpinel changed the title ref: reduce overlap between repro, run, and stage add ref: reduce overlap between repro and stage add Feb 23, 2023
@jorgeorpinel
Copy link
Contributor Author

Update: now that the run ref is gone, this should be easier to review 🙂

Will leave it up to your team @dberenbaum (sorry to keep pinging you from multiple places).

@jorgeorpinel jorgeorpinel removed their assignment Feb 23, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Feb 23, 2023 •
edited
Loading

Link Check Report

There were no links to check!

dberenbaum
dberenbaum reviewed Feb 24, 2023
View reviewed changes
content/docs/command-reference/stage/add.md
Comment on lines -99 to -103
- Outputs are deleted from the workspace before executing the command (including
at `dvc repro`) if their paths are found as existing files/directories (unless
`--outs-persist` is used). This also means that the stage command needs to
recreate any directory structures defined as outputs every time its executed
by DVC.
Copy link
Contributor

@dberenbaum dberenbaum Feb 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@jorgeorpinel Do you mind explaining why you dropped this note?

Edit: I guess you modified it, but it seems less explicit now without Outputs are deleted from the workspace before executing the command

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I was just making it shorter I guess. Idk this was a long time ago. I've reinstated the first part so it's explicit again. PTAL

dberenbaum
dberenbaum reviewed Feb 24, 2023
View reviewed changes
content/docs/command-reference/stage/add.md Outdated
Comment on lines 89 to 90
- Stage commands need to recreate any directory structures defined as outputs
every time its executed by DVC.
Copy link
Contributor

@dberenbaum dberenbaum Feb 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
- Stage commands need to recreate any directory structures defined as outputs
every time its executed by DVC.
- Outputs are deleted from the workspace before executing the command (unless
`--outs-persist` is used). Stage commands need to recreate any directory structures defined as outputs
every time it's executed by DVC.

@jorgeorpinel I will probably go with something like this unless there's a strong reason to drop the first sentence completely.

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, I already rephrased in a similar way. Sure up to you 🙂

@shcheklein shcheklein temporarily deployed to dvc-org-ref-simplify-pi-rxz0nu February 25, 2023 04:50 Inactive
@dberenbaum dberenbaum merged commit b9bb9d2 into main Mar 13, 2023
@dberenbaum dberenbaum deleted the ref/simplify-pipes branch March 13, 2023 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dberenbaum dberenbaum dberenbaum approved these changes

Assignees
No one assigned
Labels
A: docs Area: user documentation (gatsby-theme-iterative) C: ref Content of /doc/*-reference p2-nice-to-have Less of a priority at the moment. We don't usually deal with this immediately.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jorgeorpinel @shcheklein @dberenbaum