Skip to content
This repository has been archived by the owner on Oct 16, 2024. It is now read-only.

WIP: Docs draft for integration with DVC #323

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

WIP: Docs draft for integration with DVC #323

wants to merge 17 commits into from

Conversation

aguschin
Copy link
Contributor

@aguschin aguschin commented Mar 7, 2023

This is a draft for the docs - how this should look for a user after we integrate GTO artifacts.yaml part into DVC

https://mlem-ai-gto-dvc-nrmufqqihx13et.herokuapp.com/doc/gto/get-started-dvc/

cc @dberenbaum to check this out

@aguschin aguschin self-assigned this Mar 7, 2023
@shcheklein shcheklein temporarily deployed to mlem-ai-gto-dvc-nrmufqqihx13et March 7, 2023 09:35 Inactive
@github-actions
Copy link

github-actions bot commented Mar 7, 2023

Link Check Report

All 7 links passed!

@shcheklein shcheklein temporarily deployed to mlem-ai-gto-dvc-nrmufqqihx13et March 7, 2023 11:41 Inactive
@shcheklein shcheklein temporarily deployed to mlem-ai-gto-dvc-nrmufqqihx13et March 7, 2023 11:55 Inactive
@shcheklein shcheklein temporarily deployed to mlem-ai-gto-dvc-nrmufqqihx13et March 7, 2023 13:18 Inactive
@shcheklein shcheklein temporarily deployed to mlem-ai-gto-dvc-nrmufqqihx13et March 7, 2023 13:58 Inactive
@shcheklein shcheklein temporarily deployed to mlem-ai-gto-dvc-nrmufqqihx13et March 7, 2023 13:59 Inactive
@shcheklein shcheklein temporarily deployed to mlem-ai-gto-dvc-nrmufqqihx13et March 7, 2023 14:08 Inactive
Comment on lines 40 to 48
and make them shown as models in `dvc ls`:

```dvc
$ dvc ls --registry # add `--type model` to see models only
Path Name Type Labels Description
mymodel.pkl model
data.xml stackoverflow-dataset data data-registry,get-started imported code
data/data.xml another-dataset data data-registry,get-started imported
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Does Studio need this, or is solely to provide a CLI option to view the registry? I don't think the latter needs to be high priority unless I'm missing some use case where you need to access it from the CLI.

Not sure whether it can fit into dvc ls since the output is quite different (and potentially so are the arguments like --type). Need to think about whether we need this and where it can fit.

Copy link
Contributor

Choose a reason for hiding this comment

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

By the way, do we define an artifact as any output that has a type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

some use case where you need to access it from the CLI

If user would like to download a model locally, but not quite sure which one at the moment, he might want to see this. E.g. I don't remember the model name, but know labels or remember description. This OFC can be solved via Studio, and if we want to push users for that, that's also a decision.

Another use case would be if you're investigating a repo that's not familiar to you (let's say your team has few repos or you look at another team's repo). Again, if we want to make people go to Studio every time for this, it a valid workflow, but IMO it makes you leave CLI and do extra things which can't be inconvenient.

By the way, do we define an artifact as any output that has a type?

Either that, or any input/output file DVC keeps track of can be an artifact (without type in it's not defined). I think the latter is simpler and easier to convey. We can call it "file" instead of "artifact" I guess (if we're not going to introduce "compound" artifacts as we discussed before which I don't think is the case. Let's probably don't discuss this though, it's unrelated to this PR and not necessary at all now I believe).

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good. Not sure I see enough to make it a p1 yet. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

By the way, do we define an artifact as any output that has a type?

Either that, or any input/output file DVC keeps track of can be an artifact (without type in it's not defined). I think the latter is simpler and easier to convey.

It might also depend on the schema discussion below. If we have a model/registry/artifacts section of dvc.yaml, I guess we will only include what's specified there.

branch. You can keep track of the model's lineage by
[registering Semantic versions and promoting your models](/doc/gto/get-started)
(or other artifacts) to stages such as `dev` or `production` with GTO. GTO
operates by creating Git tags such as `[email protected]` or `mymodel#prod`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought the stage tags always had some number like mymodel#prod#1?

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 can be mymodel#prod as well. It's called "simple" Git tag format and it's not the default one. https://mlem.ai/doc/gto/user-guide/#git-tags-format

- id: gto
uses: iterative/gto-action@v1
with:
download: True # you can provide a specific destination path here instead of `True`
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you give an example? Will it download all artifacts in the repo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should download the artifact, e.g. it will run dvc get . mymodel --rev $GITHUB_REF for a Git tag [email protected].

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 docs to explain this.

Comment on lines 94 to 102
## Restricting which types are allowed

To specify which `type`s are allowed to be used, you can add the following to
your `.dvc/config`:

