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

Lite: Make the Examples component display media files using pseudo HTTP requests to the Wasm server #5627

Merged
merged 14 commits into from Oct 16, 2023

Conversation

whitphx
Copy link
Member

@whitphx whitphx commented Sep 20, 2023

Description

Alternative implementation of the media components in the Wasm mode, based on the discussions in #4849.

🎯 PRs Should Target Issues

Before your create a PR, please check to see if there is an existing issue for this change. If not, please create an issue before you create this PR, unless the fix is very small.

Not adhering to this guideline will result in the PR being closed.

Tests

  1. PRs will only be merged if tests pass on CI. To run the tests locally, please set up your Gradio environment locally and run the tests: bash scripts/run_all_tests.sh

  2. You may need to run the linters: bash scripts/format_backend.sh and bash scripts/format_frontend.sh

@gradio-pr-bot
Copy link
Contributor

gradio-pr-bot commented Sep 20, 2023

🪼 branch checks and previews

Name Status URL
Spaces ready! Spaces preview
Website ready! Website preview
Storybook ready! Storybook preview
Visual tests all good! Build review
🦄 Changes detected! Details

Install Gradio from this PR

pip install https://gradio-builds.s3.amazonaws.com/8b08528552cb42c837bb694b2fbd88e717c468fc/gradio-3.47.1-py3-none-any.whl

Install Gradio Python Client from this PR

pip install "gradio-client @ git+https://github.com/gradio-app/gradio@8b08528552cb42c837bb694b2fbd88e717c468fc#subdirectory=client/python"

@gradio-pr-bot
Copy link
Contributor

gradio-pr-bot commented Sep 20, 2023

🦄 change detected

This Pull Request includes changes to the following packages.

Package Version
@gradio/app minor
@gradio/audio minor
@gradio/image minor
@gradio/video minor
@gradio/wasm minor
gradio minor
  • Maintainers can select this checkbox to manually select packages to update.

With the following changelog entry.

Lite: Make the Examples component display media files using pseudo HTTP requests to the Wasm server

Maintainers or the PR author can modify the PR title to modify this entry.

Something isn't right?

  • Maintainers can change the version label to modify the version bump.
  • If the bot has failed to detect any changes, or if this pull request needs to update multiple packages to different versions or requires a more comprehensive changelog entry, maintainers can update the changelog file directly.

