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

Fix kv_cache_type issue #2219

Closed
wants to merge 1 commit into from
Closed

Conversation

qingquansong
Copy link
Contributor

@qingquansong qingquansong commented Sep 11, 2024

Fix kv_cache_type issue related to #1930

Details described: #1930 (comment)

Fix kv_cache_type issue
@qingquansong
Copy link
Contributor Author

@Barry-Delaney ^^ mind helping take a look? Seems to help resolve some issue here #1930 Thank you!

@lfr-0531 lfr-0531 self-assigned this Sep 21, 2024
@lfr-0531 lfr-0531 added the triaged Issue has been triaged by maintainers label Sep 21, 2024
@lfr-0531
Copy link
Collaborator

Thanks for the fix. We'll merge you changes into internal code base.

@hchings
Copy link
Collaborator

hchings commented Sep 27, 2024

Closing this out as it's been merged.

@hchings hchings closed this Sep 27, 2024
@kaiyux
Copy link
Member

kaiyux commented Sep 29, 2024

Hi @qingquansong , thanks a lot for the contribution! Your changes will be included in the next main branch update, and we'll mark you as co-author.

Please also note that, the Python benchmark is not suggested to be used and will soon be deprecated. Please take a look at the on-going support to a benchmarking suite, as well as the C++ benchmark for the support to the latest features.

Closing this out as it's been merged.

@hchings To clarify in case there are going to be confusion - the changes are merged in the internal repo, but not updated to the external GitHub repo yet. For future cases I would suggest to only close the PR after we pushed the main branch update that includes the changes. Please let me know if there are any questions, thanks!

Thanks again for your support!

@qingquansong
Copy link
Contributor Author

qingquansong commented Sep 29, 2024

Hi @qingquansong , thanks a lot for the contribution! Your changes will be included in the next main branch update, and we'll mark you as co-author.

Please also note that, the Python benchmark is not suggested to be used and will soon be deprecated. Please take a look at the on-going support to a benchmarking suite, as well as the C++ benchmark for the support to the latest features.

Closing this out as it's been merged.

@hchings To clarify in case there are going to be confusion - the changes are merged in the internal repo, but not updated to the external GitHub repo yet. For future cases I would suggest to only close the PR after we pushed the main branch update that includes the changes. Please let me know if there are any questions, thanks!

Thanks again for your support!

Sound great. Thank you! Besides the C++ throughput API benchmarking, I'm currently also switching to using the hlapi and benchmarking it with this perf evaluator script, is the a suggested one to use? It contains both latency and throughput results which is quite nice to use and the only thing that I'm modifying to add is the concurrency part (which I'm planning to use the Poisson request). Not sure if you think that's a good feature to add here .

@kaiyux
Copy link
Member

kaiyux commented Sep 30, 2024

Hi @qingquansong , thanks a lot for the contribution! Your changes will be included in the next main branch update, and we'll mark you as co-author.
Please also note that, the Python benchmark is not suggested to be used and will soon be deprecated. Please take a look at the on-going support to a benchmarking suite, as well as the C++ benchmark for the support to the latest features.

Closing this out as it's been merged.

@hchings To clarify in case there are going to be confusion - the changes are merged in the internal repo, but not updated to the external GitHub repo yet. For future cases I would suggest to only close the PR after we pushed the main branch update that includes the changes. Please let me know if there are any questions, thanks!
Thanks again for your support!

Sound great. Thank you! Besides the C++ throughput API benchmarking, I'm currently also switching to using the hlapi and benchmarking it with this perf evaluator script, is the a suggested one to use? It contains both latency and throughput results which is quite nice to use and the only thing that I'm modifying to add is the concurrency part (which I'm planning to use the Poisson request). Not sure if you think that's a good feature to add here .

@qingquansong Thanks a lot for your attention on those details! The tests/hlapi/hlapi_evaluator.py is under tests directory, and currently used for test purpose only. I would still suggest to try the suite and feel free to let us know your feedback. We'll consolidate the benchmarking workflows.

@DanBlanaru DanBlanaru mentioned this pull request Sep 30, 2024
@DanBlanaru
Copy link
Collaborator

Hello @qingquansong. I was in charge of the publishing for this week.
It seems GitHub did not like the formatting as it did not properly credit you.
I apologize, I will credit you in next week's push.

@DanBlanaru
Copy link
Collaborator

Hello @qingquansong, we credited you in the last push to main .Thank you for the contribution again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Merged triaged Issue has been triaged by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants