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.

Sign up for GitHub

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

Jump to bottom

Open Inference Protocol Implementation. #2609

Merged
merged 11 commits into from
Jan 24, 2024
Merged

Open Inference Protocol Implementation. #2609

merged 11 commits into from
Jan 24, 2024

Conversation

andyi2it
Copy link
Contributor

@andyi2it andyi2it commented Sep 21, 2023 •
edited
Loading

Description

Please read our CONTRIBUTING.md prior to creating your first pull request.

Please include a summary of the feature or issue being fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes #2373

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

Feature/Issue validation/testing

Please describe the Unit or Integration tests that you ran to verify your changes and relevant result summary. Provide instructions so it can be reproduced.
Please also list any relevant details for your test configuration.

  • Test A
    Logs for Test A

  • Test B
    Logs for Test B

Checklist:

  • Did you have fun?
  • Have you added tests that prove your fix is effective or that this feature works?
  • Has code been commented, particularly in hard-to-understand areas?
  • Have you made corresponding changes to the documentation?

@andyi2it andyi2it force-pushed the oip-impl branch 3 times, most recently from db0be27 to 6c967fd Compare October 11, 2023 05:53
@andyi2it andyi2it force-pushed the oip-impl branch 2 times, most recently from 7cc7579 to 5654872 Compare November 8, 2023 13:39
@andyi2it andyi2it marked this pull request as ready for review November 14, 2023 05:17
@andyi2it andyi2it force-pushed the oip-impl branch 2 times, most recently from 0c09ef4 to 19f6ab2 Compare November 16, 2023 06:23
@andyi2it andyi2it force-pushed the oip-impl branch 2 times, most recently from 206e190 to acf8910 Compare December 14, 2023 10:22
lxning
lxning reviewed Dec 18, 2023
View reviewed changes
Copy link
Collaborator

@lxning lxning left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution.

Could you add tests for the following endpoints?

  1. server and model health check.
  2. tensor input inference.

frontend/server/src/main/java/org/pytorch/serve/util/ConfigManager.java Outdated Show resolved Hide resolved
frontend/server/src/main/java/org/pytorch/serve/grpcimpl/OpenInferenceProtocolImpl.java Outdated Show resolved Hide resolved
frontend/server/src/main/java/org/pytorch/serve/grpcimpl/OpenInferenceProtocolImpl.java Outdated Show resolved Hide resolved
frontend/server/src/main/java/org/pytorch/serve/grpcimpl/OpenInferenceProtocolImpl.java Show resolved Hide resolved
...erver/src/main/java/org/pytorch/serve/http/api/rest/OpenInferenceProtocolRequestHandler.java Outdated Show resolved Hide resolved
...erver/src/main/java/org/pytorch/serve/http/api/rest/OpenInferenceProtocolRequestHandler.java Outdated Show resolved Hide resolved
frontend/server/src/main/java/org/pytorch/serve/grpcimpl/OpenInferenceProtocolImpl.java Outdated Show resolved Hide resolved
frontend/server/src/main/java/org/pytorch/serve/job/GRPCJob.java Outdated Show resolved Hide resolved
kubernetes/kserve/tests/scripts/test_mnist.sh Outdated Show resolved Hide resolved
@andyi2it andyi2it force-pushed the oip-impl branch 2 times, most recently from e893d12 to 572ec36 Compare January 10, 2024 11:16
lxning
lxning reviewed Jan 16, 2024
View reviewed changes
Copy link
Collaborator

@lxning lxning left a comment

Choose a reason for hiding this comment

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

It seems that the support for kserve v2 http inference is missed. Could you please also add the test for this?

frontend/server/src/main/java/org/pytorch/serve/ServerInitializer.java Outdated Show resolved Hide resolved
@andyi2it
Open Inference Protocol Implementation.
9db2e30 
Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com>
@andyi2it
added worker status check.
8b0a37c 
Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com>
@andyi2it
torchserve version updated with server metadata.
f1120a6 
Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com>
@andyi2it
Added OIP e2e test in CI/CD.
379334d 
    1. Allocated ISVC resource to avoid pod running timeout.
    2. Configured environment variable INFERENCE_PROTOCOL as 'oip'.
    3. Increased `max_wait_time` for pod running.
    4. Deleted ISVC once the test has been passed.

Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com>
@andyi2it
Addressed review feedback:
c48c214 
    1. Updated environment variable to `TS_OPEN_INFERENCE_PROTOCOL`.
    2. Added logic to read the variable `ts_open_inference_protocol=true` from the property file to determine if OIP is enabled or not.
    3. Implemented extra check for OIP `ModelInferResponse` in GRPC responses.
    4. Utilized local path for the proto file in test_mnist.sh.

Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com>
@andyi2it
Changes from review feedback:
6e1c4d2 
    1. Modified `server live`, `server health`, `model ready` check method.
    2. Added tests `server live`, `server health`, `model ready` for grpc
    3. Added tests `server live`, `server health`, for http

Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com>
@andyi2it
Added a new worker command to handle the KServe OIP inference request.
c9f1e40 
Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com>
@andyi2it
Added OIPPREDICT worker command in Job to allow oip requests.
d5975e0 
Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com>
@andyi2it
Format changes.
08bab0f 
Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com>
@andyi2it
Copy link
Contributor Author

@lxning
I have updated formatting.
Can you please approve the workflow run.

@lxning lxning added this pull request to the merge queue Jan 24, 2024
Merged via the queue into pytorch:master with commit 9e6f1c2 Jan 24, 2024
13 checks passed
@harshita-meena
Copy link
Contributor

harshita-meena commented Feb 21, 2024 •
edited
Loading

Posted an issue i faced while using OIP #2951
Let me know if this is closely tied with kserve because I do not use that. I am using a very generic kubernetes deployment/service with envoy proxy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gavrissh gavrissh gavrissh approved these changes

@lxning lxning lxning approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Native Support for KServe Open Inference REST/gRPC Protocol
4 participants
@andyi2it @harshita-meena @lxning @gavrissh