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

Make UploadedFile class serializable #679

Merged
merged 4 commits into from
Jul 29, 2024

Conversation

richard-to
Copy link
Collaborator

Since we're inheriting from io.Bytes, this made it harder to serialize since we couldn't just make it a data class.

So to workaround this we added custom encoder/decoder logic to handle UploadedFile specifically.

We also needed to add a custom operator for DeepDiff. For whatever reason deep diff wasn't diffing it correctly, so instead we just perform a basical equality check here.

Demos/tests have been updated to illustrate that UploadedFile is now serializable.

Ref #670

Since we're inheriting from io.Bytes, this made it harder to serialize
since we couldn't just make it a data class.

So to workaround this we added custom encoder/decoder logic to handle
UploadedFile specifically.

We also needed to add a custom operator for DeepDiff. For whatever
reason deep diff wasn't diffing it correctly, so instead we just perform
a basical equality check here.

Demos/tests have been updated to illustrate that UploadedFile is now
serializable.
Copy link
Collaborator

@wwwillchen wwwillchen left a comment

Choose a reason for hiding this comment

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

One more thing that would be good to test in the current E2E test mesop/components/uploader/e2e/uploader_test.ts is that the actual bytes are correct (and not just the metadata), just to make sure state diffing is working as intended. For example, could we inspect the data URL of the image and make sure it's correct?

)

def give_up_diffing(self, level, diff_instance) -> bool:
if level.t1 != level.t2:
Copy link
Collaborator

Choose a reason for hiding this comment

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

for BytesIO is eq equivalent to using is? If so, it may be clearer to use is to show that this is comparing whether the objects are the same identity.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't realize there existed an "is" comparator. It seems to compare that it is the same object. But I think when we're doing the diff, there are cases where the object would not be the same exact object since we do a deep copy for tracking the old state.

But maybe I'm not understanding what you're getting at.

mesop/dataclass_utils/diff_state_test.py Show resolved Hide resolved
@richard-to
Copy link
Collaborator Author

Updated e2e test with check for the entire data url to make sure it matches what we expect.

@richard-to richard-to merged commit a2681d5 into google:main Jul 29, 2024
3 checks passed
@richard-to richard-to deleted the uploaded-file-serialize branch August 4, 2024 18:03
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