class:table={type === "table"}
class:gallery={type === "gallery"}
class:selected
class={[
Copy link
Member Author

Choose a reason for hiding this comment

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

The class: directive can't be used to components 😵‍💫

Any advice? Sorry I'm not so familiar with Svelte convention.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also these styles defined at the parent component don't work...

Copy link
Member

Choose a reason for hiding this comment

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

Yes, svelte scopes styles to ensure component encapsulation.

The best approach here would be not to pass the classes down but to reimplement the styles to apply to a parent span or div, then the image can just have a 100% width/ height and the container can take care of the styling. I don't think we are doing anything too fancy with the example media so this should work well and will avoid the need to pass down the classes.

If we do need to apply style to the image from this component we can use a descendant global selector which will allow us to target the img but not leak out styles:

css.img-wrap :global(> img) {
 ...
}

Copy link
Member Author

@whitphx whitphx Oct 3, 2023

Choose a reason for hiding this comment

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

Thanks! Will try to modify them.

Copy link
Member

@pngwn pngwn left a comment

Choose a reason for hiding this comment

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

Left a few comments, let me know if you have any questions!

class:table={type === "table"}
class:gallery={type === "gallery"}
class:selected
class={[
Copy link
Member

Choose a reason for hiding this comment

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

Yes, svelte scopes styles to ensure component encapsulation.

The best approach here would be not to pass the classes down but to reimplement the styles to apply to a parent span or div, then the image can just have a 100% width/ height and the container can take care of the styling. I don't think we are doing anything too fancy with the example media so this should work well and will avoid the need to pass down the classes.

If we do need to apply style to the image from this component we can use a descendant global selector which will allow us to target the img but not leak out styles:

css.img-wrap :global(> img) {
 ...
}

Copy link
Member

Choose a reason for hiding this comment

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

Lets move this file to @gradio/image so it is available with our other image components, we can add an extra named export to the static subpath. The wasm stuff is an implementation detail that users don't need to be aware of.

Copy link
Member Author

@whitphx whitphx Oct 3, 2023

Choose a reason for hiding this comment

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

OK, I would follow your choice,
while based on the same understanding below I made a different choice here at this point; more or less, this component is an interface encapsulating the Wasm details from users as you know and both make sense to put this in @gradio/image and @gradio/wasm, while my preference was to put Wasm-related implementations in @gradio/wasm. If you prefer to omit the wasm name from all import statements in these modules, I understand it's also an option.

The wasm stuff is an implementation detail that users don't need to be aware of.

Copy link
Member

Choose a reason for hiding this comment

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

When we create components for other media we should do the same, put them alongside the components they relate to. The main reason for this is that when custom components launch these will be user facing, so it would be nice if they don't need to think about wasm/ not wasm when building.

Copy link
Member Author

Choose a reason for hiding this comment

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

Really makes sense. Thanks

Copy link
Member Author

@whitphx whitphx Oct 9, 2023

Choose a reason for hiding this comment

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

Does the shared subpath make more sense?
For example, in the case of @gradio/video, VideoPreview in the static subpath imports Player in shared (static->shared dependecy direction).

js/wasm/svelte/context.ts Outdated Show resolved Hide resolved
js/wasm/svelte/file-url.ts Outdated Show resolved Hide resolved
js/wasm/svelte/index.ts Outdated Show resolved Hide resolved
@whitphx
Copy link
Member Author

whitphx commented Oct 5, 2023

@pngwn I just pushed the changes related to the previous review. Can you check it when you have time? Thanks!
(I will be still working on Audio/Video components as well)

@whitphx whitphx force-pushed the wasm-example-media-files branch 2 times, most recently from 686a749 to fe178a7 Compare October 9, 2023 09:05
@whitphx whitphx requested a review from pngwn October 9, 2023 10:18
@whitphx whitphx marked this pull request as ready for review October 9, 2023 10:18
@whitphx
Copy link
Member Author

whitphx commented Oct 12, 2023

Currently the local dev app (js/app/src/lite/dev/App.svelte) can't add/edit media files.
So, to test the media files capability of this PR, copy code like below to js/app/src/lite/dev/App.svelte so it downloads and mount the media files programatically.

	// Source from https://huggingface.co/spaces/gradio/blocks_inputs/tree/main
	fetch("https://huggingface.co/spaces/gradio/blocks_inputs/resolve/main/lion.jpg")
		.then((response) => response.blob())
		.then((blob) => blob.arrayBuffer())
		.then((arrayBuffer) => {
			controller.write("lion.jpg", new Uint8Array(arrayBuffer), {});
			controller.write(
				"app.py",
				`
import gradio as gr
import os

def combine(a, b):
		return a + " " + b

def mirror(x):
		return x

with gr.Blocks() as demo:

		txt = gr.Textbox(label="Input", lines=2)
		txt_2 = gr.Textbox(label="Input 2")
		txt_3 = gr.Textbox(value="", label="Output")
		btn = gr.Button(value="Submit")
		btn.click(combine, inputs=[txt, txt_2], outputs=[txt_3])

		with gr.Row():
				im = gr.Image()
				im_2 = gr.Image()

		btn = gr.Button(value="Mirror Image")
		btn.click(mirror, inputs=[im], outputs=[im_2])

		gr.Markdown("## Text Examples")
		gr.Examples(
				[["hi", "Adam"], ["hello", "Eve"]],
				[txt, txt_2],
				txt_3,
				combine,
				#cache_examples=True,
		)
		gr.Markdown("## Image Examples")
		gr.Examples(
				examples=[os.path.join(os.path.dirname(__file__), "lion.jpg")],
				inputs=im,
				outputs=im_2,
				fn=mirror,
				#cache_examples=True,
		)

if __name__ == "__main__":
		demo.launch()
`,
				{}
			);
			controller.run_file("app.py");
		});

Copy link
Member

@pngwn pngwn left a comment

Choose a reason for hiding this comment

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

Other than my comment, everything looks great! Thanks @whitphx!

Copy link
Member

Choose a reason for hiding this comment

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

This is the only media component where the interactive version has been changed rather than the example. Is there a reason for that?

Copy link
Member Author

@whitphx whitphx Oct 16, 2023

Choose a reason for hiding this comment

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

It's just because of the codebase structure.
In the case of Audio, example/Audio.svelte doesn't have the <audio /> tag as it only shows the example file name, and modifying interactive/Audio.svelte was needed to make audio apps work instead.
In turn, for example in the case of Video, all Video components uses shared/Player.svelte under the hood, so I just modified it.

FYI, I benchmarked the following 3 spaces to make sure the pair of a media input component and gr.Examples() works.

So there might be missing parts. Will catch them in another PR if found.

@whitphx
Copy link
Member Author

whitphx commented Oct 16, 2023

@pngwn Thank you!!!

@whitphx whitphx merged commit b67115e into main Oct 16, 2023
19 of 20 checks passed
@whitphx whitphx deleted the wasm-example-media-files branch October 16, 2023 16:26
This was referenced Oct 16, 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.

None yet

3 participants