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

Fix video serializing in client #3860

Merged
merged 3 commits into from
Apr 15, 2023
Merged

Fix video serializing in client #3860

merged 3 commits into from
Apr 15, 2023

Conversation

freddyaboulton
Copy link
Collaborator

Description

Video components in latest gradio version were not able to be serialized because:

  1. VideoSerializer was not included in Serializer map. Solution is to include subclasses as well.
  2. There was an isinstance(x, tuple) check in deserialize but tuples don't exist in json so when we load the json they get converted to lists.

Should fix the tests on main.

Checklist:

  • I have performed a self-review of my own code
  • I have added a short summary of my change to the CHANGELOG.md
  • My code follows the style guidelines of this project
  • I have commented my code in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

A note about the CHANGELOG

Hello 👋 and thank you for contributing to Gradio!

All pull requests must update the change log located in CHANGELOG.md, unless the pull request is labeled with the "no-changelog-update" label.

Please add a brief summary of the change to the Upcoming Release > Full Changelog section of the CHANGELOG.md file and include
a link to the PR (formatted in markdown) and a link to your github profile (if you like). For example, "* Added a cool new feature by [@myusername](link-to-your-github-profile) in [PR 11111](https://github.com/gradio-app/gradio/pull/11111)".

If you would like to elaborate on your change further, feel free to include a longer explanation in the other sections.
If you would like an image/gif/video showcasing your feature, it may be best to edit the CHANGELOG file using the
GitHub web UI since that lets you upload files directly via drag-and-drop.

@gradio-pr-bot
Copy link
Collaborator

All the demos for this PR have been deployed at https://huggingface.co/spaces/gradio-pr-deploys/pr-3860-all-demos

@freddyaboulton freddyaboulton force-pushed the fix-video-serializing branch from 0d3edb5 to 41ce0b3 Compare April 14, 2023 20:19
@freddyaboulton freddyaboulton marked this pull request as ready for review April 14, 2023 20:41
@abidlabs
Copy link
Member

Thanks @freddyaboulton! Will review properly in a few hours when I'm back at my computer. But earlier today, I tested a Space with a video output, and it seemed to work fine:

from gradio_client import Client

c = Client("abidlabs/cinemascope")
c.predict("a cat jumping")

Is this PR fixing something else?

@freddyaboulton
Copy link
Collaborator Author

The issue is only affecting gradio 3.26.0 and above I believe. CI is red on main right now because VideoSerializable is not present in the serializer dict!

SERIALIZER_MAPPING = {cls.__name__: cls for cls in Serializable.__subclasses__()}
SERIALIZER_MAPPING = {}
for cls in Serializable.__subclasses__():
SERIALIZER_MAPPING[cls.__name__] = cls
Copy link
Member

@abidlabs abidlabs Apr 14, 2023

Choose a reason for hiding this comment

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

Ideally we traverse all descendant subclasses recursively

@abidlabs
Copy link
Member

Haven’t tested but code looks good, feel free to merge

@freddyaboulton
Copy link
Collaborator Author

No rush!

@abidlabs
Copy link
Member

Tested, works great on old and new Spaces with videos, thanks @freddyaboulton! Let's release a new version of the gradio client to get this out

@abidlabs abidlabs merged commit 7ef0439 into main Apr 15, 2023
@abidlabs abidlabs deleted the fix-video-serializing branch April 15, 2023 00:07
@pngwn pngwn mentioned this pull request Jul 26, 2023
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.

3 participants