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

update docs for v0.10.0 #205

Merged

Conversation

alexagriffith
Copy link
Contributor

@alexagriffith alexagriffith commented Dec 1, 2022

#204

This PR addresses some of the goals in #204 to update and improve our documentation.
These changes are the MVP needed to get version v0.10.0 out.

This PR addresses the following

  • updating v1/v2 dataplane docs and structure (still need to add examples)
  • update custom model example with headers input
  • basic metrics documentation for prometheus & qpext changes

@netlify
Copy link

netlify bot commented Dec 1, 2022

Deploy Preview for elastic-nobel-0aef7a ready!

Name Link
🔨 Latest commit 9346498
🔍 Latest deploy log https://app.netlify.com/sites/elastic-nobel-0aef7a/deploys/63c812c673ce0c0009ad400f
😎 Deploy Preview https://deploy-preview-205--elastic-nobel-0aef7a.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@@ -42,24 +42,24 @@ def image_transform(instance):

# for REST predictor the preprocess handler converts to input dict to the v1 REST protocol dict
class ImageTransformer(kserve.Model):
def __init__(self, name: str, predictor_host: str):
def __init__(self, name: str, predictor_host: str, headers: Dict[str, str] = None):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

does init also need self.ready? @yuzisun
looking at this for reference - it isn't exactly the same code though... we should probably test this still works soon https://github.com/kserve/kserve/blob/master/python/custom_transformer/model.py

Copy link
Member

Choose a reason for hiding this comment

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

ye we need self.ready, the code snippet is just for demonstrating purpose to show how to override preprocess/postprocess in transformer for REST/gRPC case, the reference points to the actual working example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok makes sense, I can add it to avoid confusion?

@alexagriffith alexagriffith force-pushed the alexagriffith/update_dataplane_docs branch from 63ea55c to d0966b9 Compare December 21, 2022 22:32
@alexagriffith alexagriffith marked this pull request as ready for review December 21, 2022 22:35
@alexagriffith alexagriffith force-pushed the alexagriffith/update_dataplane_docs branch 5 times, most recently from ae2c6db to cb5484d Compare December 21, 2022 23:35
docs/modelserving/data_plane/data_plane.md Outdated Show resolved Hide resolved
* V2 added Server Readiness/Liveness/Metadata endpoints
* V2 endpoint paths contain `/` instead of `:`
* V2 renamed `:predict` endpoint to `/infer`
* V2 allows for model versions in the path
Copy link
Member

Choose a reason for hiding this comment

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

model version is optional, it is up to the model server if it wants to implement it and you can achieve model versioning in different ways e.g external to the model server.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I guess the point I was adding here was that it allows you to add model version in path, which v1 did not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is there something I should change or add here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe path is ambiguous. I meant request path

docs/modelserving/data_plane/v1_protocol.md Outdated Show resolved Hide resolved
docs/modelserving/observability/prometheus_metrics.md Outdated Show resolved Hide resolved
@alexagriffith alexagriffith force-pushed the alexagriffith/update_dataplane_docs branch 2 times, most recently from 3298774 to 78dbe24 Compare December 22, 2022 00:25
@@ -1,5 +1,5 @@
# Deploy InferenceService with Alternative Networking Layer
KServe v0.9 and prior versions create the top level `Istio Virtual Service` for routing to `InferenceService` components based on the virtual host or path based routing.
KServe creates the top level `Istio Virtual Service` for routing to `InferenceService` components based on the virtual host or path based routing.
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 seems like saying 0.9 and prior is kind of not useful right? means every version. and it doesn't change with 0.10. so that we dont have to update this with each version update, we can just say kserve creates until that changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unless im reading this wrong. the next sentence is that "Now kserve provides an option..." but kserve is at v0.9 so im confused a little by what this means.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should it maybe say "Starting with v0.9, Kserve provides an option...."

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with above change. And replacing "Now" with "Starting with v0.9" makes it clearer too when the option to disable was provided.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

im not 100% sure this started with v0.9, @yuzisun can you confirm?

@alexagriffith alexagriffith changed the title Alexagriffith/update dataplane docs update docs for v0.10.0 Dec 22, 2022
@alexagriffith alexagriffith force-pushed the alexagriffith/update_dataplane_docs branch from 47b7399 to af604e3 Compare January 10, 2023 21:24
@alexagriffith alexagriffith requested review from yuzisun and removed request for theofpa and xfu83 January 10, 2023 21:24

