Skip to content

custom-components#985

Merged
omerXfaruq merged 7 commits into
blocks-devfrom
custom-components
Apr 14, 2022
Merged

custom-components#985
omerXfaruq merged 7 commits into
blocks-devfrom
custom-components

Conversation

@omerXfaruq
Copy link
Copy Markdown
Contributor

  • create template components
  • changes in PKG and requires comes from scripts/install_gradio.sh

closes #731

- create template components
- changes in PKG and requires comes from scripts/install_gradio.sh
Comment thread gradio/__init__.py
Comment thread gradio.egg-info/PKG-INFO
Comment thread demo/blocks_templates/run.py Outdated
Comment thread demo/blocks_templates/run.py Outdated
Comment thread gradio/templates.py Outdated
@abidlabs
Copy link
Copy Markdown
Member

A general point I want to make:

Now that we have templates, we should remove the get_shortcut_implementations() method to avoid defining parameters twice. We can simply adapt get_component_instance() to search through both component names and template names.

But we have an internal discrepancy, which is that gr.Textbox() creates a textbox with 1 line, whereas "textbox" creates a textbox with 7 lines. I think we should resolve this discrepancy by:

  • letting "textbox" only create a textbox with 1 line (breaks backwards compatibility, but in a very small way, and we are changing the design of components anyways)
  • creating a Textarea template and "textarea" string shortcut that creates textboxes with 7 lines (as mentioned in my inline comment)

Also, we should create a separate template for Mic that is the same as Microphone for backwards compatibility

@omerXfaruq
Copy link
Copy Markdown
Contributor Author

omerXfaruq commented Apr 13, 2022

A general point I want to make:

Now that we have templates, we should remove the get_shortcut_implementations() method to avoid defining parameters twice. We can simply adapt get_component_instance() to search through both component names and template names.

But we have an internal discrepancy, which is that gr.Textbox() creates a textbox with 1 line, whereas "textbox" creates a textbox with 7 lines. I think we should resolve this discrepancy by:

* letting "textbox" only create a textbox with 1 line (breaks backwards compatibility, but in a very small way, and we are changing the design of components anyways)

* creating a `Textarea` template and "textarea" string shortcut that creates textboxes with 7 lines (as mentioned in my inline comment)

Also, we should create a separate template for Mic that is the same as Microphone for backwards compatibility

The only reason we support string shortcuts is for backwards compatibility. So I wouldn't consider this as a discrepancy and would not hurt to leave it like this. That's why I dropped some of the shortcuts like text and mic. That's also why we should not edit string shortcuts IMO.
Btw we can drop string shortcuts whenever we want in the future.

- fix get_block_name
- add webcam demo
@omerXfaruq
Copy link
Copy Markdown
Contributor Author

Good to merge if everything is ok!

@abidlabs
Copy link
Copy Markdown
Member

abidlabs commented Apr 13, 2022

The only reason we support string shortcuts is for backwards compatibility

Not really -- string shortcuts for the Interface class will continue to be useful and we should continue supporting this.

That being said, we can leave the get_shortcut_implementations() method as is for now.

Comment thread gradio/blocks.py Outdated
{
"id": _id,
"type": block.__class__.__name__.lower(),
"type": (
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This could be block.get_block_name() given the method that was just added to the Blocks class

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

good catch!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

well I actually mainly created the function to use here, but it seems I forgot, thx for the careful review.

@abidlabs
Copy link
Copy Markdown
Member

Sorry GitHub was being weird for me, and I think some of my comments above got duplicated.

- make use of get_block_name function whenever possible
@omerXfaruq
Copy link
Copy Markdown
Contributor Author

Good to go! Updating get_shortcut_implementations in another branch.

Comment thread gradio/routes.py
input_names = [type(inp).__name__ for inp in app.blocks.input_components]
output_names = [type(out).__name__ for out in app.blocks.output_components]
input_names = [inp.get_block_name() for inp in app.blocks.input_components]
output_names = [out.get_block_name() for out in app.blocks.output_components]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice

@abidlabs
Copy link
Copy Markdown
Member

Yeah, so this looks good. Though I still think we should change:

import gradio.templates as Templates

to

from gradio.templates import Webcam, Sketchpad, ..

@omerXfaruq
Copy link
Copy Markdown
Contributor Author

Yeah, so this looks good. Though I still think we should change:

import gradio.templates as Templates

to

from gradio.templates import Webcam, Sketchpad, ..

Adding @aliabid94 and @pngwn as it seems we cannot reach to an agreement :)

I am not in favor of importing templates via

import gradio as gr
gr.Webcam()

Related comment.

I prefer these

from gradio import Templates

sketchpad = Templates.Sketchpad()
from gradio.Templates import Sketchpad

sketchpad = Sketchpad()

Btw I am okay with templates instead of Templates as well, but I think Templates look way more nicer.

@aliabid94
Copy link
Copy Markdown
Contributor

Sorry for the late comment on this. I do not understand the purpose here. Are we trying to encourage the use of these Template components? I think it's going to be very confusing for a user to have both Components and Templates in the same namespace. When should they use gr.Image(type="pil"), and when should they use gr.PIL()? And if they use gr.PIL, how do they find the documentation for the other constructor arguments - do they go to gr.Image, or is the documentation duplicated entirely for this gr.PIL class?

Using naming format gr.Template.PIL creates a distinction between Templates and regular Components, but having gr.Template.PIL() is such a long name, it does not save enough time compared to gr.Image(source="pil") to be worth it. It also just adds a lot of duplicate documentation.

I really think having these Templates is going to create much more confusion down the line. If we want to support string shortcuts, then they should remain strings. Creating extended subclasses, when in most cases the extension only sets a single keyword argument is just making two ways to do the same thing, which only adds confusion. It's true that the original string shortcuts also introduced two ways to do the same thing, but because the strings were short and they could not be configured like classes, it was clear when to use the string shortcut and when the use the Component class.

If we want to keep string shortcuts, I would be maybe be okay with something like gr.get("webcam").

Happy to discuss in person.

@omerXfaruq
Copy link
Copy Markdown
Contributor Author

omerXfaruq commented Apr 14, 2022

Hmm, I actually agree with some of your points. Though the reason we have these templates is because we want to support previous use of string shortcuts in Blocks(which is I am against it actually, we should drop the shortcuts and make people use components like you mentioned because of the points you gave above). Other than we can't use string shortcuts in blocks,.

Btw having shortcuts for Webcam, Microphone and Sketchpad are handy but PIL and etc. are not.

@omerXfaruq
Copy link
Copy Markdown
Contributor Author

omerXfaruq commented Apr 14, 2022

Btw for shortcutting in Blocks we can make use of this function in the other PR:


from gradio.components import get_component_shortcut

mic = get_component_shortcut("mic")
text= get_component_shortcut("textbox")
dropdown= get_component_shortcut("dropdown")
json = get_component_shortcut("json")



@abidlabs
Copy link
Copy Markdown
Member

I'm persuaded as well.

Ok so let's just stick to using the get_component_shortcut() method instead of defining templates.

@farukozderim we can still keep the demos here, let's just update them to use get_component_shortcut() and we can focus on other things.

@omerXfaruq
Copy link
Copy Markdown
Contributor Author

Will update in #995

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