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 MediaArtifact #520

Merged
merged 7 commits into from
Apr 12, 2024
Merged

Add MediaArtifact #520

merged 7 commits into from
Apr 12, 2024

Conversation

andrewfrench
Copy link
Member

@andrewfrench andrewfrench commented Dec 21, 2023

Adds a MediaArtifact class. ImageArtifact as well as future types inherit from a common MediaArtifact to capture common fields and functionality like artifact naming and MIME type support.

@andrewfrench andrewfrench changed the title Add MediaArtifact Add BaseMediaArtifact Dec 21, 2023
@collindutter
Copy link
Member

@andrewfrench would you like to keep this PR open?

@andrewfrench
Copy link
Member Author

Yes, I'll dust this off and we can get it in. It's still useful as we expand to other media artifact types.

@andrewfrench andrewfrench changed the title Add BaseMediaArtifact Add MediaArtifact Apr 8, 2024
@andrewfrench andrewfrench marked this pull request as ready for review April 8, 2024 22:50
Comment on lines 21 to 23
value: Raw bytes representing media data.
name: Artifact name, generated using creation time and a random string.
mime_type: The mime type of the image, like image/png or audio/wav.
Copy link
Member

Choose a reason for hiding this comment

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

Docstring appears a bit out of sync with actual fields.

Comment on lines 28 to 29
artifact_type: str = field(default="media", kw_only=True, metadata={"serializable": True})
format: str = field(kw_only=True, metadata={"serializable": True})
Copy link
Member

Choose a reason for hiding this comment

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

Do these need to be split out like this? Do they serve any purpose other than building mime_type? I think artifact_type is a bit confusing because it's really the media type.

Copy link
Member Author

Choose a reason for hiding this comment

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

These attributes contribute to mime_type as well as artifact name. Independent format metadata in particular is useful for things like enforcing format limitations on model inputs.

Copy link
Member

@collindutter collindutter left a comment

Choose a reason for hiding this comment

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

Looks great, can you please update the changelog?

@collindutter collindutter merged commit 54ef8fb into dev Apr 12, 2024
6 checks passed
@collindutter collindutter deleted the french/multimedia-artifacts branch April 12, 2024 21:13
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.

None yet

2 participants