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

add _defaultApiClient in api/client.go for reuse #4096

Closed
wants to merge 1 commit into from

Conversation

alwqx
Copy link
Contributor

@alwqx alwqx commented May 2, 2024

Hi, this pr mainly make following improvements:

  1. add _defaultApiClient for reuse
    I find that api.ClientFromEnvironment() is called more than one time by some functions in cmd/cmd.go (e.g. RunHandler() generateInteractive). So I add _defaultApiClient for reuse and to reduce memory alloc.
  2. update tests and split TestClientFromEnvironment to TestClientFromEnvironment and TestGetOllamaHost

I hope these improvements are helpful. @jmorganca @dhiltgen Please feel free to provide feedback if you have any suggestions. And if you think this pr is not needed, I will close it.

@alwqx alwqx changed the title api: add _defaultApiClient for resue api: add _defaultApiClient in cmd/cmd.go for resue May 2, 2024
@alwqx alwqx changed the title api: add _defaultApiClient in cmd/cmd.go for resue api: add _defaultApiClient in api/client.go for resue May 2, 2024
@alwqx alwqx force-pushed the feat/default-api-client-reuse branch from 5085172 to 75c5080 Compare May 3, 2024 13:32
@alwqx alwqx changed the title api: add _defaultApiClient in api/client.go for resue [Done] api: add _defaultApiClient in api/client.go for resue May 3, 2024
@alwqx alwqx changed the title [Done] api: add _defaultApiClient in api/client.go for resue Done: api add _defaultApiClient in api/client.go for resue May 3, 2024
@alwqx alwqx force-pushed the feat/default-api-client-reuse branch 2 times, most recently from e42a23b to 821864d Compare May 7, 2024 02:00
@alwqx alwqx force-pushed the feat/default-api-client-reuse branch from 821864d to c55c01f Compare May 8, 2024 07:09
@alwqx alwqx changed the title Done: api add _defaultApiClient in api/client.go for resue add _defaultApiClient in api/client.go for resue May 8, 2024
@alwqx alwqx changed the title add _defaultApiClient in api/client.go for resue add _defaultApiClient in api/client.go for reuse May 8, 2024
@jmorganca
Copy link
Member

Hi @alwqx thanks for the PR! I do think this might not be as big of a deal as we think as the http clients in Go should be relatively lightweight, and so to keep things simple we can probably just re-create it every time.

@jmorganca jmorganca closed this May 10, 2024
@alwqx
Copy link
Contributor Author

alwqx commented May 10, 2024

I understand, thanks for your feedback.

@alwqx alwqx deleted the feat/default-api-client-reuse branch May 16, 2024 12:45
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.

None yet

2 participants