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 parameters support to InferResponse #394

Merged
merged 5 commits into from
Jan 25, 2025
Merged

Conversation

kthui
Copy link
Contributor

@kthui kthui commented Jan 24, 2025

What does the PR do?

Add support for setting response parameters in regular and decoupled Python backend model response(s).

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
  • test

Related PRs:

triton-inference-server/server#7964

Where should the reviewer start?

Start with the test cases on the related server PR, and then pb_stub.cc -> infer_response.cc -> python_be.cc.

Test plan:

New tests are added to the related server PR.

  • CI Pipeline ID: 22968260

Caveats:

Responses to BLS models are not populated with the response parameters, if any. (DLIS-7864)

Background

N/A

Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)

N/A

@kthui kthui added the PR: feat A new feature label Jan 24, 2025
@kthui kthui force-pushed the jacky-py-res-param branch from 7b63cc9 to c3a9c1b Compare January 24, 2025 01:20
@kthui kthui marked this pull request as ready for review January 24, 2025 02:35
src/infer_response.cc Outdated Show resolved Hide resolved
src/infer_response.h Outdated Show resolved Hide resolved
src/pb_stub.cc Outdated Show resolved Hide resolved
kthui added 2 commits January 24, 2025 11:24
* Infer response to track parameters

* Add parameters to binding infer response

* Rank parameters argument up among InferResponse constructor arguments

* Add setting parameters to Triton response

* Send response parameters only on non-error

* Fix double declaration

* Unify py dictionary parameters to json str
@kthui kthui force-pushed the jacky-py-res-param branch from c3a9c1b to 816d4ac Compare January 24, 2025 19:25
README.md Show resolved Hide resolved
Copy link
Contributor

@rmccorm4 rmccorm4 left a comment

Choose a reason for hiding this comment

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

General look&feel LGTM - left a couple questions. Mostly defer to @krishung5 for an expert opinion

Copy link
Contributor

@krishung5 krishung5 left a comment

Choose a reason for hiding this comment

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

🚀

@kthui kthui merged commit 1ea48a6 into main Jan 25, 2025
3 checks passed
@kthui kthui deleted the jacky-py-res-param branch January 25, 2025 00:20
@Tabrizian
Copy link
Member

Thanks Jacky!

@@ -803,6 +803,9 @@ You can read more about the inference response parameters in the [parameters
extension](https://github.com/triton-inference-server/server/blob/main/docs/protocol/extension_parameters.md)
documentation.

Inference response parameters is currently not supported on BLS inference
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure this is clear ....

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: feat A new feature
Development

Successfully merging this pull request may close these issues.

5 participants