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: indexError for [] candidates #4

Closed
wants to merge 1 commit into from

Conversation

hemanth
Copy link

@hemanth hemanth commented May 6, 2023

This PR fixes the indexError but not the root cause.

For cases where candidates are [] we end up with IndexError: list index out of range maybe it is a limitation of bison? Should we show a default error message?

For the below input we always get an indexError:

input:
Can you fetch from URL?

output:

{'messages': [{'author': '0', 'content': 'Can you fetch from URL?'}], 'candidates': []}

This PR fixes the `indexError` but not the root cause.
@markmcd
Copy link
Member

markmcd commented May 8, 2023

Thanks, and congrats on being the first external PR 👍

LGTM but @MarkDaoust or @shilpakancharla know this part better than me, so I'll wait for them.

@markmcd markmcd requested review from MarkDaoust and markmcd May 8, 2023 09:13
@MarkDaoust
Copy link
Collaborator

MarkDaoust commented May 8, 2023

Yes, thanks, for the PR!

This change avoids the index error, but I think its a little too subtle about it.

import this -> "Errors should not pass silently"

After the call anyone who isn't dealing with candidates themselves is expecting that the last item in messages is the response from the model, and .last is the text of that response. If there was no response I'd want to have a None in both those locations.

If we just skip the append like this and you call reply you'll be replying to your previous message, with no message from the bot in between, and because the we assume alternating messages, you'll have switched roles with the bot which is strange. I'd rather just fail with a clear error in reply.

I was actually fixing this in this PR (its kind of a mess but look at chat.py): #7

So I'm sorry I have to close this. But thanks for finding us and helping out, we really appreciate it.

@MarkDaoust MarkDaoust closed this May 8, 2023
@MarkDaoust
Copy link
Collaborator

Please reach out if you find any other issues.

@hemanth
Copy link
Author

hemanth commented May 15, 2023

Thank you! Yes, that makes sense, better than erroring out or silently failing.

 if response["candidates"]:
    last = response["candidates"][0]
 else:
    last = None
 request["messages"].append(last)

MarkDaoust added a commit that referenced this pull request Jun 13, 2024
* Squashed commit of the following:

commit acb3806
Author: Mayuresh Agashe <magashe@google.com>
Date:   Wed Jun 5 00:51:30 2024 +0000

    fix update method

    Change-Id: I433c25b2d80cdf6e483b59f61ff29bb8d2dc6595

commit fb9995c
Merge: 4627fe1 7b9758f
Author: Mark Daoust <markdaoust@google.com>
Date:   Tue Jun 4 09:55:38 2024 -0700

    Merge branch 'main' into caching

    Change-Id: I2bade6b0099f12dd37a24fe26cfda1981c58fbc0

commit 4627fe1
Author: Mark Daoust <markdaoust@google.com>
Date:   Tue Jun 4 09:54:31 2024 -0700

    use preview build

    Change-Id: Ic1cd4fc28f591794dc5fbff0647a00a77ea7f601

commit 8e86ef1
Author: Mayuresh Agashe <magashe@google.com>
Date:   Thu May 30 16:18:22 2024 +0000

    Refactor for genai.protos module

    Change-Id: I2f02d2421d7303f0309ec86f05d33c07332c03c1

commit 82d3c5a
Merge: bf6551a f08c789
Author: Mayuresh Agashe <magashe@google.com>
Date:   Thu May 30 15:57:27 2024 +0000

    Merge branch 'main' of https://github.com/mayureshagashe2105/generative-ai-python into caching

    Change-Id: Id2b259fe4b2c91653bf5e4d5e883f556366d8676

commit bf6551a
Author: Mayuresh Agashe <magashe@google.com>
Date:   Mon May 27 11:26:03 2024 +0000

    Fix types

    Change-Id: Id3e7316562f4029e5b7409ae725bb66e2207f075

commit 67472d3
Author: Mayuresh Agashe <magashe@google.com>
Date:   Mon May 27 11:26:03 2024 +0000

    Fix types

    Change-Id: Id3e7316562f4029e5b7409ae725bb66e2207f075

