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

dvclive: Updates for implicit no step #3147

Merged
merged 11 commits into from
Jan 27, 2022
Merged

dvclive: Updates for implicit no step #3147

merged 11 commits into from
Jan 27, 2022

Conversation

daavoo
Copy link
Contributor

@daavoo daavoo commented Jan 3, 2022

@daavoo daavoo added the C: dvclive Content of /doc/dvclive label Jan 3, 2022
@daavoo daavoo self-assigned this Jan 3, 2022
@shcheklein shcheklein temporarily deployed to dvc-org-dvclive-no-step-vuh63u January 3, 2022 21:15 Inactive
@daavoo daavoo requested a review from dberenbaum January 3, 2022 21:16
@daavoo daavoo added the ⌛ status: wait-core-merge Waiting for related product PR merge/release label Jan 3, 2022
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.

Looks good! Can you also update https://dvc.org/doc/dvclive/api-reference/live/log to mention the metrics summary and/or no_step?

@jorgeorpinel
Copy link
Contributor

A bit confused by the concept of DVCLive without steps (or why that addresses iterative/dvclive#182). Plus I see there's a discussion on the semantics in iterative/dvclive#203 (comment) so I'll wait until when/if you request my review to take a closer look. Thanks

@shcheklein shcheklein temporarily deployed to dvc-org-dvclive-no-step-vuh63u January 19, 2022 18:45 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-dvclive-no-step-vuh63u January 19, 2022 19:26 Inactive
@daavoo daavoo changed the title dvclive: Add no_step dvclive: Updates for implicit no step Jan 19, 2022
@shcheklein shcheklein temporarily deployed to dvc-org-dvclive-no-step-vuh63u January 19, 2022 19:27 Inactive
@daavoo
Copy link
Contributor Author

daavoo commented Jan 19, 2022

A bit confused by the concept of DVCLive without steps (or why that addresses iterative/dvclive#182).

One of the outputs of DVCLive are linear plots, containing metric values associated to each iteration (step) of the training process.

When exploring DVCLive for ML scenarios where there are no iterations we realize that these outputs don't have any use so in iterative/dvclive#203 we introduce an implicit no step mode.

This makes DVCLive to don't generate linear plots unless next_step or set_step is called in the code, addressing iterative/dvclive#182.

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.

Lots of comments since this is a big change.

This also probably requires an update to the dvclive readme. We probably need some process to keep that updated.

When you don't update the step number in your code, DVCLive will **not**
generate:

- [linear plots](/doc/dvclive/get-started#linear-plots)
Copy link
Contributor

Choose a reason for hiding this comment

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

It wouldn't be obvious to me what linear plots means here (or throughout the rest of the docs), and it will be really confusing with iterative/dvclive#189. What was wrong with metrics logs?

Some other ideas:

  • metrics trends
  • metrics steps
  • trend logs
  • step logs

Copy link
Contributor Author

@daavoo daavoo Jan 21, 2022

Choose a reason for hiding this comment

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

  • What was wrong with metrics logs?

I just find metrics logs or the other alternatives a little abstract. I think that "data series", "scalar plots", or line/linear plots better indicate what it's being saved.

Copy link
Contributor

Choose a reason for hiding this comment

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

Without dvc plots, they are .tsv files, and that's what's actually being saved. IMO plots is a bit unnatural and DVC-specific. It also seems to me like the key feature of these are that they capture the metrics over time at each step, showing training progression. What would describe that? What about something like "step .tsv files"? I don't really like it but not sure what's better.

Do you feel the same about "metrics summary"? That seems similarly abstract to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It also seems to me like the key feature of these are that they capture the metrics over time at each step, showing training progression. What would describe that?

Metrics history?

Do you feel the same about "metrics summary"? That seems similarly abstract to me.

I don't know why but I don't feel the same for summary. Maybe because I can't think of alternatives 😅

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 "metrics history" is pretty good. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated with metrics history. Overall I like how it reads

Copy link
Contributor

@jorgeorpinel jorgeorpinel Jan 25, 2022

Choose a reason for hiding this comment

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

I don't see how "metrics logs" is abstract. It's pretty concrete and specific to what those TSV files are. It's just a bit confusing in the current Get Started since there's a "Log Metrics" section and later a "Metrics Logs" subsection. Probably there's no need for those subsections (under Outputs): just use paragraphs, then the terms don't become titles.

"Metrics logs" does also read funny because of the final s in both words though so maybe be more descriptive like TSV log files of your metrics or something like that (and metrics summary file (JSON format)).

"Metrics history" to me sounds like it could relate to the Git history more? Not sure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For me, history makes more sense than logs when combined with the other output, named summary. It feels like summary and history are "connected".

history is also a term used in Keras with similar meaning as here, so data scientist might be already familiar with it.

Probably there's no need for those subsections (under Outputs): just use paragraphs, then the terms don't become titles.

In subsequent P.R. I will remove the outputs subsections https://dvc-org-dvclive-data-hpgunbngf.herokuapp.com/doc/dvclive/get-started#outputs

content/docs/dvclive/get-started.md Outdated Show resolved Hide resolved
content/docs/dvclive/get-started.md Outdated Show resolved Hide resolved
content/docs/dvclive/api-reference/live/index.md Outdated Show resolved Hide resolved
content/docs/dvclive/api-reference/live/log.md Outdated Show resolved Hide resolved
content/docs/dvclive/api-reference/live/log.md Outdated Show resolved Hide resolved
@shcheklein shcheklein temporarily deployed to dvc-org-dvclive-no-step-vuh63u January 21, 2022 18:02 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-dvclive-no-step-vuh63u January 21, 2022 18:24 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-dvclive-no-step-vuh63u January 21, 2022 19:03 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-dvclive-no-step-vuh63u January 21, 2022 19:15 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-dvclive-no-step-vuh63u January 21, 2022 19:20 Inactive
@daavoo daavoo requested a review from dberenbaum January 21, 2022 20:22
@shcheklein shcheklein temporarily deployed to dvc-org-dvclive-no-step-vuh63u January 25, 2022 10:51 Inactive
@jorgeorpinel jorgeorpinel removed the C: dvclive Content of /doc/dvclive label Jan 27, 2022
@shcheklein shcheklein temporarily deployed to dvc-org-dvclive-no-step-vuh63u January 27, 2022 19:33 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-dvclive-no-step-vuh63u January 27, 2022 19:35 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-dvclive-no-step-vuh63u January 27, 2022 19:46 Inactive
@daavoo daavoo requested a review from dberenbaum January 27, 2022 19:55
@dberenbaum
Copy link
Contributor

Looks good! Just checking on previous comment from @jorgeorpinel about keeping the summaries at the top of the api methods.

@shcheklein shcheklein temporarily deployed to dvc-org-dvclive-no-step-vuh63u January 27, 2022 20:31 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-dvclive-no-step-vuh63u January 27, 2022 20:52 Inactive
@daavoo daavoo requested a review from dberenbaum January 27, 2022 21:23
@daavoo daavoo merged commit 26ec1be into master Jan 27, 2022
@daavoo daavoo deleted the dvclive-no-step branch January 27, 2022 22:16
iesahin pushed a commit that referenced this pull request Apr 11, 2022
* dvclive: Updates for implicit no step.

* Apply suggestions from code review

Co-authored-by: Dave Berenbaum <[email protected]>

* remove no-step section

* Update description

* Add no step warning for summary and resume

* dvclive: Rename linear plots to metric history

* Bring back short descriptions

* Update content/docs/dvclive/dvclive-with-dvc.md

Co-authored-by: Dave Berenbaum <[email protected]>

* Update warnings

* Update content/docs/dvclive/api-reference/live/index.md

Co-authored-by: Dave Berenbaum <[email protected]>

* Add missing descriptions

Co-authored-by: Dave Berenbaum <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants