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

Update GS experiments with dvc exp init #3051

Closed
wants to merge 37 commits into from

Conversation

iesahin
Copy link
Contributor

@iesahin iesahin commented Nov 30, 2021

This one is migrated from Notion. I have added a hidden section on hyperparameters there.

I have modified here:

  • It starts with dvc exp init now. Without mentioning any pipelines stuff or dvc.yaml

  • I'm planning to remove "If you've run repro before" hidden section, the section where we tell how to prepare pipelines for arbitrary projects, and keep the hyperparameter stuff to a minimum.

Closes #3072
Closes #3118

This one will be closed after merging the following small PRs:

@iesahin iesahin requested a review from dberenbaum November 30, 2021 17:39
@shcheklein shcheklein temporarily deployed to dvc-org-iesahin-update--7fqxw0 November 30, 2021 17:39 Inactive
@iesahin iesahin requested a review from shcheklein November 30, 2021 17:40
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 me know if you think this direction makes sense @iesahin!

content/docs/start/experiments.md Outdated Show resolved Hide resolved
content/docs/start/experiments.md Outdated Show resolved Hide resolved
@iesahin
Copy link
Contributor Author

iesahin commented Dec 1, 2021

I'm copying @jorgeorpinel 's comments in Notion:

TBH I'm not sure I love hidden sections for metrics/params. They're usually reserved for off-topic implementation details. Maybe some of the metrics info can be collapsed but a basic description that is relevant to the trail should be in a regular paragraph, I think.

About the sentence starting with "This experiment is then associated with the values in parameters file":

Parameters mention start all the way up here. If we're going to insert params info after the exp show table, is there any way to avoid the concept before it? Let's try to all of the params info until after the table so it's consolidated and more logical.

@iesahin
Copy link
Contributor Author

iesahin commented Dec 1, 2021

It looks we still need to use dvc init before dvc exp init, right?

dvc exp init python3 src/train.py
ERROR: you are not inside of a DVC repository (checked up to mount point '/home/iex/work')

Is there any work to check whether we're inside a DVC repository and dvc init if we're not? @dberenbaum

@dberenbaum
Copy link
Contributor

It looks we still need to use dvc init before dvc exp init, right?

dvc exp init python3 src/train.py
ERROR: you are not inside of a DVC repository (checked up to mount point '/home/iex/work')

Is there any work to check whether we're inside a DVC repository and dvc init if we're not? @dberenbaum

This has been deprioritized for now, so dvc init is still needed.

@shcheklein shcheklein temporarily deployed to dvc-org-iesahin-update--7fqxw0 December 8, 2021 16:48 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-iesahin-update--7fqxw0 December 13, 2021 14:14 Inactive
@shcheklein shcheklein had a problem deploying to dvc-org-iesahin-update--7fqxw0 December 14, 2021 11:04 Failure
@shcheklein shcheklein temporarily deployed to dvc-org-iesahin-update--7fqxw0 December 14, 2021 21:30 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-iesahin-update--7fqxw0 December 15, 2021 15:25 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-iesahin-update--7fqxw0 December 17, 2021 16:58 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-iesahin-update--7fqxw0 December 17, 2021 17:17 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-iesahin-update--7fqxw0 December 23, 2021 11:28 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-iesahin-update--7fqxw0 December 23, 2021 11:37 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-iesahin-update--7fqxw0 December 23, 2021 11:46 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-iesahin-update--7fqxw0 December 23, 2021 11:51 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-iesahin-update--7fqxw0 December 23, 2021 13:06 Inactive
@iesahin iesahin marked this pull request as ready for review December 23, 2021 13:06
@iesahin
Copy link
Contributor Author

iesahin commented Dec 23, 2021

This one is ready for review @shcheklein @dberenbaum @jorgeorpinel @julieg18

@shcheklein shcheklein temporarily deployed to dvc-org-iesahin-update--7fqxw0 December 28, 2021 13:45 Inactive
@iesahin iesahin force-pushed the iesahin/update-gs-experiments-with-metrics branch from bef8d07 to 3369e5e Compare January 6, 2022 16:22
@shcheklein shcheklein temporarily deployed to dvc-org-iesahin-update--7fqxw0 January 6, 2022 16:22 Inactive
@iesahin iesahin marked this pull request as draft January 6, 2022 16:32
@iesahin
Copy link
Contributor Author

iesahin commented Jan 6, 2022

Converted to draft. Will work on these in separate branches and this will be closed after other PRs are submitted.

@iesahin
Copy link
Contributor Author

iesahin commented Jan 6, 2022

#3112 modified to include --drop (but not --keep) in experiments.md instead of --include-params etc. I'll base the modifications on the current master and won't deal with it again.

@@ -138,6 +111,34 @@ Experiment results have been applied to your workspace.

<details>

### ℹ️ More information about (Hyper)parameters
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 section is in #3163

Copy link
Contributor

Choose a reason for hiding this comment

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

#3163 is merged. Should master be merged into this branch so this change goes away, and resolve the comment?

@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Jan 13, 2022

#3051 is reviewed & merged bit by bit. It will have 2 or 3 more PRs that will apply all the changes.

Interesting method. Similar to a nested PR or PR series but less messy. However, it can also be confusing to review in 2 places.

What's the exact use of this draft if it's split into other PRs? Should anyone be looking at the changes? What parts)? Thanks

@iesahin
Copy link
Contributor Author

iesahin commented Jan 14, 2022

This is for my own tracking. For longer documents, I can rebase time to time to see the remaining parts. For this one, I don't expect any review. It's like a guide to move the document.

@jorgeorpinel jorgeorpinel added the status: wontfix This will not be worked on (e.g. a known issue). label Jan 18, 2022
@iesahin iesahin added status: splitting and removed status: wontfix This will not be worked on (e.g. a known issue). labels Jan 19, 2022
@iesahin
Copy link
Contributor Author

iesahin commented Feb 23, 2022

This one is completed, now GS:Experiments starts with dvc exp init. Thank you for your reviews @jorgeorpinel @dberenbaum @shcheklein

@iesahin iesahin closed this Feb 23, 2022
@jorgeorpinel jorgeorpinel deleted the iesahin/update-gs-experiments-with-metrics branch July 29, 2022 17:08
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.

start: update Experiments with dvc exp show --drop/keep start: clarify rel. between exp run and repro.
5 participants