commit a1c8c72
Author: Mayuresh Agashe <magashe@google.com>
Date:   Mon May 27 11:15:15 2024 +0000

    Fix docstrings

    Change-Id: I6020df4e862a4f1d58462a4cd70876a8448293cf

commit f48cedc
Author: Mayuresh Agashe <magashe@google.com>
Date:   Mon May 27 11:13:44 2024 +0000

    Fix types

    Change-Id: Ia4bf6b936fab4c1992798c65cff91c15e51a92c0

commit 645ceab
Author: Mayuresh Agashe <magashe@google.com>
Date:   Mon May 27 05:54:26 2024 +0000

    blacken

    Change-Id: I4e073d821d29eea30801bdb7e2a8dc01bb7d6b9a

commit 17372e3
Author: Mayuresh Agashe <magashe@google.com>
Date:   Mon May 27 05:54:06 2024 +0000

    Add 'cached_content' to GenerativeModel's repr

    Change-Id: I06676fad23895e3e1a6393baa938fc1f2df57d80

commit d1fd749
Author: Mayuresh Agashe <magashe@google.com>
Date:   Mon May 27 05:04:43 2024 +0000

    Add type-annotations to __new__ to fix pytype checks

    Change-Id: I6c69c036e54d56d18ea60368fa0a1dcda2d315fd

commit f37df8c
Author: Mayuresh Agashe <magashe@google.com>
Date:   Sun May 26 06:51:54 2024 +0000

    mark name as OPTIONAL for CachedContent creation

    If not provided, the name will be randomly generated

    Change-Id: Ib95fbafd3dfe098b43164d7ee4d6c2a84b0aae2e

commit 59663c8
Author: Mayuresh Agashe <magashe@google.com>
Date:   Fri May 24 10:22:08 2024 +0000

    Add tests

    Change-Id: I249188fa585bd9b7193efa48b1cfca20b8a79821

commit e1d8c7a
Author: Mayuresh Agashe <magashe@google.com>
Date:   Fri May 24 10:21:42 2024 +0000

    Validate name checks for CachedContent creation

    Change-Id: Ie41602621d99ddff6404c6708c7278e0da790652

commit 2cde1a2
Author: Mayuresh Agashe <magashe@google.com>
Date:   Thu May 23 18:09:14 2024 +0000

    fix tests

    Change-Id: I39f61012f850a82e09a7afb80b527a0f99ad0ec7

commit d862dae
Author: Mayuresh Agashe <magashe@google.com>
Date:   Thu May 23 18:09:14 2024 +0000

    fix tests

    Change-Id: I39f61012f850a82e09a7afb80b527a0f99ad0ec7

commit d35cc71
Author: Mayuresh Agashe <magashe@google.com>
Date:   Thu May 23 23:12:38 2024 +0530

    Improve tests

commit e65d16e
Author: Mayuresh Agashe <magashe@google.com>
Date:   Thu May 23 23:12:05 2024 +0530

    blacken

commit cfc936e
Author: Mayuresh Agashe <magashe@google.com>
Date:   Thu May 23 23:10:16 2024 +0530

    Stroke out functional approach for CachedContent CURD ops

commit afd066d
Merge: 6fafe6b 0dca4ce
Author: Mayuresh Agashe <magashe@google.com>
Date:   Wed May 22 23:10:20 2024 +0530

    Merge branch 'main' into caching

commit 6fafe6b
Author: Mayuresh Agashe <magashe@google.com>
Date:   Wed May 22 10:49:35 2024 +0530

    rename get_cached_content to get

commit a4ac7a5
Merge: f13228d f987fde
Author: Mayuresh Agashe <magashe@google.com>
Date:   Tue May 21 23:32:41 2024 +0530

    Merge branch 'main' into caching

commit f13228d
Author: Mayuresh Agashe <magashe@google.com>
Date:   Fri Apr 26 16:54:09 2024 +0000

    *Inital prototype for explicit caching

    *Add basic CURD support for caching

    *Remove INPUT_ONLY marked fields from CachedContent dataclass

    *Rename files 'cached_content*' -> 'caching*'

    *Update 'Create' method for explicit instantination of 'CachedContent'

    *Add a factory method to instatinate model with `CachedContent` as
    its context

    *blacken

    *Add tests

    Change-Id: I694545243efda467d6fd599beded0dc6679b727d

