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

add deployment to k8s docs #183

Merged
merged 8 commits into from
Oct 21, 2022
Merged

add deployment to k8s docs #183

merged 8 commits into from
Oct 21, 2022

Conversation

madhur-tandon
Copy link
Contributor

No description provided.

@shcheklein shcheklein temporarily deployed to mlem-ai-k8s-docs-ehqztvkipg9ch September 20, 2022 15:23 Inactive
@github-actions
Copy link

github-actions bot commented Sep 20, 2022 •
edited
Loading

9926b74

Link Check Report

13/169 links failed.

CML watermark

@shcheklein shcheklein temporarily deployed to mlem-ai-k8s-docs-ehqztvkipg9ch September 20, 2022 15:31 Inactive
@madhur-tandon madhur-tandon self-assigned this Sep 26, 2022
jorgeorpinel
jorgeorpinel reviewed Sep 28, 2022
View reviewed changes
Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

In general the content seems to be appropriate but perhaps too long. I have several questions/doubts/recommendations about the structure though.

But I can't really review this before #179 (review) (and other pending review/comments) are addressed. Also noting that the parent PR this is nested into (#182, in itself nested) is still a draft (also with ongoing reviews)

@madhur-tandon
Copy link
Contributor Author

Sure @jorgeorpinel Let me know and I will change the structure.

@jorgeorpinel jorgeorpinel added A: docs Area: user documentation (gatsby-theme-iterative) C: user-guide Content of /doc/user-guide labels Oct 3, 2022
@jorgeorpinel
Copy link
Contributor

I think we need to resolve previous PRs first. The structure discussions may happen in those. But lmk if you have a different plan of action please.

@aguschin
Copy link
Contributor

aguschin commented Oct 5, 2022 •
edited
Loading

@jorgeorpinel, please see #188
we're going to close #179 and #182 in favor of #188
if you're ok with the structure used in #188, you can start reviewing this PR.

@jorgeorpinel

This comment was marked as resolved.

Sign in to view
@aguschin

This comment was marked as resolved.

Sign in to view
@shcheklein shcheklein temporarily deployed to mlem-ai-k8s-docs-ehqztvkipg9ch October 11, 2022 09:43 Inactive
@madhur-tandon madhur-tandon changed the base branch from feature/sagemaker to new-docs-structure October 11, 2022 09:44
@madhur-tandon

This comment was marked as resolved.

Sign in to view
@aguschin
Copy link
Contributor

@jorgeorpinel, the PR waits for your review I believe)

@madhur-tandon madhur-tandon mentioned this pull request Oct 13, 2022
Merged
@shcheklein shcheklein temporarily deployed to mlem-ai-k8s-docs-ehqztvkipg9ch October 13, 2022 10:14 Inactive
aguschin
aguschin reviewed Oct 13, 2022
View reviewed changes
content/docs/user-guide/deploying/kubernetes.md Outdated
Comment on lines 3 to 6
Deploy a model to a kubernetes cluster exposing its prediction endpoints through
a service.
Copy link
Contributor

@aguschin aguschin Oct 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The same idea as in "requirements" PR: let's start with use case (e.g. something like "To serve models in production in a scalable and failure-safe way you need something more than Heroku") and explain what K8s is (e.g. "Kubernetes (or K8s) is a ..."). + link to k8s docs or website

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I am not sure. This is User Guide and not Use Cases. We already have a separate section for Use Cases. There is a subtle difference between them. IMO, for user guides, the user is already familiar with the tool etc. while in use cases, an introduction and context building is needed. So either we do the above and move it to Use Cases or let it be as it is. Maybe @jorgeorpinel can suggest or correct me if I am wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

let's start with use case (e.g. something like "To serve models in production

I agree its good to provide motivation in the user guide in general. Starting with "do this, do that" is better suited for references. (Refer to types of docs if needed))

We already have a separate section for Use Cases

Yeah but they're all "wrong" haha (not wrong, just temporary). See #192.

for user guides, the user is already familiar with the tool etc

So the User Guide section was originally copied from dvc.org/doc/user-guide/overview and in that case, it's where we explain the most (including lots of motivation), and some times even give deep implementation details. So it depends what you guys want to get from the UG I suppose. We could even call it something else.

We don't need to copy DVC docs structure, but from that experience, the longer format guides get the least traffic so a) not a high priority and b) we assume it's users who are familiar but only with the basics and want to learn more (to truly understand what's going on), which is why I would include motivation anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@jorgeorpinel
Copy link
Contributor

the PR waits for your review I believe

Sorry, still a bit confused since this is nested on #188 which is still a draft. I prioritized reviewing that one (see #188 (comment)).

jorgeorpinel
jorgeorpinel reviewed Oct 20, 2022
View reviewed changes
content/docs/user-guide/deploying/kubernetes.md Outdated
Comment on lines 281 to 283
### A note about NodePort Service

<admon type="info">
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
### A note about NodePort Service
<admon type="info">
<admon type="info" title="A note about NodePort Service">

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

jorgeorpinel
jorgeorpinel reviewed Oct 20, 2022
View reviewed changes
content/docs/user-guide/deploying/kubernetes.md Outdated

## Swapping the model in deployment

If you want to change the model that is currently under deployment, simply run
Copy link
Contributor

@jorgeorpinel jorgeorpinel Oct 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

See #114 wrt (unintentionally) condescending language.

Suggested change
If you want to change the model that is currently under deployment, simply run
If you want to change the model that is currently under deployment, run

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@shcheklein shcheklein temporarily deployed to mlem-ai-k8s-docs-ehqztvkipg9ch October 21, 2022 07:56 Inactive
@shcheklein shcheklein temporarily deployed to mlem-ai-k8s-docs-ehqztvkipg9ch October 21, 2022 08:00 Inactive
@shcheklein shcheklein temporarily deployed to mlem-ai-k8s-docs-ehqztvkipg9ch October 21, 2022 08:04 Inactive
@shcheklein shcheklein temporarily deployed to mlem-ai-k8s-docs-ehqztvkipg9ch October 21, 2022 08:19 Inactive
aguschin
aguschin approved these changes Oct 21, 2022
View reviewed changes
Copy link
Contributor

@aguschin aguschin left a comment

Choose a reason for hiding this comment

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

Thanks @madhur-tandon, the overall page is looking good to me. Great job! Please do the last fixes if there are some, and you can merge it! 🚀

@madhur-tandon
Copy link
Contributor Author

Please do the last fixes if there are some, and you can merge it! 🚀

Can you just maybe look if removal of --conf is correct? aka the commit here: fb0f477

@shcheklein shcheklein temporarily deployed to mlem-ai-k8s-docs-ehqztvkipg9ch October 21, 2022 10:20 Inactive
@madhur-tandon madhur-tandon merged commit f6c94e0 into new-docs-structure Oct 21, 2022
@madhur-tandon madhur-tandon deleted the k8s-docs branch October 21, 2022 10:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jorgeorpinel jorgeorpinel jorgeorpinel left review comments

@aguschin aguschin aguschin approved these changes

@mike0sv mike0sv Awaiting requested review from mike0sv

Assignees

@madhur-tandon madhur-tandon

Labels
A: docs Area: user documentation (gatsby-theme-iterative) C: user-guide Content of /doc/user-guide
Projects
No open projects
MLEM + GTO
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@madhur-tandon @jorgeorpinel @aguschin @shcheklein