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

The project is still crating many gcs clients #626

Open
david-gang opened this issue Nov 28, 2024 · 5 comments
Open

The project is still crating many gcs clients #626

david-gang opened this issue Nov 28, 2024 · 5 comments

Comments

@david-gang
Copy link
Contributor

After i fixed: ef44612

I suddenly saw that ImageBytesLoader is created ad hoc in every function. So the commit mentioned is just a first step to fix this issue.

Look please here: https://github.com/search?q=repo%3Alangchain-ai%2Flangchain-google%20ImageBytesLoader&type=code

What do you suggest here?

The path which i hit is at

return ImageBytesLoader(project=project).load_gapic_part(path)
and this is a stateless function.

I see one possible solution which is creating a cache of imagebytsloader. for xample creating a map from project to imagbytesloader (lru_cache) . But this is a bigger change and want to hear what you think.

@lkuligin
Copy link
Collaborator

lkuligin commented Dec 2, 2024

maybe it's easier to create a property and a lazily initialzed _image_loader attribute (the first time you get the property, it checks whether the loader is initialized, and initializes it if needed)

@jzaldi
Copy link
Contributor

jzaldi commented Dec 11, 2024

Why is this a problem? AFAIK GCS Client is just a bundle of configuration parameters to make API calls (but feel free to correct me). Is this a big performance hit?

@david-gang
Copy link
Contributor Author

as far as i know every storage client maintains its http client which is a request session so it will not reuse connections

@david-gang
Copy link
Contributor Author

maybe it's easier to create a property and a lazily initialzed _image_loader attribute (the first time you get the property, it checks whether the loader is initialized, and initializes it if needed)

I don't understand what this means exactly. I have two issues here:

  • The class is created from a function and not another class so i don't understand the attribute
  • We are passing project as input so an attribute is not enough we need a map from project to client

@jzaldi
Copy link
Contributor

jzaldi commented Dec 11, 2024

Did a dirty performance check and it seems that it does have an impact.

I think what @lkuligin means is:

  • Make storage_client a ImageBytesLoader attribute (Maybe optional?)
  • Have a client (lazyly initialized and cached) at ChatVertexAI level
  • Pass this client in the builder of ImageBytesLoader so that it reuses it.

This limits the client creation to one per ChatVertexAI initialization

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

No branches or pull requests

3 participants