If needed, this value can be overridden in the InferenceService YAML.

To enable prometheus metrics, add the annotation `serving.kserve.io/enable-prometheus-scraping` to the InferenceService YAML.
Copy link
Member

Choose a reason for hiding this comment

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

There is also a global config to turn on the scraping without adding this to each inference service yaml right ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the info for that is in the docs linked below I can add another link here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we expect user to patch the config map themselves right?

@alexagriffith alexagriffith force-pushed the alexagriffith/update_dataplane_docs branch 2 times, most recently from 99d2048 to 7993db1 Compare January 17, 2023 18:32
Copy link
Contributor

@tessapham tessapham left a comment

Choose a reason for hiding this comment

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

This looks great! The doc is clear enough to me who don't know much about KServe. Only have a few comments on the V2 API.

@@ -1,5 +1,5 @@
# Deploy InferenceService with Alternative Networking Layer
KServe v0.9 and prior versions create the top level `Istio Virtual Service` for routing to `InferenceService` components based on the virtual host or path based routing.
KServe creates the top level `Istio Virtual Service` for routing to `InferenceService` components based on the virtual host or path based routing.
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with above change. And replacing "Now" with "Starting with v0.9" makes it clearer too when the option to disable was provided.

docs/modelserving/data_plane/v1_protocol.md Outdated Show resolved Hide resolved

GET v2
<!-- // TODO: add example with -d inputs. -->
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 still need an example here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can, im unsure right now of the best way to add these..

docs/modelserving/data_plane/v2_protocol.md Show resolved Hide resolved
docs/modelserving/data_plane/v2_protocol.md Outdated Show resolved Hide resolved
docs/modelserving/data_plane/v2_protocol.md Outdated Show resolved Hide resolved
docs/modelserving/data_plane/v2_protocol.md Outdated Show resolved Hide resolved
docs/modelserving/data_plane/v2_protocol.md Outdated Show resolved Hide resolved
docs/modelserving/data_plane/v2_protocol.md Outdated Show resolved Hide resolved
docs/modelserving/data_plane/v2_protocol.md Outdated Show resolved Hide resolved
@alexagriffith alexagriffith force-pushed the alexagriffith/update_dataplane_docs branch 2 times, most recently from f4dbc55 to 7831ed6 Compare January 18, 2023 15:38
Signed-off-by: agriffith50 <[email protected]>

Signed-off-by: agriffith50 <[email protected]>
Signed-off-by: agriffith50 <[email protected]>
Signed-off-by: agriffith50 <[email protected]>
Signed-off-by: agriffith50 <[email protected]>
Signed-off-by: agriffith50 <[email protected]>
Signed-off-by: agriffith50 <[email protected]>
Signed-off-by: agriffith50 <[email protected]>
Signed-off-by: agriffith50 <[email protected]>
@alexagriffith alexagriffith force-pushed the alexagriffith/update_dataplane_docs branch from 7831ed6 to 9346498 Compare January 18, 2023 15:39
@yuzisun
Copy link
Member

yuzisun commented Jan 21, 2023

Awesome work! @alexagriffith

/lgtm
/approve

@kserve-oss-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexagriffith, yuzisun

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kserve-oss-bot kserve-oss-bot merged commit 34ddb0a into kserve:main Jan 21, 2023
alexagriffith added a commit to alexagriffith/website that referenced this pull request Jan 26, 2023
* clarify runtimes.txt usage in custom model

kserve#180
Signed-off-by: agriffith50 <[email protected]>

* updating docs to reflect v0.10
Signed-off-by: agriffith50 <[email protected]>

Signed-off-by: agriffith50 <[email protected]>

* update wording ab model version

Signed-off-by: agriffith50 <[email protected]>

* add model ready

Signed-off-by: agriffith50 <[email protected]>

* update serving runtime table

Signed-off-by: agriffith50 <[email protected]>

* add mlfow to runtime table

Signed-off-by: agriffith50 <[email protected]>

* update dataplane md

Signed-off-by: agriffith50 <[email protected]>

* update v1

Signed-off-by: agriffith50 <[email protected]>

* fix links

Signed-off-by: agriffith50 <[email protected]>

* resolving pr comments

Signed-off-by: agriffith50 <[email protected]>

Signed-off-by: agriffith50 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants