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

guide: note that exp run commits data #3271

Merged
merged 5 commits into from
Feb 12, 2022
Merged

Conversation

jorgeorpinel
Copy link
Contributor

@jorgeorpinel jorgeorpinel commented Feb 11, 2022

and other misc edits to Running Exps, and to the gc ref.

To close #2418

other misc edits to Running Exps
@shcheklein shcheklein temporarily deployed to dvc-org-exp-run-commit--rs7ac0 February 11, 2022 04:13 Inactive
@jorgeorpinel jorgeorpinel marked this pull request as ready for review February 11, 2022 04:19
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-org-exp-run-commit--rs7ac0 February 11, 2022 04:19 Inactive
Comment on lines 27 to 30
`targets` and stage execution (restores the dependency graph, etc.). One
important difference is that changed dependencies are committed to the <abbr>DVC
cache</abbr> when the experiment begins execution, to guarantee the
reproducibility of any recorded experiment.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One question for this PR's reviewer(s): #2418 was about performance implications. Should this be stressed further here or in the guide?

For now I may just link to gc and check whether that ref. mentions exp gc(as vice versa).

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 just noting that it is committed is fine

Copy link
Contributor

Choose a reason for hiding this comment

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

See my comment in #2418 (comment). dvc repro also commits changed dependencies to the DVC cache, so I don't see a difference here.

I'm having flashbacks to our original discussion about this 😅 . I think this initially came up because dvc exp run didn't used to commit changed dependencies, but we changed it to behave more like dvc repro.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this initially came up because dvc exp run didn't used to commit changed dependencies, but we changed it to behave more like dvc repro

Right I just went all the way back to iterative/dvc#5593, iterative/dvc#5859, and #2418 and it seems that's what the change was. So this paragraph about being different to repro is incorrect... ⌛

Copy link
Contributor Author

@jorgeorpinel jorgeorpinel Feb 11, 2022

Choose a reason for hiding this comment

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

OK I updated the changes... PTAL @dberenbaum

I think just noting that it is committed is fine

Yeah that's what I did now. I didn't specifically mention it in the context of queueing, just left generic language that applies to running or queueing ("when preparing the experiment").

@jorgeorpinel jorgeorpinel temporarily deployed to dvc-org-exp-run-commit--rs7ac0 February 11, 2022 05:01 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-org-exp-run-commit--rs7ac0 February 11, 2022 05:12 Inactive
@jorgeorpinel jorgeorpinel self-assigned this Feb 11, 2022
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-org-exp-run-commit--rs7ac0 February 11, 2022 18:46 Inactive
@jorgeorpinel jorgeorpinel merged commit 4f1e30d into master Feb 12, 2022
@jorgeorpinel jorgeorpinel deleted the exp-run/commit-repro branch February 12, 2022 02:52
iesahin pushed a commit that referenced this pull request Apr 11, 2022
* guide: note that `exp run` commits data and
other misc edits to Running Exps

* ref: note that `exp run` commits dependencies

* ref: udpates to `gc` ref inc. link to `exp gc`

* exp run: clarify implicit implications of committing deps
per #3271 (review)
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.

exp run: internal data committing performance implications
4 participants