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

Refactor tasks to use ListArtifact as input #811

Merged
merged 1 commit into from
May 31, 2024

Conversation

collindutter
Copy link
Member

@collindutter collindutter commented May 31, 2024

Several Tasks have non-BaseArtifact input fields. For instance, ImageQueryTask.input has a type of:

tuple[TextArtifact, list[ImageArtifact]]

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 the setter for the field is mostly unchanged, only the getter has changed.

@collindutter collindutter force-pushed the refactor/list-artifact branch 4 times, most recently from a2a9f01 to 4a367ef Compare May 31, 2024 01:00
Copy link

codecov bot commented May 31, 2024

@collindutter collindutter force-pushed the refactor/list-artifact branch 4 times, most recently from d377a42 to ba58c99 Compare May 31, 2024 16:14
@collindutter collindutter marked this pull request as ready for review May 31, 2024 16:14
andrewfrench
andrewfrench previously approved these changes May 31, 2024
Copy link
Member

@andrewfrench andrewfrench left a 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!

Comment on lines +50 to +58
value: (
tuple[str, list[ImageArtifact]]
| tuple[TextArtifact, list[ImageArtifact]]
| Callable[[BaseTask], ListArtifact]
Copy link
Member

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.

@@ -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)]
Copy link
Member

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 ImageArtifacts like in the in/outpainting tasks?

@collindutter
Copy link
Member Author

@andrewfrench thanks for the review, just fixed both of your comments.

@collindutter collindutter merged commit 5a0e1c3 into dev May 31, 2024
10 checks passed
@collindutter collindutter deleted the refactor/list-artifact branch May 31, 2024 18:11
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.

2 participants