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: Deprecate live section of dvc.yaml. #3411

Merged
merged 17 commits into from
May 12, 2022
Merged

Conversation

daavoo
Copy link
Contributor

@daavoo daavoo commented Mar 31, 2022

In favor of reusing metrics and plots.

Closes iterative/dvclive#77
Closes iterative/dvclive#229
Closes iterative/dvclive#230

In favor of reusing `metrics` and `plots`.
Closes iterative/dvclive#229
@daavoo daavoo added the ⌛ status: wait-core-merge Waiting for related product PR merge/release label Mar 31, 2022
@daavoo daavoo self-assigned this Mar 31, 2022
@daavoo daavoo removed the ⌛ status: wait-core-merge Waiting for related product PR merge/release label Apr 18, 2022
@daavoo daavoo marked this pull request as ready for review April 18, 2022 15:05
@@ -97,13 +97,11 @@ $ dvc stage add --name train \
--params seed,lr,weight_decay \
--checkpoints model.pt \
--plots-no-cache predictions.json \
--live dvclive \
--plots-no-cache dvclive/scalars \
Copy link
Contributor

Choose a reason for hiding this comment

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

Any particular reason to use --plots-no-cache instead of --plots like above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that we should encourage plots-no-cache in the default example usages (for dvclive/scalars)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No longer sure about this as default 😅 .

I was thinking it might be better to document the previous default behavior:

metrics:
  - dvclive.json:
      cache: True
plots:
  - dvclive

And later document this flexibility in as an admonition or something.

Copy link
Contributor

@jorgeorpinel jorgeorpinel May 11, 2022

Choose a reason for hiding this comment

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

What do you mean "as default"? Do you mean this is an implicit recommendation? Well, up to you here @daavoo. I think -no-cache makes more sense for small text files Git can handle but maybe that context is lost in the sample, and regular --plots is def. easier to read.

Maybe the Q is whether --plots should by default not commit outputs to cache, but that's a core Q and I think it was discussed some time ago.

@@ -97,13 +97,11 @@ $ dvc stage add --name train \
--params seed,lr,weight_decay \
--checkpoints model.pt \
--plots-no-cache predictions.json \
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know if --plots-no-cache predictions.json is used at all? Seems like that file isn't meant to be a plot and maybe should be --outs-no-cache instead. cc @flippedcoder

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! I can take another look once deployments are up again.

@jorgeorpinel
Copy link
Contributor

Hi. Why was the live section of dvc.yaml deprecated?

Comment on lines 135 to 131
> This only has an effect when used with `--type=dl`.
- `--live` - use the given path to set up `metrics` and `plots` as in
Copy link
Contributor

Choose a reason for hiding this comment

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

--type=dl is no longer needed? Is it still a thing? That option desc. may also need an update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The thing is that:

This only has an effect when used with --type=dl

Was not true at all. Passing live can have effect regardless of whether you use --type=dl or not

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. So is the --type description accurate?

dl stages are intended for use in deep-learning scenarios, where metrics and plots are tracked with DVCLive.

Copy link
Contributor Author

@daavoo daavoo May 10, 2022

Choose a reason for hiding this comment

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

It needs to be revisited now that iterative/dvc#7703 has been merged. It belongs to different P.R. though

Copy link
Contributor

Choose a reason for hiding this comment

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

@gatsby-cloud
Copy link

gatsby-cloud bot commented May 1, 2022

Gatsby Cloud Build Report

dvc.org

🎉 Your build was successful! See the Deploy preview here.

Build Details

View the build logs here.

🕐 Build time: 1m

Performance

Lighthouse report

Metric Score
Performance 🔶 61
Accessibility 💚 98
Best Practices 🔶 83
SEO 💚 93

🔗 View full report

@julieg18

This comment was marked as resolved.

@jorgeorpinel
Copy link
Contributor

Yup! Thanks @julieg18 . Cc @dberenbaum you can see it here: https://dvcorg-deprecatelive.gtsb.io/doc/dvclive

@daavoo daavoo requested a review from jorgeorpinel May 4, 2022 15:04
@dberenbaum
Copy link
Contributor

Waiting to resolve iterative/dvc#7534.

@dberenbaum dberenbaum added the ⌛ status: wait-core-merge Waiting for related product PR merge/release label May 4, 2022
@jorgeorpinel
Copy link
Contributor

Waiting to resolve iterative/dvc#7534.

I see. Probably doesn't have to block this PR tho?

@dberenbaum
Copy link
Contributor

dberenbaum commented May 5, 2022

Probably doesn't have to block this PR tho?

Yup, you are right, although it will make documenting exp init --live behavior much simpler. We can move forward and see which gets done first.

Edit: opened iterative/dvc#7703 to move it along.

@dberenbaum dberenbaum removed the ⌛ status: wait-core-merge Waiting for related product PR merge/release label May 5, 2022
@daavoo daavoo requested a review from dberenbaum May 10, 2022 09:46
@daavoo
Copy link
Contributor Author

daavoo commented May 10, 2022

@dberenbaum @jorgeorpinel could you take another look to see what's missing?

I have addressed everything except for #3411 (comment) , which I believe requires separate P.R. to update (as type="dl" no longer exists)

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.

One sug ⬆️

@dberenbaum dberenbaum merged commit 277a49f into master May 12, 2022
@dberenbaum dberenbaum deleted the deprecate-live branch May 12, 2022 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants