Skip to content

feat: Improve client perf and error handling #247

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
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: Improve client perf and error handling #247

wants to merge 1 commit into from
+57 −41

Conversation

@LukeAVanDrie
Copy link
Contributor

Reuses aiohttp.ClientSession across requests in openAIModelServerClient to reduce connection overhead. This change improves client-side throughput and latency.

Additional improvements:

  • Refines error handling to distinguish between network errors (like aiohttp.ClientError), non-200 HTTP status codes, and errors during response processing.
  • Ensures non-200 responses with text bodies are captured.
  • Guarantees response body is always consumed to release connections.

@LukeAVanDrie
feat: Improve client perf and error handling
2350ef6 
Reuses aiohttp.ClientSession across requests in openAIModelServerClient
to reduce connection overhead. This change improves client-side
throughput and latency.

Additional improvements:
- Refines error handling to distinguish between network errors
  (like aiohttp.ClientError), non-200 HTTP status codes, and errors
	during response processing.
- Ensures non-200 responses with text bodies are captured.
- Guarantees response body is always consumed to release connections.
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 7, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: LukeAVanDrie
Once this PR has been reviewed and has the lgtm label, please assign achandrasekar for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 7, 2025
jjk-g
jjk-g reviewed Oct 7, 2025
View reviewed changes
inference_perf/client/modelserver/openai_client.py
)
)

end_time = time.perf_counter()
Copy link
Contributor

Choose a reason for hiding this comment

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

can move to a finally block

jjk-g
jjk-g reviewed Oct 7, 2025
View reviewed changes
Copy link
Contributor

@jjk-g jjk-g left a comment

Choose a reason for hiding this comment

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

Thanks for adding! One nit

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 7, 2025
achandrasekar
achandrasekar reviewed Oct 10, 2025
View reviewed changes
Copy link
Contributor

@achandrasekar achandrasekar left a comment

Choose a reason for hiding this comment

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

Can you add how the change was tested and if you have any numbers on improvements that'd be great too?

@achandrasekar
Copy link
Contributor

Please address the linting and type check issue above

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 16, 2025
@k8s-ci-robot
Copy link
Contributor

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@jjk-g
Copy link
Contributor

jjk-g commented Oct 23, 2025

@LukeAVanDrie friendly ping for linting and type check errors

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

Reviewers

@achandrasekar achandrasekar achandrasekar left review comments

@Bslabe123 Bslabe123 Awaiting requested review from Bslabe123

+1 more reviewer

@jjk-g jjk-g jjk-g left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@jjk-g jjk-g

Labels

cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@LukeAVanDrie @k8s-ci-robot @achandrasekar @jjk-g