Support `artifacts` section #521

daavoo · 2023-04-11T16:54:01Z

Closes #519

daavoo

Not sure how to try this "end to end" until there is a Studio P.R. ready

codecov-commenter · 2023-04-11T17:05:01Z

Codecov Report

Patch coverage: 96.42% and project coverage change: +0.14 🎉

Comparison is base (1cb31aa) 95.74% compared to head (d942cd9) 95.88%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #521      +/-   ##
==========================================
+ Coverage   95.74%   95.88%   +0.14%     
==========================================
  Files          41       41              
  Lines        2606     2698      +92     
  Branches      223      238      +15     
==========================================
+ Hits         2495     2587      +92     
  Misses         66       66              
  Partials       45       45

Impacted Files	Coverage Δ
src/dvclive/live.py	`92.13% <80.00%> (+0.33%)`	⬆️
src/dvclive/dvc.py	`93.38% <100.00%> (+0.28%)`	⬆️
src/dvclive/fastai.py	`86.27% <100.00%> (ø)`
src/dvclive/huggingface.py	`93.10% <100.00%> (ø)`
src/dvclive/keras.py	`100.00% <100.00%> (ø)`
src/dvclive/utils.py	`84.37% <100.00%> (+2.02%)`	⬆️
tests/test_frameworks/test_fastai.py	`100.00% <100.00%> (ø)`
tests/test_frameworks/test_huggingface.py	`98.05% <100.00%> (ø)`
tests/test_frameworks/test_keras.py	`100.00% <100.00%> (ø)`
tests/test_log_artifact.py	`100.00% <100.00%> (ø)`
... and 1 more

... and 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

src/dvclive/live.py

dberenbaum · 2023-04-11T18:26:13Z

Thanks @daavoo!

Questions I have:

Not a blocker, but this reminds me we need to revisit the lightning callback since it's most popular but doesn't auto-log the model artifact.
Do we know what the artifact names look like for each framework? Should we be inferring them from the path or trying to come up with some better name for them?
If we log the artifact at the end of each step/epoch, will it raise an error because another artifact exists with the same name?

aguschin

Looks great overall (and simple - I like that very much after mlem/dvc codebases 😅 ). I see that Dave already posted feedback - have only one few items to add on top of that.

src/dvclive/live.py

src/dvclive/utils.py

src/dvclive/live.py

src/dvclive/dvc.py

src/dvclive/utils.py

src/dvclive/dvc.py

dberenbaum · 2023-04-18T15:07:35Z

src/dvclive/dvc.py

I think we can drop find_unused_name. If users are worried about conflicts, they can explicitly set a name.

I included it because it was explicitly mentioned as a requirement in our last sync with @aguschin and @omesser .
I am happy to go ahead and remove code if there are no strong objections

dberenbaum · 2023-04-18T15:11:35Z

Sorry for so many iterations here @daavoo. The blocker left for me is what to do with attrs if there isn't type=model. IMO we should write any attrs to artifacts. It's simple and even if it doesn't do much now, neither does anything else we do with those attrs (at least it could be useful in the future and an artifact registry would work with the existing artifacts written to artifacts).

daavoo · 2023-04-18T17:22:30Z

Sorry for so many iterations here @daavoo. The blocker left for me is what to do with attrs if there isn't type=model. IMO we should write any attrs to artifacts. It's simple and even if it doesn't do much now, neither does anything else we do with those attrs (at least it could be useful in the future and an artifact registry would work with the existing artifacts written to artifacts).

Pushed a new version

tests/test_log_artifact.py

- Update `make_dvcyaml` to write `artifacts` section. - Extend `log_artifact` to accept `type`, `name`, `desc`, `labels`, `meta`.

daavoo · 2023-04-21T10:46:40Z

Don't see much point in merging until we can test end-to-end against Studio.

aguschin · 2023-04-21T13:14:52Z

@daavoo, looks like it will be working, but if you want me, I can test this against what I have now. WDYT?

daavoo · 2023-04-21T13:32:11Z

@daavoo, looks like it will be working, but if you want me, I can test this against what I have now. WDYT?

Sounds good, what just saying that we can wait for merge and release until it works with Studio prod

dberenbaum · 2023-04-21T15:04:35Z

Can we merge and push people to start using it now, letting them know what's coming? We have people today asking to get started with the model registry. I'd like to tell them to use this and their models will appear once Studio is updated. Otherwise, I can only tell them do it the old way but it will break soon, or they can't start using it at all.

daavoo linked an issue Apr 11, 2023 that may be closed by this pull request

make_dvcyaml: Create artifacts section #519

Closed

