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

live: markdown report support. #3856

Merged
merged 5 commits into from
Aug 8, 2022
Merged

live: markdown report support. #3856

merged 5 commits into from
Aug 8, 2022

Conversation

daavoo
Copy link
Contributor

@daavoo daavoo commented Aug 1, 2022

@daavoo daavoo added the C: dvclive Content of /doc/dvclive label Aug 1, 2022
@daavoo daavoo requested a review from dberenbaum August 1, 2022 15:30
@daavoo daavoo self-assigned this Aug 1, 2022
@shcheklein shcheklein temporarily deployed to dvc-org-dvclive-markdow-9hd16s August 1, 2022 15:31 Inactive
@daavoo
Copy link
Contributor Author

daavoo commented Aug 1, 2022

Only added "API Reference" updates. Not sure (if) how to document the full "Live metrics" scenario with CML.
Maybe a new dvclive-with-cml page?

@github-actions
Copy link
Contributor

github-actions bot commented Aug 1, 2022

Link Check Report

All 4 links passed!

Comment on lines 38 to 42
- `dir` - Location of the directory to store
[outputs](/doc/dvclive/get-started#outputs).

- `summary_path` - `{dir}.json`. Location of the
- `summary_path` - `{Live.dir}.json`. Location of the
[summary](/doc/dvclive/api-reference/live/log#description).
Copy link
Contributor

@jorgeorpinel jorgeorpinel Aug 2, 2022

Choose a reason for hiding this comment

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

Hola. Why this change? Was just dir incorrect? It's listed as just dir above. Same Q applies to a bunch of other changes here.

Seems unrelated to iterative/dvclive#266 too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

{dir} and {path} were being used inconsistently across docs. I wanted to consolidate a single one.

Here, the dir definition it's right above, but in the other references that context doesn't exist and I wanted to emphasize that it refers to the dir property of the Live class. So I went with Live.dir.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems unrelated to iterative/dvclive#266 too.

It has nothing to do, just something I noticed during this P.R. Could be extracted to other

Comment on lines -68 to -69
- `auto_open` - If `True`, on the first `Live.make_report()` call, DVCLive will
automatically open `html_path` in a browser. _Default_: `False`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait what happened to auto_open?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Docs were outdated. I just noticed this was still here during this P.R.
It was removed a few versions ago, discussion here iterative/dvclive#241

Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

Combined report param suggestion:

content/docs/dvclive/api-reference/live/index.md Outdated Show resolved Hide resolved
content/docs/dvclive/api-reference/live/index.md Outdated Show resolved Hide resolved
@shcheklein shcheklein temporarily deployed to dvc-org-dvclive-markdow-9hd16s August 4, 2022 14:16 Inactive
@shcheklein shcheklein had a problem deploying to dvc-org-dvclive-markdow-9hd16s August 4, 2022 14:17 Failure
@shcheklein shcheklein temporarily deployed to dvc-org-dvclive-markdow-9hd16s August 4, 2022 14:43 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-dvclive-markdow-9hd16s August 4, 2022 14:49 Inactive
@daavoo
Copy link
Contributor Author

daavoo commented Aug 8, 2022

@jorgeorpinel merging the current state. If there are any suggestions left, please comment and I will address them in a follow-up

@daavoo daavoo merged commit a07b39b into main Aug 8, 2022
@daavoo daavoo deleted the dvclive-markdown branch August 8, 2022 18:27
Copy link
Contributor

@casperdcl casperdcl left a comment

Choose a reason for hiding this comment

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

late review

@@ -1,6 +1,6 @@
# Live.make_report()

Generates an HTML Report from the logged data.
Generates a metrics report from the logged data.
Copy link
Contributor

Choose a reason for hiding this comment

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

should we say "metrics & plots?"

[html report](/doc/dvclive/api-reference/live/make_report#description).
- `report_path` - `{Live.dir}/report.{format}`. Location of the
[metrics report](/doc/dvclive/api-reference/live/make_report). The `format`
can be HTML) or Markdown depending on the value of the `report` parameter.
Copy link
Contributor

Choose a reason for hiding this comment

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

unmatched parenthesis

On each call, DVCLive will collect all the data logged in `{Live.dir}`, generate
a report and save it in `{Live.dir}/report.{format}`.

The `format` can be HTML) or Markdown depending on the value of the `report`
Copy link
Contributor

Choose a reason for hiding this comment

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

unmatched parenthesis

@@ -62,11 +63,11 @@ other metadata.

</admon>

- `report` - If `html`, DVCLive will call `Live.make_report()` on each step
update. _Default_: `html`.
- `report` - If `auto`,`html`, or `md`, DVCLive will call `Live.make_report()`
Copy link
Contributor

Choose a reason for hiding this comment

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

missing space after comma

@@ -113,16 +113,16 @@ The [metrics summary](/doc/dvclive/api-reference/live/log#description) in
The [metrics history](/doc/dvclive/api-reference/live/log#step-updates)
`training_metrics/scalars` can be visualized with `dvc plots`.

The [HTML report](/doc/dvclive/api-reference/live/make_report#description) in
The [metrics report](/doc/dvclive/api-reference/live/make_report) in
Copy link
Contributor

Choose a reason for hiding this comment

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

"metrics & plots?"

Comment on lines +124 to +125
If you don't update the step number, the report won't be generated unless you
call `Live.make_report()` directly.
Copy link
Contributor

Choose a reason for hiding this comment

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

lots of double negatives; hard to follow. Maybe "Reports are generated if you update the step number or call Live.make_report() directly."

@@ -109,17 +109,20 @@ not.

See `Live.log()`, `Live.log_image()` and `Live.log_plot()` for more details.

### HTML report
### Metrics report
Copy link
Contributor

Choose a reason for hiding this comment

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

"Metrics & plots"


![](/img/dvclive-html.gif)

The `format` can be HTML) or Markdown depending on the value of the `report`
Copy link
Contributor

Choose a reason for hiding this comment

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

unmatched parenthesis

Comment on lines +124 to +125
If you don't update the step number, the report won't be generated unless you
call `Live.make_report()` directly.
Copy link
Contributor

Choose a reason for hiding this comment

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

"Reports are generated if you update the step number or call Live.make_report() directly."

@jorgeorpinel jorgeorpinel added A: docs Area: user documentation (gatsby-theme-iterative) C: ref Content of /doc/*-reference labels Sep 8, 2022
@jorgeorpinel
Copy link
Contributor

Did you have a chance to follow-up on @casperdcl's comments @daavoo ? Maybe you can include any updates that still apply to #3923.

@daavoo
Copy link
Contributor Author

daavoo commented Sep 14, 2022

Did you have a chance to follow-up on @casperdcl's comments @daavoo ? Maybe you can include any updates that still apply to #3923.

I will include the updates there

@jorgeorpinel
Copy link
Contributor

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: docs Area: user documentation (gatsby-theme-iterative) C: dvclive Content of /doc/dvclive C: ref Content of /doc/*-reference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants