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

feat: Add GRPC error codes to GRPC streaming if enabled by user. #7499

Merged
merged 35 commits into from
Aug 16, 2024

Conversation

indrajit96
Copy link
Contributor

@indrajit96 indrajit96 commented Aug 5, 2024

What does the PR do?

Enhanced GRPC streaming to enable sending GRPC error codes if enabled by user.
User can enabled/disable this feature by including "grpc_strict":true as part of GRPC headers.

Checklist

  • PR title reflects the change and is of format <commit_type>: <Title>
  • Changes are described in the pull request.
  • Related issues are referenced.
  • [] Populated github labels field
  • Added test plan and verified test passes.
  • [] Verified that the PR passes existing CI.
  • Verified copyright is correct on all changed files.
  • [] Added succinct git squash message before merging ref.
  • All template sections are filled out.
  • [] Optional: Additional screenshots for behavior/output changes with before/after.

Commit Type:

Check the conventional commit type
box here and add the label to the github PR.

  • build
  • ci
  • docs
  • feat
  • fix
  • perf
  • refactor
  • revert
  • style
  • [Y] test

Related PRs:

None

Where should the reviewer start?

ExtractStateFromHeaders() function and Process() function in stream_infer_handler.cc

Test plan:

  • Added to qa/L0_backend_python/lifecycle/lifecycle_test.py
  • L0_decoupled
  • L0_grpc
  • L0_grpc_state_cleanup
  • L0_request_cancellation
  • L0_secure_grpc

Background

Customer request to have GRPC error codes as part of GRPC response in case of errors from core
TEP link : https://docs.google.com/document/d/1TfNAMYLsPuLrduBtAKo64YqV55IWQhcn6zayc_CBY18/edit?pli=1

@rmccorm4 rmccorm4 changed the title feat:Add GRPC error codes to GRPC streaming if enabled by user. feat: Add GRPC error codes to GRPC streaming if enabled by user. Aug 5, 2024
src/grpc/infer_handler.h Outdated Show resolved Hide resolved
src/grpc/infer_handler.h Outdated Show resolved Hide resolved
src/grpc/infer_handler.h Outdated Show resolved Hide resolved
src/grpc/infer_handler.h Outdated Show resolved Hide resolved
@@ -0,0 +1,52 @@
# Copyright (c) 2024, NVIDIA CORPORATION. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to move the model to the L0_backend_python/lifecycle folder? I think it might be easier this way to track which test the models belong to.

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 don't have any models or model folder in
https://github.com/triton-inference-server/server/tree/main/qa/L0_backend_python/lifecycle
In test.sh we copy over from python_models and create the models folder with versions inside.
I have kept this new model in parallel to existing models used in L0_backend_python/lifecycle

Copy link
Contributor

Choose a reason for hiding this comment

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

you can always create a models subfolder under L0_* tests

Copy link
Member

Choose a reason for hiding this comment

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

thanks Olga, that's what I meant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the existing test.sh we do
rm -fr *.log ./models before we start the test.

rm -fr *.log ./models

For me to add models to L0_backend_python/lifecycle I will need to remove this.
We cleanup the models before every test, I will need to remove all the instances where we delete the models folder.


Not sure if I should change existing design might impact other tests too.

Copy link
Contributor

Choose a reason for hiding this comment

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

name the folder differently and copy from it to models ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tanmayv25 suggesnted I move all the changes from L0_lifecycle to a new L0_ dedicated for this feature going forward.
The reason I made changes here was, L0_lifecycle/models had models where we send errors pragmatically helped me reuse all the models.
Will resolve this comment along with the original comment by @tanmayv25 in a new PR after the cherrypick.
Keeping it unresolved for now will mark this resolved after the new PR

src/grpc/infer_handler.h Outdated Show resolved Hide resolved
src/grpc/infer_handler.h Outdated Show resolved Hide resolved
qa/L0_backend_python/lifecycle/lifecycle_test.py Outdated Show resolved Hide resolved
qa/L0_decoupled/test.sh Outdated Show resolved Hide resolved
tanmayv25
tanmayv25 previously approved these changes Aug 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants