-
Notifications
You must be signed in to change notification settings - Fork 162
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
Refactor tasks to use ListArtifact as input #811
Conversation
a2a9f01
to
4a367ef
Compare
Codecov ReportAttention: Patch coverage is 📢 Thoughts on this report? Let us know! |
d377a42
to
ba58c99
Compare
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.
Two tiny comments, very cool to be able to support this change and not worry about breaking interfaces for users!
value: ( | ||
tuple[str, list[ImageArtifact]] | ||
| tuple[TextArtifact, list[ImageArtifact]] | ||
| Callable[[BaseTask], ListArtifact] |
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.
My one consideration was whether or not it's useful to accept a ListArtifact
here directly, similarly to how the Callable
now just returns a ListArtifact
. I don't think I've run into a situation where this would have made anything more convenient, and these input type unions are already unwieldy enough, but curious what you think.
griptape/tasks/image_query_task.py
Outdated
@@ -68,8 +69,9 @@ def image_query_engine(self, value: ImageQueryEngine) -> None: | |||
self._image_query_engine = value | |||
|
|||
def run(self) -> TextArtifact: | |||
query, image_artifacts = self.input | |||
query = self.input.value[0] | |||
image_artifacts = [input for input in self.input.value[1:] if isinstance(input, ImageArtifact)] |
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.
This quietly drops non-image inputs, should we raise an exception if self.input.value[1:]
aren't all ImageArtifact
s like in the in/outpainting tasks?
ba58c99
to
4ada8c6
Compare
@andrewfrench thanks for the review, just fixed both of your comments. |
4ada8c6
to
49ad364
Compare
Several Tasks have non-
BaseArtifact
input
fields. For instance,ImageQueryTask.input
has a type of:The complexity of this type makes it very difficult to serialize the inputs of these Tasks in downstream abstractions like Events. Additionally, it make the Task API inconsistent across Task types.
This PR changes the input type to
ListArtifact
, making it consistent with other Task types. Note that thesetter
for the field is mostly unchanged, only thegetter
has changed.