daavoo self-assigned this Apr 11, 2023

daavoo added A: dvcyaml Area: `live.make_dvcyaml` A: log_artifact Area: `live.log_artifact` labels Apr 11, 2023

daavoo requested review from aguschin and dberenbaum April 11, 2023 16:54

daavoo commented Apr 11, 2023

View reviewed changes

daavoo changed the title ~~Support artfiacts section~~ Support artifacts section Apr 11, 2023

dberenbaum reviewed Apr 11, 2023

View reviewed changes

src/dvclive/live.py Outdated Show resolved Hide resolved

daavoo force-pushed the 519-make_dvcyaml-create-artifacts-section branch 2 times, most recently from 40f8062 to a935d89 Compare April 11, 2023 20:00

This comment was marked as outdated.

Sign in to view

daavoo force-pushed the 519-make_dvcyaml-create-artifacts-section branch from a935d89 to d942cd9 Compare April 11, 2023 20:09

daavoo requested a review from dberenbaum April 12, 2023 08:24

This comment was marked as outdated.

Sign in to view

aguschin approved these changes Apr 17, 2023

View reviewed changes

src/dvclive/live.py Outdated Show resolved Hide resolved

src/dvclive/utils.py Outdated Show resolved Hide resolved

daavoo force-pushed the 519-make_dvcyaml-create-artifacts-section branch from d942cd9 to d161ce6 Compare April 17, 2023 11:03

This comment was marked as outdated.

Sign in to view

daavoo force-pushed the 519-make_dvcyaml-create-artifacts-section branch from d161ce6 to b27d978 Compare April 17, 2023 11:04

dberenbaum reviewed Apr 17, 2023

View reviewed changes

src/dvclive/live.py Outdated Show resolved Hide resolved

dberenbaum reviewed Apr 17, 2023

View reviewed changes

src/dvclive/dvc.py Outdated Show resolved Hide resolved

dberenbaum reviewed Apr 17, 2023

View reviewed changes

src/dvclive/utils.py Outdated Show resolved Hide resolved

daavoo force-pushed the 519-make_dvcyaml-create-artifacts-section branch from b27d978 to d51938f Compare April 18, 2023 10:38

daavoo requested a review from dberenbaum April 18, 2023 10:40

dberenbaum reviewed Apr 18, 2023

View reviewed changes

src/dvclive/dvc.py Outdated Show resolved Hide resolved

dberenbaum reviewed Apr 18, 2023

View reviewed changes

src/dvclive/dvc.py Outdated Show resolved Hide resolved

dberenbaum reviewed Apr 18, 2023

View reviewed changes

daavoo force-pushed the 519-make_dvcyaml-create-artifacts-section branch from d51938f to 4435d07 Compare April 18, 2023 17:20

daavoo requested a review from dberenbaum April 18, 2023 17:21

dberenbaum approved these changes Apr 18, 2023

View reviewed changes

daavoo commented Apr 20, 2023

View reviewed changes

tests/test_log_artifact.py Outdated Show resolved Hide resolved

Support artifacts section.

9231ec8

- Update `make_dvcyaml` to write `artifacts` section. - Extend `log_artifact` to accept `type`, `name`, `desc`, `labels`, `meta`.

daavoo force-pushed the 519-make_dvcyaml-create-artifacts-section branch from 4435d07 to 9231ec8 Compare April 21, 2023 10:45

daavoo added the Blocked Blocked by external requirements label Apr 21, 2023

daavoo removed the Blocked Blocked by external requirements label Apr 21, 2023

daavoo merged commit a4df3a9 into main Apr 21, 2023

daavoo deleted the 519-make_dvcyaml-create-artifacts-section branch April 21, 2023 15:08

Support artifacts section #521

Support artifacts section #521

Uh oh!

Conversation

daavoo commented Apr 11, 2023

Uh oh!

daavoo left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Apr 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

dberenbaum commented Apr 11, 2023

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

aguschin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dberenbaum Apr 18, 2023

Choose a reason for hiding this comment

Uh oh!

daavoo Apr 18, 2023

Choose a reason for hiding this comment

Uh oh!

dberenbaum commented Apr 18, 2023

Uh oh!

daavoo commented Apr 18, 2023

Uh oh!

Uh oh!

daavoo commented Apr 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aguschin commented Apr 21, 2023

Uh oh!

daavoo commented Apr 21, 2023

Uh oh!

dberenbaum commented Apr 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Support `artifacts` section #521

Support `artifacts` section #521

codecov-commenter commented Apr 11, 2023 •

edited

Loading

daavoo commented Apr 21, 2023 •

edited

Loading