Change-Id: I7b14d94f729953294780815f4c496888bb2ad46f

* Remove auto cache deletion

Change-Id: I4658e1c57f967faeb3945dffef0181a456d65370

* Rename _to_dict --> _get_update_fields

Change-Id: I3c92c65e8e5b215e98c1ac0eea6db033166dec78

* Fix tests

Change-Id: Id36d7606e13d15caf6870f29a108944c7f36eaeb

* Set 'CachedContent' as a public property

Remove __new__ construct

Change-Id: Ie4f5527270be90730341b6c3b67de71b9b6e9c5c

* blacken

Change-Id: I12498213a7fc2b257827ab0df87c6913e04cad25

* set 'role=user' when content is passed as a str (#4)

'to_content' method assigns a default 'role=user' to all the contents passed as a string

Change-Id: I748514a7839b7f1d36150b879c3d1464ca9e11ba

* Handle ttl and expire_time separately

Change-Id: If9c6f04fe8d419828e3efd2249f0698bca4d5bdc

* Remove name param

Change-Id: I40fe7c8fafdb014fb9c7e74956452aca9a666641

* Update caching_types.py

* Update caching.py

* Update docstrs and error messages

Change-Id: I111a1218a7d9783d494b84f0a11cb3b76c7ad9da

* Update model name to gemini-1.5-pro for caching tests

Change-Id: Ibb1f75c409afaac124ef70232be71e3a882f6015

* Remove dafault ttl assignment

Let the API set the dafault

Change-Id: Id8d125a085ed27229ddb78d5812ed5b5ad39227b

* blacken

Change-Id: I1d7fe0ec422589e237502b0eda687cf81ef21a21

* Remove client arg

Change-Id: I17f05a90a1514f404dd3527c0db1ce6147d2c47a

* Add 'usage_metadata' param to CachedContent class

Change-Id: Ic527c157bc2cd114948b73a8f1832c21dd61b52e

* Add 'display_name' to CachedContent class

Change-Id: Id0a9be9d1bfdb94dc9d5c4fc7af9dee89e5365a4

* update generativelanguage version, fix tests

Change-Id: I0acc57853ab7dde863bbbe4b30ae3957e6ec3d11

* format

Change-Id: Ib2e9a16aaa989021d3498f3e59f9983560919159

* fewer automatic 'role' insertions

Change-Id: I0752741532a451f8720fa5e110e68f0b4e66cc4b

* cleanup

Change-Id: I151a809f6d079b8e4b0ed30d1153a638c98cacfd

* Wrap the proto

Change-Id: I14b4c54652fb51b867fb43d4b3e9091e6eaccd4e

* Apply suggestions from code review

Co-authored-by: Mayuresh Agashe <magashe@google.com>

* fix

Change-Id: I381029fc8fc13c39e432b39084fc8feba305514e

* format

Change-Id: I8e0b44aebc102d3b2afb27a422c4d70d6c99d5d2

* cleanup

Change-Id: I024733b53cede5bfdf957ce7e56d6ad01fd4b2bf

* update version

Change-Id: Ic95dffb3e945e31adc0d98787942d27289512b8a

* fix

Change-Id: I6ffdabbddf0e803606b3638521ebfeb6796d2e4b

* typing

Change-Id: I629d4d111f0e640f4f4bf602ea33f70fdc9ca3e4

* Simplify update method

Accept kwargs instead of dict of updates and construct protos using kwargs

Change-Id: I7858d585b1aa6b965134e2fb90adff737172af92

* Add repr to CachedContent

Change-Id: Id4ec78ebf9d6e96f22f6bf37fc4509268fa552f4

* cleanup

Change-Id: I684b46f881735bceb3f9e09d8573721ddb29f98a

* blacken

Change-Id: I773e7a5b8a222c8b4435470cdc2b53be425d95e4

* Apply suggestions from code review

Change-Id: I2a12b9689001bbc41c460db5a9f0e87c77d4caf6

---------

Co-authored-by: Mark Daoust <markdaoust@google.com>
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.

3 participants