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: Support for request id field in generate API #7392

Merged
merged 9 commits into from
Jul 10, 2024

Conversation

shreyas-samsung
Copy link
Contributor

@shreyas-samsung shreyas-samsung commented Jun 28, 2024

What does the PR do?

This PR adds support for request id field in /generate API. This makes it possible to debug and trace any particular request by giving an identifier in request body which gets logged everywhere.

Checklist

  • I have read the Contribution guidelines and signed the Contributor License
    Agreement
  • 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.
  • I ran pre-commit locally (pre-commit install, pre-commit run --all)
  • 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 Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)

@nv-kmcgill53 nv-kmcgill53 requested a review from GuanLuo July 1, 2024 16:38
@dafu-wu
Copy link

dafu-wu commented Jul 8, 2024

Hi Already done?

@shreyas-samsung
Copy link
Contributor Author

Hi @GuanLuo request you to review

@rmccorm4 rmccorm4 requested review from yinggeh and rmccorm4 July 8, 2024 19:40
@rmccorm4 rmccorm4 self-assigned this Jul 8, 2024
@rmccorm4
Copy link
Collaborator

rmccorm4 commented Jul 8, 2024

Hi @shreyas-samsung,

Thanks for contributing the PR!

Can you also try adding a new test function test_request_id to the existing test, similar to here?

This test should minimally:

  1. try adding an ID to the request, and assert that the responses contain the same ID
  2. send a request with no ID and assert that the response contains no ID
  3. try sending a request with id = "" to check no unexpected failures happen with an empty string

If you run into any issues with this, I can try to help with adding the tests as well.

@shreyas-samsung
Copy link
Contributor Author

Hi @rmccorm4 Thank you for the review. I have added the test as per your suggestion.

src/http_server.cc Outdated Show resolved Hide resolved
@@ -142,6 +142,49 @@ def test_generate(self):
self.assertIn("TEXT", data)
self.assertEqual(text, data["TEXT"])

def test_request_id(self):
Copy link
Collaborator

@rmccorm4 rmccorm4 Jul 9, 2024

Choose a reason for hiding this comment

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

Thanks for adding the test! I will run a pipeline with these changes.

Pipeline ID: 16450437

Copy link
Collaborator

@rmccorm4 rmccorm4 Jul 10, 2024

Choose a reason for hiding this comment

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

Pipeline looks mostly good, and the new unit test passed, however, please update this variable from 15 to 16 to account for the new test added:

EXPECTED_NUM_TESTS=15

EXPECTED_NUM_TESTS=16

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Updated count to 16

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks! Ran a new pipeline 16473896 and it passed 🚀 Once I verify the CLA, this can be merged 👍

@yinggeh
Copy link
Contributor

yinggeh commented Jul 10, 2024

LGTM

@nv-kmcgill53
Copy link
Contributor

We have accepted your CLA and can merge at the Triton team's discretion.

Copy link
Collaborator

@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.

LGTM, thanks for the contribution @shreyas-samsung, we really appreciate it!

@rmccorm4 rmccorm4 changed the title fix: Support for request id field in generate API feat: Support for request id field in generate API Jul 10, 2024
@rmccorm4 rmccorm4 added the PR: feat A new feature label Jul 10, 2024
@rmccorm4 rmccorm4 merged commit d1780d1 into triton-inference-server:main Jul 10, 2024
3 checks passed
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