-
Notifications
You must be signed in to change notification settings - Fork 394
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
Adding artifacts
section to docs
#4481
Conversation
@dberenbaum this is a first touch on docs - it updates some pages. If I'm not missing something, this is what already done (log_artifact, |
content/docs/dvclive/how-it-works.md
Outdated
[cache](/doc/start/data-management/data-versioning) the `model.pt` file with DVC | ||
and make Git ignore it. It will generate a `model.pt.dvc` metadata file, which | ||
can be tracked in Git and becomes part of the experiment. With this metadata | ||
file, you can [retrieve](/doc/start/data-management/data-versioning#retrieving) | ||
the versioned artifact from the Git commit. | ||
|
||
Passing `type="model"` or `type="data"` will it to `artifacts` section of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we are adding to artifacts.yaml
regardless of what other info gets passed, right @daavoo? As mentioned in the other comments, let's clarify what gets written to artifacts.yaml
and clarify that the model registry won't do anything yet but soon will work with type=model
artifacts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, You are right @dberenbaum
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice start @aguschin! Left some suggestions to clarify behavior in a few places.
Also added @tapadipti and @shcheklein as reviewers. I'd like for either them or some others to take a look since it's our first attempt to start explaining the revamped model registry. |
Co-authored-by: Dave Berenbaum <[email protected]>
@dberenbaum thanks for the helpful review! Processed that. I resolved all of your comments since they're addressed now. |
Co-authored-by: tapadipti <[email protected]>
content/docs/dvclive/how-it-works.md
Outdated
Passing `type="model"` or `type="data"` will add it to `artifacts` section of | ||
`dvc.yaml`, allowing DVC to understand what it is and show models in | ||
[Studio Model Registry](/doc/use-cases/model-registry). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still feels a little confusing to me because:
- We aren't currently working on any Studio functionality for
type=data
(let's only talk about plans for things that are at least in progress already). - It still sounds like it only adds to
artifacts
iftype=model
ortype=data
, which isn't true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, sorry - I thought I fixed that. Doing it now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, did that. It's ready to merge I believe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making those updates @aguschin! I left a couple follow-up comments. Once those are addressed, I think it's ready to merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good @aguschin !
I've added a few comments and suggestions, please take a look.
Still, IMO non are real blockers to merge - approving
If `Live` was initialized with `dvcyaml=True` (which is the default), it will | ||
add an [artifact](/doc/user-guide/project-structure/dvcyaml-files#artifacts) and | ||
all the metadata passed as arguments to corresponding `dvc.yaml`. Passing | ||
`type="model"` will mark it as a `model` for DVC and will make it appear in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... for DVC and will also make Studio Model Registry support it (coming soon!).
Same as above
Co-authored-by: Oded Messer <[email protected]>
let's merge, we'll have another chance to update this
Nice work, Thanks @aguschin 🙏 |
Yep, looks good, thanks @aguschin! |
adds first items in #4423