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

MR docs update #4544

Merged
merged 17 commits into from
May 22, 2023
Merged

MR docs update #4544

merged 17 commits into from
May 22, 2023

Conversation

aguschin
Copy link
Contributor

@aguschin aguschin commented May 16, 2023

implements a part of #4423

  • I used links to external images here, so I should make them stored with DVC.

@aguschin aguschin requested a review from dberenbaum May 16, 2023 17:55
@aguschin aguschin self-assigned this May 16, 2023
@aguschin
Copy link
Contributor Author

@dberenbaum, got distracted by other things, updated one page here only today. PTAL, it should be quick enough, and let's align on expectations there.

@shcheklein shcheklein temporarily deployed to dvc-org-new-model-regis-o8axdg May 16, 2023 17:59 Inactive
@github-actions
Copy link
Contributor

github-actions bot commented May 16, 2023

Link Check Report

There were no links to check!

@dberenbaum
Copy link
Contributor

@dberenbaum, got distracted by other things, updated one page here only today. PTAL, it should be quick enough, and let's align on expectations there.

Sounds goo, @aguschin. Let's not make it close #4423 in that case.

@shcheklein shcheklein temporarily deployed to dvc-org-new-model-regis-o8axdg May 17, 2023 11:37 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-new-model-regis-o8axdg May 17, 2023 11:41 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-new-model-regis-o8axdg May 17, 2023 17:36 Inactive
@aguschin
Copy link
Contributor Author

@dberenbaum, I've took into account your review and updated MR use case, and Studio's MR User Guide. It's WIP, so I'll be refining. Please TAL, and let's align on the changes again.

Few things to mention:

  • I didn't really try to shorten long detailed instructions you're mentioning, since I don't want to really throw them away. Let's maybe convert them into notes (pop up when you hover over them) or something?
  • I didn't address "add separate sections for adding from GUI/Python/CLI": too early right now I think + copy-pasting instructions from other pages would make this duplicating. Not sure it worth it for the first iteration at least.

@dberenbaum
Copy link
Contributor

Thanks @aguschin! Can we separate the PRs (one for the use case and a follow-up for the user guide)?

@shcheklein shcheklein temporarily deployed to dvc-org-new-model-regis-o8axdg May 18, 2023 09:16 Inactive
@aguschin aguschin mentioned this pull request May 18, 2023
@shcheklein shcheklein temporarily deployed to dvc-org-new-model-regis-o8axdg May 18, 2023 09:23 Inactive
@aguschin
Copy link
Contributor Author

aguschin commented May 18, 2023

@dberenbaum, I moved Use case page changes from this PR to #4548

Let's review both and make them mergeable since @shcheklein promised to a customer we're going to release new MR this week. We can refine content later I think.

@shcheklein shcheklein temporarily deployed to dvc-org-new-model-regis-o8axdg May 18, 2023 18:29 Inactive
Comment on lines +23 to +26
3. Enter the path to `dvc.yaml` the model will be added to. Adding your model to
non-root `dvc.yaml` can be helpful if you develop this ML model in a specific
subfolder or if this repo is a
[monorepo](/doc/studio/user-guide/projects-and-experiments/configure-a-project#monorepo).
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this handled already at the project level? Or are we adding an additional option to pick a subfolder when you add a model?

Copy link
Contributor Author

@aguschin aguschin May 22, 2023

Choose a reason for hiding this comment

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

Yes, Now in "add a model" user select from views => he's selecting repo + subdir

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there anywhere with context about this decision? Why did we decouple it from the project?

Comment on lines 52 to 60
9. At this point, the new model appears in the models dashboard.

9. In your Git repository, you will find that an entry for the new model has
been created in the `dvc.yaml` file in the repository's root. If you had
committed to a new branch, a new pull request (or merge request in the case
of GitLab) will also have been created to merge the new branch into the base
branch.
10. If you had added a model from a cloud storage, the following will also
10. In your Git repository, you will find that an entry for the new model has
been created in the `dvc.yaml` file in the repository's root. If you had
committed to a new branch, a new pull request (or merge request in the case
of GitLab) will also have been created to merge the new branch into the base
branch.
11. If you had added a model from a cloud storage, the following will also
happen before the commit is created:
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: I don't think these are actual steps, and it makes this list longer than it needs to be. Can we take out the numbering and leave these in as an explanation of what you see after you complete these steps?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for changing this @aguschin! Again, this is minor and not a blocker, but you could do the same for the register and assign pages (they include steps that are really about explaining what you see afterwards).

@dberenbaum
Copy link
Contributor

Thanks @aguschin! I left a bunch of comments, but I don't think there's anything that's so critical that it has to block the PR, so you and @shcheklein can decide when it's in good enough shape to merge.

@shcheklein shcheklein temporarily deployed to dvc-org-new-model-regis-o8axdg May 22, 2023 12:00 Inactive
Co-authored-by: Dave Berenbaum <dave@iterative.ai>
@restyled-io restyled-io bot mentioned this pull request May 22, 2023
@shcheklein shcheklein temporarily deployed to dvc-org-new-model-regis-o8axdg May 22, 2023 12:26 Inactive
@shcheklein shcheklein temporarily deployed to dvc-org-new-model-regis-o8axdg May 22, 2023 12:48 Inactive
@aguschin
Copy link
Contributor Author

@dberenbaum thanks for the feedback! Made changes, hopefully it's all good now. I have a single TODO item on my own (store images with dvc), but otherwise we can merge this soon. Let's iterate and get this approved :)

following ways:

1. Log your model during the training process using [dvclive].
2. Edit `dvc.yaml` directly.
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
2. Edit `dvc.yaml` directly.
2. Edit [`dvc.yaml`](/doc/user-guide/project-structure/dvcyaml-files#artifacts) directly.

@dberenbaum
Copy link
Contributor

One more minor, non-blocking suggestion that isn't directly related to these changes. There are a couple places where it looks like we are focusing too much on GTO CLI over Studio:

You can register versions using the [`gto` CLI]. To register versions using
Iterative Studio, watch this tutorial video or read on below:

You can assign stages using the [`gto` CLI]. To assign stages using Iterative
Studio, watch this tutorial video or read on below:

In both places, we already mention in the intro that Studio relies on GTO to do these actions, so I think it's already implied that you can use the GTO CLI. Saying it again in these paragraphs makes it sound to me like we recommend the CLI over Studio, but I think it's actually preferable to use Studio unless they have some complex scenario that Studio can't handle.

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 now @aguschin!

Looks good, thanks for making all those changes! Nothing is a blocker as I mentioned before, so I am approving, but I left a few comments for minor cleanup if you have a chance.

@shcheklein shcheklein temporarily deployed to dvc-org-new-model-regis-o8axdg May 22, 2023 16:48 Inactive
@shcheklein
Copy link
Member

@aguschin @dberenbaum what are the blockers here? let's publish it please?

@dberenbaum dberenbaum merged commit 70be7b0 into main May 22, 2023
3 of 4 checks passed
@dberenbaum dberenbaum deleted the new-model-registry branch May 22, 2023 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants