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 ability to configure timeouts #204

Merged
merged 62 commits into from
Feb 20, 2024

Conversation

aidoskanapyanov
Copy link
Contributor

@aidoskanapyanov aidoskanapyanov commented Feb 10, 2024

Description of the change

Added a timeout kwarg to relevant methods where it can be used (closes #182)

Methods extended with timeout kwarg:

  • generate_answer
  • embed_content
  • generate_content
  • generate_content_async

Motivation

Makes it possible to wait for the response longer, for example if the number of tokens is large, or the payload has multiple files/images. The default behavior is to stop after 60 seconds and the current API doesn't allow to change it (see default value here).

Type of change

Feature request

Checklist

  • I have performed a self-review of my code.
  • I have added detailed comments to my code where applicable.
  • I have verified that my change does not break existing code.
  • My PR is based on the latest changes of the main branch (if unsure, please run git pull --rebase upstream main).
  • I am familiar with the Google Style Guide for the language I have coded in.
  • I have read through the Contributing Guide and signed the Contributor License Agreement.

@aidoskanapyanov aidoskanapyanov requested a review from a team as a code owner February 10, 2024 17:22
@github-actions github-actions bot added status:awaiting review PR awaiting review from a maintainer component:python sdk Issue/PR related to Python SDK labels Feb 10, 2024
Copy link
Member

@markmcd markmcd 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 the PR!

WDYT @MarkDaoust? This is helpful - though I wonder if it's worth exposing something like req_kwargs instead?

Another thought is that we can already set the timeout in the client constructors, so this isn't strictly needed, but I do prefer the ergonomics of setting per-request.

google/generativeai/answer.py Outdated Show resolved Hide resolved
google/generativeai/answer.py Outdated Show resolved Hide resolved
@aidoskanapyanov
Copy link
Contributor Author

It turns out every other method/function involving the client has a timeout option. For some reason I missed that.
I guess there should be a better way than passing it as an argument in all of them. 😅

@MarkDaoust
Copy link
Collaborator

Hi, thanks for this.

The other SDK teams have already decided to sync on passing this (and anything else) as a request_options dictionary/object, like: request_options={'timeout': ...}.

We can go a bit farther to define what those other options are, but I think the minimum we would need to accept this would be if you can replace all the timeouts with request_options (request_options: dict[str:Any] | None).

Then client.generate_answer(request, **request_options).

If you can pick that up it would be great. If not I'll push some commits on top of this when I can.

Thanks again.

@MarkDaoust
Copy link
Collaborator

Also GenerativeModel needs to accept request_options to keep as the default for any call made to that model.

For some reason I missed that. I guess there should be a better way than passing it as an argument in all of them.

Like @markmcd said, I think users can set some of these things at the client level, but if you care about these settings at all, you probably don't want the same ones for every method.

@aidoskanapyanov
Copy link
Contributor Author

Hi, thanks for the feedback!
Sure, I'll update the methods and tests accordingly, to use request_options.

@aidoskanapyanov
Copy link
Contributor Author

Hi @MarkDaoust! I've added request_options argument and the corresponding unit tests for them. Made sure the values are properly passed and unpacked into client api. Didn't expect this PR to be this big, though it's mostly repetitive changes. 😅

@jun0wanan
Copy link

Description of the change

Added a timeout kwarg to relevant methods where it can be used (closes #182)

Methods extended with timeout kwarg:

  • generate_answer
  • embed_content
  • generate_content
  • generate_content_async

Motivation

Makes it possible to wait for the response longer, for example if the number of tokens is large, or the payload has multiple files/images. The default behavior is to stop after 60 seconds and the current API doesn't allow to change it (see default value here).

Type of change

Feature request

Checklist

  • I have performed a self-review of my code.
  • I have added detailed comments to my code where applicable.
  • I have verified that my change does not break existing code.
  • My PR is based on the latest changes of the main branch (if unsure, please run git pull --rebase upstream main).
  • I am familiar with the Google Style Guide for the language I have coded in.
  • I have read through the Contributing Guide and signed the Contributor License Agreement.

hi, so how can we use it ? I also think 60 is too short.... But now how can we change this ?

@aidoskanapyanov
Copy link
Contributor Author

hi, so how can we use it ? I also think 60 is too short.... But now how can we change this ?

Hi, @jun0wanan! This Pull Request is pending a review/approval and not in the release yet.

@jun0wanan
Copy link

hi, so how can we use it ? I also think 60 is too short.... But now how can we change this ?

Hi, @jun0wanan! This Pull Request is pending a review/approval and not in the release yet.

Thanks!

So, if I want to input long text and images into an API ( the model is gemini-vision-pro ), is there a way to solve this? Because this model does not support multi-turn dialogues and can only input very long text and images.

However, I am currently evaluating the capabilities of this multimodal model for handling long text and images.

Can you have any ideas?

Copy link
Collaborator

@MarkDaoust MarkDaoust left a comment

Choose a reason for hiding this comment

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

Wow! Thanks @aidoskanapyanov!

This is a huge help.

Everything looks right to me. Thank you!

@MarkDaoust MarkDaoust merged commit c3c90f8 into google-gemini:main Feb 20, 2024
7 checks passed
@github-actions github-actions bot removed the status:awaiting review PR awaiting review from a maintainer label Feb 20, 2024
@quimpm
Copy link

quimpm commented Mar 15, 2024

@jun0wanan In the release notes there is an example on how to use the new timeout feature:

https://github.com/google/generative-ai-python/releases/tag/v0.4.0

@aidoskanapyanov aidoskanapyanov deleted the add-timeout-option branch March 15, 2024 12:15
@renatofrota
Copy link

Hello. I can't seerequest_options parameter on start_chat or send_message methods, nor a way to define a timeout globally on chat/model init.

@sig-mandel
Copy link

This should priorized if Google wants devs to use the Gemini API, specially since they promote it as having an edge over other models due to the large context window. But its not usable because it timeouts if you send large context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:python sdk Issue/PR related to Python SDK
Projects
None yet
7 participants