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

fix(docs): Change V2 Inference Protocol to OIP #5297

Merged
merged 4 commits into from
Feb 8, 2024

Conversation

paulb-seldon
Copy link
Contributor

What this PR does / why we need it: Change reference from V2 Inference Protocol to OIP

Copy link
Contributor

@jesse-c jesse-c left a comment

Choose a reason for hiding this comment

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

LGTM aside from the 1 comment.

Comment on lines 11 to 13
- Health endpoints, to assess liveness and readiness of your model.
- Inference endpoints, to interact with your model.
- Metadata endpoints, to query your model metadata (e.g. expected inputs, expected
- Assess liveness and readiness of your model.
- Interact with your model.
- Query your model metadata (e.g. expected inputs, expected
Copy link
Contributor

@jesse-c jesse-c Feb 8, 2024

Choose a reason for hiding this comment

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

thought: I think these changes make them more vague as to what the endpoints are about. I'd keep the "sections" (Health, Inference, and Metadata).

KServe uses them as "sections" too, e.g. https://kserve.github.io/website/0.10/modelserving/data_plane/v2_protocol/#inference.

Perhaps instead it could be changed to be something like:

  • Inference: Interact with your model

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea it was more of a grammar thing that bothered me initially, but have updated to add categories back in

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 it may have been the grammar! That comma was a bit ugly. Thank you for adding the sections back. My last comment on it would be that for an inference protocol, we now don't mention inference at all in the partial list of endpoints 😅

Copy link
Contributor

@jesse-c jesse-c left a comment

Choose a reason for hiding this comment

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

LGTM

@paulb-seldon paulb-seldon merged commit 33dc760 into master Feb 8, 2024
12 of 13 checks passed
@paulb-seldon paulb-seldon deleted the paulb-seldon-api-docs branch February 8, 2024 11:29
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.

2 participants