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

start: Add new experiments section #4393

Merged
merged 16 commits into from
Mar 20, 2023
Merged

start: Add new experiments section #4393

merged 16 commits into from
Mar 20, 2023

Conversation

daavoo
Copy link
Contributor

@daavoo daavoo commented Mar 15, 2023

No description provided.

@daavoo daavoo requested a review from a team March 15, 2023 18:17
@shcheklein shcheklein temporarily deployed to dvc-org-experiments-tra-ntv0xg March 15, 2023 18:20 Inactive
@github-actions
Copy link
Contributor

github-actions bot commented Mar 15, 2023

Link Check Report

There were no links to check!

@shcheklein shcheklein temporarily deployed to dvc-org-experiments-tra-bnvp9j March 15, 2023 18:28 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-experiments-tra-uraaki March 15, 2023 18:36 Inactive
@daavoo daavoo closed this Mar 15, 2023
@daavoo daavoo reopened this Mar 15, 2023
@daavoo daavoo force-pushed the experiments-trail branch from c6b5bab to 37f5469 Compare March 15, 2023 18:44
@shcheklein shcheklein temporarily deployed to dvc-org-experiments-tra-o7ac0h March 15, 2023 18:46 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-experiments-tra-flfifr March 15, 2023 18:53 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-experiments-tra-aokvc6 March 15, 2023 19:01 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-experiments-tra-6tb4bm March 15, 2023 19:08 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-experiments-tra-7jo3an March 15, 2023 19:15 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-experiments-tra-doizqr March 15, 2023 19:22 Inactive
@daavoo daavoo closed this Mar 15, 2023
@daavoo daavoo reopened this Mar 15, 2023
@shcheklein shcheklein temporarily deployed to dvc-org-experiments-tra-kn6jyw March 15, 2023 21:49 Inactive
@daavoo daavoo closed this Mar 15, 2023
@daavoo daavoo reopened this Mar 15, 2023
@shcheklein shcheklein temporarily deployed to dvc-org-experiments-tra-qdise9 March 15, 2023 22:02 Inactive
@daavoo daavoo force-pushed the experiments-trail branch from 37f5469 to 98619d2 Compare March 15, 2023 22:08
@shcheklein shcheklein temporarily deployed to dvc-org-experiments-tra-rvwges March 15, 2023 22:09 Inactive
@dberenbaum

This comment was marked as outdated.

@daavoo

This comment was marked as outdated.

@dberenbaum

This comment was marked as outdated.

Comment on lines +61 to +63

trainer.add_callback(DVCLiveCallback(save_dvc_exp=True))
trainer.train()
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a blocker but a follow-up to #4367: should we use the context manager everywhere to be safe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should we use the context manager everywhere to be safe?

Not sure.

Given that we are showing the basic callback usage, I am not sure it's worth it to use the context manager here.
Just including it without practical use would make the basic usage look more complicated than it is.

We could show logging additional stuff after the callback but that would complicate the overall example

@shcheklein shcheklein temporarily deployed to dvc-org-experiments-tra-rvwges March 17, 2023 11:58 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-experiments-tra-rvwges March 17, 2023 13:37 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-experiments-tra-rvwges March 17, 2023 13:44 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-experiments-tra-rvwges March 17, 2023 13:45 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-experiments-tra-rvwges March 17, 2023 13:49 Inactive
@dberenbaum
Copy link
Contributor

Sorry, I'm taking a long time on this and still only got through the 1st two pages. Should we cut the PR down even further to only the first 1-2 pages to get it merged even faster?

Done

Thank you! Feel free to open a PR for the other sections and I will start reviewing.

If you have [data tracked by DVC](/doc/start/data-management/data-versioning),
it will be automatically included in the experiment.

Once you have added [DVCLive] to your code, you can track the changes with git:
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I get what this sentence is trying to say. Are we trying to make sure the code gets included with the experiments? If so, maybe it needs to be more explicit, like your code will be included with your experiment if the code file is tracked by git or something like that.

Copy link
Contributor Author

@daavoo daavoo Mar 20, 2023

Choose a reason for hiding this comment

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

I am removing it for now and will revisit it if/when we try to make the example more reproducible or tied to the example repo.


</admon>

## Persisting
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we go with apply here? We have tried to put branch first but apply continues to have higher usage. It also doesn't have the awkwardness of a new branch name or leaving a dirty git workspace to clean up, and it's easier to see that everything is in your workspace. Also, I expect branch will seem kind of redundant once you can branch/PR experiments from Studio.

Copy link
Contributor Author

@daavoo daavoo Mar 20, 2023

Choose a reason for hiding this comment

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

Can we go with apply here? We have tried to put branch first but apply continues to have higher usage.

I feel like we have gone in circles with this discussion 😅 .

My reasoning is that exp branch is easier to explain/understand (we are in the get started) and less likely to fail or cause unexpected results.

It also doesn't have the awkwardness of a new branch name

iterative/dvc#9211

or leaving a dirty git workspace to clean up, and it's easier to see that everything is in your workspace.

I don't understand how that is not also true for exp apply.

Also, I expect branch will seem kind of redundant once you can branch/PR experiments from Studio.

I don't understand the redundancy point.
It's like the tabs we already have where you do the same in different contexts.
When it's implemented in Studio, we can just show it in a new tab.

@dberenbaum
Copy link
Contributor

Thanks @daavoo! A few questions left, and the links still need to be updated, but I'm ready to approve once all that is done.

Co-authored-by: Dave Berenbaum <dave@iterative.ai>
@shcheklein shcheklein temporarily deployed to dvc-org-experiments-tra-rvwges March 20, 2023 11:06 Inactive
Co-authored-by: Restyled.io <commits@restyled.io>
@shcheklein shcheklein temporarily deployed to dvc-org-experiments-tra-rvwges March 20, 2023 11:11 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-experiments-tra-rvwges March 20, 2023 11:41 Inactive
@daavoo daavoo requested review from dberenbaum and removed request for a team March 20, 2023 11:42
@shcheklein shcheklein temporarily deployed to dvc-org-experiments-tra-rvwges March 20, 2023 11:56 Inactive
Copy link
Contributor

@dberenbaum dberenbaum left a comment

Choose a reason for hiding this comment

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

Let's merge it since nothing left is a blocker and it's all a big improvement over what we have. I may follow up on some of the lingering discussions. Thanks again!

@daavoo daavoo merged commit de4dec2 into main Mar 20, 2023
@daavoo daavoo deleted the experiments-trail branch March 20, 2023 13:10
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.

3 participants