-
Notifications
You must be signed in to change notification settings - Fork 11
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
Support multiple images #151
Conversation
src/aviary/message.py
Outdated
@@ -124,16 +123,22 @@ def create_message( | |||
cls, | |||
role: str = DEFAULT_ROLE, | |||
text: str | None = None, | |||
image: np.ndarray | None = None, | |||
images: list[np.ndarray | str] | None = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this will break existing uses of create_message
elsewhere. However, imo it's better to make this shift now while it isn't used much than in the future. It will be quite common for users to have multiple images associated with one prompt eg "what species do all the bacteria in these images belong to?". Single images can just be a list of length 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it can have type list[np.ndarray] | np.ndarray | None
. Then, we always encode it inside the function.
We can have a check like this:
if isinstance(images, str):
images = [images]
I believe this will keep compatibility with the previous behavior
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc. @maykcaldas for visibility
This reverts commit 612be2f.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
src/aviary/message.py
Outdated
@@ -124,16 +123,22 @@ def create_message( | |||
cls, | |||
role: str = DEFAULT_ROLE, | |||
text: str | None = None, | |||
image: np.ndarray | None = None, | |||
images: list[np.ndarray | str] | None = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it can have type list[np.ndarray] | np.ndarray | None
. Then, we always encode it inside the function.
We can have a check like this:
if isinstance(images, str):
images = [images]
I believe this will keep compatibility with the previous behavior
pyproject.toml
Outdated
@@ -27,6 +27,7 @@ classifiers = [ | |||
dependencies = [ | |||
"docstring_parser>=0.16", # Pin for description addition | |||
"httpx", | |||
"numpy", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any chance we can avoid adding deps? Sorry I missed this in the original review
We should only add deps if we actually need them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice thanks for taking care of numpy
stuff 🙏
No description provided.