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

Extract request's trace context in GenerateStream() #64

Merged

Conversation

ronensc
Copy link
Contributor

@ronensc ronensc commented Jul 25, 2024

Description

This PR applies the logic of extracting request's trace context in GenerateStream() as done in Generate()

This is a followup on #20.

How Has This Been Tested?

I set up a local environment where I ran vllm-tgis-adapter and fmaas-router.
Additionally, I ran jaeger and configured both vllm-tgis-adapter and fmaas-router to export traces to jaeger.
Then, I sent a GenerateStream request using grpcurl to fmaas-router and verified that there is a single trace in Jaeger containg spans from both vllm-tgis-adapter and fmaas-router.

Merge criteria:

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

@codecov-commenter
Copy link

codecov-commenter commented Jul 25, 2024

Codecov Report

Attention: Patch coverage is 42.85714% with 4 lines in your changes missing coverage. Please review.

Project coverage is 61.19%. Comparing base (09290e6) to head (3953bd8).

Files Patch % Lines
src/vllm_tgis_adapter/grpc/grpc_server.py 42.85% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #64      +/-   ##
==========================================
- Coverage   61.29%   61.19%   -0.11%     
==========================================
  Files          20       20              
  Lines        1186     1193       +7     
  Branches      210      212       +2     
==========================================
+ Hits          727      730       +3     
- Misses        383      385       +2     
- Partials       76       78       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dtrifiro dtrifiro self-requested a review July 29, 2024 17:18
Copy link
Contributor

@dtrifiro dtrifiro left a comment

Choose a reason for hiding this comment

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

This looks good, though before merging, we need to fix CI to make sure that vllm@main tests pass.

@dtrifiro dtrifiro force-pushed the generate-stream-trace-context branch from 5ebd786 to 3953bd8 Compare August 7, 2024 15:17
@dtrifiro dtrifiro enabled auto-merge August 7, 2024 15:17
@dtrifiro dtrifiro added this pull request to the merge queue Aug 7, 2024
Merged via the queue into opendatahub-io:main with commit 188d3ce Aug 7, 2024
3 checks passed
@ronensc ronensc deleted the generate-stream-trace-context branch August 21, 2024 07:17
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.

3 participants