Closed tarilabs closed 1 month ago
@tarilabs thanks a lot for getting this started! I went through though the current version, I think it's a good first version.
First of all a comment on timeline: we do not have to rush this in time for feature freeze. We have a couple more weeks to finetune and improve the docs. So let's keep this PR running until we have a good version.
Then, I'll refrain from commenting specific sections and instead provide some higher level feedback:
Getting Started: I would split up the getting started into two guides:
This is important because installation is a one off thing (done by the Kubeflow admin) whereas the getting started guide targets ML practitioners who just use the platform
I know it's a lot. Great documentation takes a lot of time and effort. Let's try and do these things incrementally.
I hope that @diegolovison and @hbelmiro can also help out a little bit here. Looking forward to hearing your thoughts!
First of all a comment on timeline: we do not have to rush this in time for feature freeze. We have a couple more weeks to finetune and improve the docs. So let's keep this PR running until we have a good version.
ack, and agreed
- We should also document a list of use cases and jobs that the model registry solves within the Kubeflow ecosystem
yes, this was discussed in the KF community-calls, in the KF MR bi-weekly Josh already suggested the same, we're iterating on a google doc already which will be "poured" in content into this PR as soon as the copy is ready
- Getting Started: I would split up the getting started into two guides
noted, will do
👉 done with 2f567ae
- I'd also love to see an architecture diagram under a "reference" section.
noted, will do
👉 done with 3bbf750
Thanks for the feedback @StefanoFioravanzo ! 📥 ✍️
@tarilabs This is what I was talking about on Monday:
I took a random picture from the web and added labels by hand. But you get the idea. It would be nice if the MLOps loop that we present in the overview showed how the model registry contributes to the various MLOps stages. Especially when paired with the descriptions you added here https://github.com/kubeflow/website/pull/3698/commits/be3ebf8be6d62ed3e5b5871784c753e5cac96cfd
We could add a small icon that represents the MR presence within the stages with the label, to highlight that MR has a contribution in that stage (what I did in green)
We could add a small icon that represents the MR presence within the stages with the label, to highlight that MR has a contribution in that stage (what I did in green)
nice suggestion, thanks! Implemented on top of current diagram with: fca21205f6a58448839386dd7c0598a8fe722bec
nice suggestion, thanks! Implemented on top of current diagram with: https://github.com/kubeflow/website/commit/fca21205f6a58448839386dd7c0598a8fe722bec
@tarilabs but what do you think about my proposal of moving the MR labeling to each individual section, removing it from the middle? Just want to understand if you like the idea and the mock I sent above. If you do, we can work together on an editable space.
what do you think about my proposal of moving the MR labeling to each individual section, removing it from the middle? Just want to understand if you like the idea and the mock I sent above. If you do, we can work together on an editable space.
Removing a "single instance" of a Model Registry "box" and make a cloned "smaller boxes" repeated on the loop can arguably give the false impression there is no "single pane of glass". That is why I incorporated as much as possible from your original comment: https://github.com/kubeflow/website/pull/3698#issuecomment-2063330587 and adapted on top of existing work.
I'm not against iterating again on the diagram, but I would prefer moving forward with a "best candidate" (which imho is present now on this PR) and eventually iterate on it as a separate, focused PR: wdyt?
can arguably give the false impression there is no "single pane of glass".
I don't agree with this but let's not block on it now. Let's iterate on this after this PR!
Hey @tarilabs ! Is there something left before we can merge this one?
Hey @tarilabs ! Is there something left before we can merge this one?
@StefanoFioravanzo to me this can be merged as this thread shows most of the feedback was incorporated, either as-is, either as incorporation into previous work done as part of this PR. From my pov this PR can be merged following https://github.com/kubeflow/website/pull/3698#issuecomment-2071694592 And further iteration on diagrams, tutorial, etc could be done in later, separate and potentially in-parallels PR :)
Then let's go! 🚀 @andreyvelich can you approve?
@tarilabs @andreyvelich I see there are a few last comments to be addressed here. Can we quickly close the loop and get this merged?
@tarilabs @andreyvelich I see there are a few last comments to be addressed here. Can we quickly close the loop and get this merged?
yep thanks again for the feedback @andreyvelich , I addressed most of them, about the remaining kindly let me know if, as discussed previously in this thread, do you require them as part of this PR or could be follow-ed up in subsequent PRs (so as also to "parallelize" changes). Thank you!
@tarilabs @andreyvelich I see there are a few last comments to be addressed here. Can we quickly close the loop and get this merged?
yep thanks again for the feedback @andreyvelich , I addressed most of them, about the remaining kindly let me know if, as discussed previously in this thread, do you require them as part of this PR or could be follow-ed up in subsequent PRs (so as also to "parallelize" changes). Thank you!
Sure, we can incorporate them in a following PR. Do you want to address this change in the next PR: https://github.com/kubeflow/website/pull/3698#discussion_r1607313025 ?
Sure, we can incorporate them in a following PR. Do you want to address this change in the next PR: #3698 (comment) ?
unless anyone has any "blocker" or "veto" comments, since this PR has been open for a long time (March) and incorporated as many comments as possible in the time being (see thread), I'd be happy to have it merged, and parallelize any further comments/amendments in separate PRs (multiple).
Kindly let me know your guidance here, and once again thanks for all who contributed reviews!!
Absolutely, thanks again for your hard work @tarilabs! /lgtm /assign @jbottum for the approval
@andreyvelich: GitHub didn't allow me to assign the following users: for, the, approval.
Note that only kubeflow members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. For more information please see the contributor guide
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: jbottum
The full list of commands accepted by this bot can be found here.
The pull request process is described here
Opening this PR for early feedback.
As feedback received by Release Team during KF community meetings and Model Registry meetings, As identified in the Model Registry KF 1.9 tracker issue on request by Release Team,
➡️ Documentation is needed for Model Registry alpha on the KF website, as part of the KF 1.9 deliverables by Model Registry wg.
This PR focus on the following received requests:
This documentations is loosely based on the content presented with this demo.