```
# .dvc/config
types: [model, data]
```
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 think it needs to be part of the initial scope? I know it was requested, but it doesn't feel strictly necessary yet.

Copy link
Contributor Author

@aguschin aguschin Mar 9, 2023

Choose a reason for hiding this comment

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

Not necessary. I can make a notes in the docs page for what's extra for now, if that makes sense.

## Seeing new model versions pushed with DVC experiments

After you run `dvc exp push` to push your experiment that updates your model,
you'll see a commit candidate to be registered:
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 think we should allow registration of unmerged experiments? Or maybe restrict what actions are available for them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. Don't have a strong opinion.
First, it's possible to do, so why not. We can also allow to click on "register", but then say something like "We advise to merge the experiment first" with buttons like "create a PR in GH" (default) and "register anyway".
We can prohibit registering (again, don't see a reason except for skipping polluting repo with dangling refs in a the workflow that requires users to merge experiments first).
We can delay answering this question for now I 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.

Btw, do we have an understanding how dvc exp push flow should look like on Studio's side? If that's still WIP, I guess we need to implement that first.

@dberenbaum
Copy link
Contributor

Looks good @aguschin! Just a few details to work out. Thanks!

live.log_artifact(artifact, "path", type="model")
```

This will make them appear in DVC Model Registry:
Copy link
Contributor

@omesser omesser Mar 9, 2023

Choose a reason for hiding this comment

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

Suggested change
This will make them appear in DVC Model Registry:
This will make them appear in [Studio Model Registry](https://dvc.org/doc/studio/user-guide/model-registry/what-is-a-model-registry):

(Fo now,. it's still called Studio and not DVC.Cloud or similar)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Overall, let's keep the review scope to the level of ideas and user experience. We don't even know if this will be a separate page in DVC docs, or maybe we integrate it with some other page.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah this is a nitpick 😉 but was hard for me to pass the opportunity and suggest

separately, address them by `name` in `dvc get`, and eventually, see them in DVC
Studio.

Let's start with marking an artifact as data or model.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Let's start with marking an artifact as data or model.
Let's start with marking a tracked artifact (file) as a `model`.

Personally, I don't think that "data" is a valid example for a type

Copy link
Contributor Author

@aguschin aguschin Mar 9, 2023

Choose a reason for hiding this comment

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

Why? I assumed Data Registry would show type: data once we implement it.

Copy link
Contributor

Choose a reason for hiding this comment

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

It carries 0 information though, right?
"data of type data", similar to "artifact of type artifact" means the same as not defining type at all. it's the most general thing there is (data even more than artifact maybe?), just super abstract. doubt users will use it that way

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Maybe it's dataset instead of data then? Or anyways, if after subtracting plots, metrics and models everything that's left (among DVC PL inputs and outputs) is dataset, then I guess it's redundant?

Copy link
Member

Choose a reason for hiding this comment

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

My 2cs. Data is not abstract for me (it's different vs model in my perception). But in DVC it's not needed. Any out w/o a specified type can be considered data.

I would personally try to simplify all of this - no multiple types initially. only models. We are prematurely generalizing this I think.

Choose a reason for hiding this comment

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

I think of the terms Data Registry, Model Registry, and Artifact Registry. I like having the type track to those names, so I like data.

Would love to keep the idea of data registry support around, haven't thought a lot about using gto for that, but certainly have thought about models and binaries.

I would love to use/try gto as an Artifact Registry for build artifacts - specifically compiled binaries. It might also be interesting to use as a Container Registry - there are lots of solutions in this space like Jfrog Artifactory, cloudsmith, GCP. If you wanted to be really fancy you could support some of those offerings as backends.

With all of that said I think prioritizing the Model Registry use case makes a lot of sense, a Data Registry would be my next priority. The solution seems like it may be general enough to support an Artifact Registry and Container Registry, might be worth doing some thinking about it and if it does make sense and there are advantages to keeping that in gto then keep that use case in mind while making the Model Registry experience awesome.

Choose a reason for hiding this comment

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

Trying to think of advantages of having gto also be a Container Registry and remembered that mlem can deploy docker containers, it would be nice to have the versioning of those container artifacts in gto and be able to reference with gto syntax.

Choose a reason for hiding this comment

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

If I might chip in as a user - I absolutely would like GTO to be used also to build a data(set) registry (together with DVC) and possibly a combined data(set) and model registry.

In fact, we intend to use GTO and DVC to build a dataset registry for one (fairly large) client in the coming weeks. By the way, while stuff like MLFlow is a viable alternative to the GTO+MLEM-based model registry (it does not have all of its features but has some others), I don't know any open source alternative to a GTO+DVC-based dataset registry (and general I've yet to see data versioning done better than with DVC)...which means it is a very good selling point IMO.

I like the idea of viewing GTO as a tool to build pretty much any artifact registry, though models and datasets are the most likely use-cases.

types: [model, data]
```

## Seeing new model versions pushed with DVC experiments
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
## Seeing new model versions pushed with DVC experiments
## Models and Experiments

Also, a question - is this an implicit behavior for artifacts with type: model specifically? or will there be similar side effects for any artifact with "type" defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This only updated MDP, so this is only for type: model for now. How did you assume this should work with other types?

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't, I'm a bit concerned for implicit behaviors, we should probably find a way to give the user control over what to do with which artifacts on exp push. wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What example of implicit behavior you have in mind? Like pushing a model that can be few GB in size? Not quite have specific examples in mind.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, auto-pushing, exactly. maybe auto-versioning as well in the future (could be useful if running in the pipeline CI-CD as part of release. generate model, push it, and assign a version using GTO

@@ -0,0 +1,115 @@
# Get Started DVC

Copy link
Contributor

@omesser omesser Mar 9, 2023

Choose a reason for hiding this comment

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

HL:
Suggest to add sub-headers / sections
And structure for this page will be something like this:

## Defining artifact types
... example of a model, defining in different ways
... mentioning that in a similar way you can define other types, e.g. `type: artifact` or similar

## Browsing Artifacts
.. studio
... dvc ls

## Additional artifact metadata
...

## Registered artifact versions
...
## Working with artifacts in CI
...
## Restricting which types are allowed
...

The same way you specify `type`, you can specify `description`, `labels` and
`name`. Defining human-readable `name` (should be unique) is useful when you
have complex folder structures or if you artifact can have different paths
during the project lifecycle.
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 here we need 1 (concise) example setting all relevant fields
e.g.

dvc add models/mymodel.pkl --name def-detector --type model --description "glass defect image classifier" --label "algo=cnn" --label "owner=aguschin" --label "project=prod-qual-002"

- data.xml
outs:
- mymodel.pkl:
type: model # like this
Copy link
Member

Choose a reason for hiding this comment

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

to make it symmetrical with plots, params, etc- why don't we make it:

models:
   mymodel.pkl

do we plan / need other artifact types? (data can be covered by outs I think)

Copy link
Contributor Author

@aguschin aguschin Mar 9, 2023

Choose a reason for hiding this comment

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

QQ: in this case, do you assume other fields can be specified like:

models:
   mymodel.pkl:
     name: particles-classifier
     description: best hits
     labels:
        - boson
        - higgs

?

For now it looks like type: model may be enough. I can introduce this models: section in this docs page OFC (and I'd like to do this TBH: artifacts.yaml is almost like it, which will make a transition easier for existing users with something like models: artifacts.yaml), but then we need to take into account how we deal with duplication/overwrites if we allow models to be specified in models: section and in outs (type: model) simultaneously.

We also have a user asking if we could allow this to have a separate file, that's another "+" for having a models: section.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking it might make more sense to have a dedicated section for registry with all metadata defined there, with artifact names, and the pipeline should only refer to items from there by names - otherwise it really clutters the pipeline mixing all those concerns.

Copy link
Contributor

Choose a reason for hiding this comment

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

A top-level dvc.yaml section means we need to do some more work on the DVC side instead of using what we have.

If we go this direction, the biggest question is whether to call it models: or make it generic like registry: or artifacts:. The appeal of models is that we start small, but I worry it doesn't make much sense long-term because:

  1. There's no model-specific functionality in DVC now or in current plans.
  2. People already use DVC as a data registry, and it's inevitable people will want to log non-model artifacts.
  3. It provides flexibility for custom types (not sure if this is good or bad).

Copy link
Contributor Author

@aguschin aguschin Mar 10, 2023

Choose a reason for hiding this comment

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

Thanks for ideas. I've updated the docs page - please TAL.

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 the dvc.yaml schema can look like this:

# dvc.yaml
stages:
  train:
    cmd: python train.py
    deps:
      - data.xml
    outs:
      - models/mymodel.pkl
artifacts:
  - def-detector:
      path: models/mymodel.pkl
      type: model
      desc: glass defect image classifier
      labels:
        - algo=cnn
        - owner=aguschin
        - project=prod-qual-002
  - models/othermodel.pkl # If no path provided, use name as the path

This would reuse most of what we already have between:

  1. Existing metadata fields for type, desc, labels.
  2. Top-level fields we have for plots, metrics, and params.
  3. GTO artifacts.yaml (artifacts are in a dict format in artifacts.yaml but a list format above; we could support both to make it less error-prone, which we already do for top-level plots).

I think we can mostly leave alone everything already implemented and only add this new top-level section. I would suggest that only things listed in artifacts: (and in artifacts.yaml if we keep it) show up in the Studio registry. Until we get to a non-model registry, we could hide artifacts without type: model.

Copy link
Member

Choose a reason for hiding this comment

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

It's in the right direction,I like it. by I would make it even simpler initially and do the first class citizen top level models section. I don't see a reason to overgeneralize everything at the moment tbh.

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 there are good reasons to generalize types. Besides those mentioned above in this thread and by users, we already support type in DVC metadata and GTO artifacts.yaml, so it fits with what we have already. Is there a good enough reason to rebuild to make it model-specific?


If you're producing your models in DVC pipeline, you'll need to add
`type: model` to `dvc.yaml` instead:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now I think there is a user case this change may not support that well. One of our prospects asked to allow a single file (let's say mymodel.pkl) to be referenced as several GTO models (e.g. model1 and model2 - these are names). Since moving to DVC makes path essential (instead of name), I don't see how that feature would fit here. 🤔

The motivation is to be able to promote model1 and model2 to different stages at different moments of time separately. To clarify, let's assume there are two populations mymodel.pkl should be applied for. You can create stages like populationA-prod, populationA-staging and populationB-prod, populationB-staging, if you have many populations, this would make things cumbersome. The solution was to introduce model1 (for populationA) and model2 (for populationB). That required this feature.

The only workaround I see now is to create a "mirror file" with cp mymodel.pkl mymodel-for-populationB.pkl in some DVC PL stage. Or keep this name:path mapping outside of DVC somehow. Do you see any other solutions guys? WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with @omesser that it's related the discussion above. Take a look at the top-level plots schema, where plots may be identified by either path or an arbitrary name. Feels like following a similar syntax may be best here.

@shcheklein shcheklein temporarily deployed to mlem-ai-gto-dvc-nrmufqqihx13et March 10, 2023 08:15 Inactive
Comment on lines 40 to 46
If you want this to be in a separate file (say, `artifacts.yaml`), you can tell
DVC to use it with:

```yaml
# dvc.yaml
registry: artifacts.yaml
```
Copy link
Contributor Author

@aguschin aguschin Mar 10, 2023

Choose a reason for hiding this comment

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

Extra for now, but was requested by users

Choose a reason for hiding this comment

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

Love this! I would love to do this with plots too.

@shcheklein shcheklein temporarily deployed to mlem-ai-gto-dvc-nrmufqqihx13et March 10, 2023 08:16 Inactive
Copy link

@shortcipher3 shortcipher3 left a comment

Choose a reason for hiding this comment

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

Overall I think this looks great.

separately, address them by `name` in `dvc get`, and eventually, see them in DVC
Studio.

Let's start with marking an artifact as data or model.

Choose a reason for hiding this comment

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

I think of the terms Data Registry, Model Registry, and Artifact Registry. I like having the type track to those names, so I like data.

Would love to keep the idea of data registry support around, haven't thought a lot about using gto for that, but certainly have thought about models and binaries.

I would love to use/try gto as an Artifact Registry for build artifacts - specifically compiled binaries. It might also be interesting to use as a Container Registry - there are lots of solutions in this space like Jfrog Artifactory, cloudsmith, GCP. If you wanted to be really fancy you could support some of those offerings as backends.

With all of that said I think prioritizing the Model Registry use case makes a lot of sense, a Data Registry would be my next priority. The solution seems like it may be general enough to support an Artifact Registry and Container Registry, might be worth doing some thinking about it and if it does make sense and there are advantages to keeping that in gto then keep that use case in mind while making the Model Registry experience awesome.

separately, address them by `name` in `dvc get`, and eventually, see them in DVC
Studio.

Let's start with marking an artifact as data or model.

Choose a reason for hiding this comment

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

Trying to think of advantages of having gto also be a Container Registry and remembered that mlem can deploy docker containers, it would be nice to have the versioning of those container artifacts in gto and be able to reference with gto syntax.

Comment on lines 40 to 46
If you want this to be in a separate file (say, `artifacts.yaml`), you can tell
DVC to use it with:

```yaml
# dvc.yaml
registry: artifacts.yaml
```

Choose a reason for hiding this comment

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

Love this! I would love to do this with plots too.

@shcheklein shcheklein temporarily deployed to mlem-ai-gto-dvc-nrmufqqihx13et March 21, 2023 10:36 Inactive
@shcheklein shcheklein temporarily deployed to mlem-ai-gto-dvc-nrmufqqihx13et March 21, 2023 10:40 Inactive
@shcheklein shcheklein temporarily deployed to mlem-ai-gto-dvc-nrmufqqihx13et March 31, 2023 06:22 Inactive
@shcheklein shcheklein temporarily deployed to mlem-ai-gto-dvc-nrmufqqihx13et March 31, 2023 06:32 Inactive
@omesser omesser self-requested a review June 4, 2023 13:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants