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

feat(weave): Add support for jpegs and pngs #3304

Merged
merged 15 commits into from
Jan 14, 2025
Merged

feat(weave): Add support for jpegs and pngs #3304

merged 15 commits into from
Jan 14, 2025

Conversation

andrewtruong
Copy link
Collaborator

@andrewtruong andrewtruong commented Dec 21, 2024

Resolves:

This PR adds support for logging and viewing JPEGs. Previously, all images would be saved as PNG, even if the source image was a JPEG.

If you somehow force-logged a JPEG, it still wouldn't work because the UI didn't support JPEGs.
(before)
image
(after)
image

Testing is mostly manual, with some client tests added for the different formats. We should add playwright tests when they are available. I have verified that the image that comes back is a jpeg (and you can confirm by inspecting/saving the img)

@andrewtruong andrewtruong changed the title feat(weave): Add support for jpegs feat(weave): Add support for jpegs and pngs Dec 21, 2024
@circle-job-mirror
Copy link

circle-job-mirror bot commented Dec 21, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@andrewtruong andrewtruong marked this pull request as ready for review January 7, 2025 16:27
@andrewtruong andrewtruong requested review from a team as code owners January 7, 2025 16:27
weave/trace/util.py Outdated Show resolved Hide resolved
@andrewtruong andrewtruong requested a review from tssweeney January 9, 2025 18:18
key => key in imageTypes
) as keyof PILImageImageTypePayload['files'];
if (!imageKey) {
throw new Error('No image file found');
Copy link
Collaborator

Choose a reason for hiding this comment

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

def don't throw. just render a empty / some info state. Throwing will crash the page

key => key in imageTypes
) as keyof PILImageImageTypePayload['files'];
if (!imageKey) {
return <span>Img not found!</span>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would use weave-js/src/components/PagePanelComponents/Home/Browse2/NotApplicable.tsx this component

key => key in imageTypes
) as keyof PILImageImageTypePayload['files'];
if (!imageKey) {
return <span>Img not found!</span>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

early returns must happen after all use hooks


const imageKey = Object.keys(props.data.files).find(
key => key in imageTypes
) as keyof PILImageImageTypePayload['files'];
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is still not correct since you are not guaranteed to find the key. This cast is invalid and causes line 37 to be incorrect.

@andrewtruong andrewtruong enabled auto-merge (squash) January 14, 2025 18:57
@andrewtruong andrewtruong merged commit 07e3c40 into master Jan 14, 2025
121 of 123 checks passed
@andrewtruong andrewtruong deleted the andrew/pil branch January 14, 2025 19:12
@github-actions github-actions bot locked and limited conversation to collaborators Jan 14, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants