gradio-app / gradio

Build and share delightful machine learning apps, all in Python. 🌟 Star to support our work!
http://www.gradio.app
Apache License 2.0
29.84k stars 2.22k forks source link

Blocks components #765

Closed omerXfaruq closed 2 years ago

omerXfaruq commented 2 years ago

Combine all input output and static components under components. A huge refactoring, detailed review would be great!

Fixes: #639

abidlabs commented 2 years ago

This generally looks like a promising approach to me!

However, specifically for the Textbox, there are some changes that need to be made. The reason for that is the type is deprecated for the inputs.Textbox but not for the outputs.Textbox where it is important.

So what we should do is to accept the type parameter but have its behavior mimic the behavior in outputs.Textbox constructor.

I'll add specific comments in the code to show what I mean

aliabid94 commented 2 years ago

I put these thoughts in gradio-technical channel but probably more appropriate here:

~ ~ ~

~ ~ ~

Based on these thoughts, would be great if all components.* classes had default= kwarg first!

aliabid94 commented 2 years ago

Also, how do we keep backward compatibility with any code that may have specified Component arguments without keywords? (this is something we can disable if we want). So for example, if someone uses gr.inputs.Image((200,200), "RGB") instead of gr.inputs.Image(shape=(200,200), color_mode="RGB"). Abubakar's suggestion of keeping inputs.Textbox and having that call components.Textbox would solve this issue.

omerXfaruq commented 2 years ago

I am not in favor of having different initilizations for the input or output, I think that makes combining them meaningless and would prefer having a NewComponent type for blocks instead of this. People will be able to reference Textbox through components, inputs and outputs. And use it both as an input or output.

However if are going to create code duplication by having different classes for input and output let's have NewComponents and don't break anything that Interface uses.

If we are going to combine components input and output should merge into one. That's the whole point. I think having different initilizations misses and loses this point.

These are my thoughts, WDYT?

omerXfaruq commented 2 years ago

Also, how do we keep backward compatibility with any code that may have specified Component arguments without keywords? (this is something we can disable if we want). So for example, if someone uses gr.inputs.Image((200,200), "RGB") instead of gr.inputs.Image(shape=(200,200), color_mode="RGB"). Abubakar's suggestion of keeping inputs.Textbox and having that call components.Textbox would solve this issue.

We are changing our component structure & design. I think it is ok to move to only keyword parameters except default parameter. We can warn the users about this.

abidlabs commented 2 years ago

I am not in favor of having different initilizations for the input or output, I think that makes combining them meaningless

I don't think it makes it meaningless. We still avoid significant code duplication and create a single component that is the preferred way of instantiating components in the future. However, what @aliabid94 is saying is how we can preserve backwards compatibility and reduce the amount of code that break. We will update our docs to use the new components.py module, but there's a lot of existing code / tutorials / examples etc out there. Breaking that would be a bad experience for our users and create a lot issues that we should avoid.

Here's how it would like for the Textbox class (roughly).

inputs.py

class Textbox(InputComponent):
    """
    Component creates a textbox for user to enter input. Provides a string as an argument to the wrapped function.
    Input type: str
    Demos: hello_world, diff_texts
    """

    def __init__(
        self,
        lines: int = 1,
        placeholder: Optional[str] = None,
        default: str = "",
        numeric: Optional[bool] = False,
        type: Optional[str] = "str",
        label: Optional[str] = None,
        optional: bool = False,
    ):
        if numeric or type == "number":
            warnings.warn(
                "The 'numeric' type has been deprecated and this parameter has no effect. Use the Number input component instead.",
                DeprecationWarning,
            )
        return components.Textbox(lines=lines, 
                                  placeholder=placeholder, 
                                  default=default, 
                                  label=label,
                                  optional=optional)

outputs.py

class Textbox(OutputComponent):
    """
    Component creates a textbox to render output text or number.
    Output type: Union[str, float, int]
    Demos: hello_world, sentence_builder
    """

    def __init__(self, type: str = "auto", label: Optional[str] = None):
        """
        Parameters:
        type (str): Type of value to be passed to component. "str" expects a string, "number" expects a float value, "auto" detects return type.
        label (str): component name in interface.
        """
        return components.Textbox( 
                                  type=type, 
                                  label=label)

components.py

The full code for the components class

This way we achieve merged components with minimal code duplication and no code breakage.

abidlabs commented 2 years ago

We are changing our component structure & design. I think it is ok to move to only keyword parameters except default parameter. We can warn the users about this.

Why do this? We should avoid unnecessary code breakage as much as possible

omerXfaruq commented 2 years ago

I am not in favor of having different initilizations for the input or output, I think that makes combining them meaningless

I don't think it makes it meaningless. We still avoid significant code duplication and create a single component that is the preferred way of instantiating components in the future. However, what @aliabid94 is saying is how we can preserve backwards compatibility and reduce the amount of code that break. We will update our docs to use the new components.py module, but there's a lot of existing code / tutorials / examples etc out there. Breaking that would be a bad experience for our users and create a lot issues that we should avoid.

Here's how it would like for the Textbox class (roughly).

inputs.py

class Textbox(InputComponent):
    """
    Component creates a textbox for user to enter input. Provides a string as an argument to the wrapped function.
    Input type: str
    Demos: hello_world, diff_texts
    """

    def __init__(
        self,
        lines: int = 1,
        placeholder: Optional[str] = None,
        default: str = "",
        numeric: Optional[bool] = False,
        type: Optional[str] = "str",
        label: Optional[str] = None,
        optional: bool = False,
    ):
        if numeric or type == "number":
            warnings.warn(
                "The 'numeric' type has been deprecated and this parameter has no effect. Use the Number input component instead.",
                DeprecationWarning,
            )
        return components.Textbox(lines=lines, 
                                  placeholder=placeholder, 
                                  default=default, 
                                  label=label,
                                  optional=optional)

outputs.py

class Textbox(OutputComponent):
    """
    Component creates a textbox to render output text or number.
    Output type: Union[str, float, int]
    Demos: hello_world, sentence_builder
    """

    def __init__(self, type: str = "auto", label: Optional[str] = None):
        """
        Parameters:
        type (str): Type of value to be passed to component. "str" expects a string, "number" expects a float value, "auto" detects return type.
        label (str): component name in interface.
        """
        return components.Textbox( 
                                  type=type, 
                                  label=label)

components.py

The full code for the components class

This way we achieve merged components with minimal code duplication and no code breakage.

Though this is not just about code duplication. It creates component duplication as well. When users are going to create a Textbox component they can create it via 3 ways.

omerXfaruq commented 2 years ago

We are changing our component structure & design. I think it is ok to move to only keyword parameters except default parameter. We can warn the users about this.

Why do this? We should avoid unnecessary code breakage as much as possible

Mainly to support Ali's suggestion. While changing our design, it is a good idea to move into keyword only parameters to block any problems that could generate with positional parameters.

omerXfaruq commented 2 years ago

With an offline chat we decided on:

omerXfaruq commented 2 years ago

With the latest commit xray demo is working, you can take a look there.

From now on I will stop supporting component_type in the config, with the new component design.

abidlabs commented 2 years ago

Very small nit. Can we rename component.py to be components.py (the same way inputs and outputs and strings and routes are all pluralized)?

It also reads better to have:

from gradio.components import Textbox, Image, etc

omerXfaruq commented 2 years ago

Very small nit. Can we rename component.py to be components.py (the same way inputs and outputs and strings and routes are all pluralized)?

It also reads better to have:

from gradio.components import Textbox, Image, etc

That's actually is imported like that, could change name of the file as well :)

abidlabs commented 2 years ago

That's actually is imported like that, could change name of the file as well :)

Yup I think refactoring it to be components.py would be nice

abidlabs commented 2 years ago

Regarding components like Image who have both an input type and and output type, how should we handle the parameter naming? The simplest approach would be to have Image take both an input_type and output_type parameter.

Some thoughts:

Cc @FarukOzderim @aliabid94 @pngwn

omerXfaruq commented 2 years ago

Regarding components like Image who have both an input type and and output type, how should we handle the parameter naming? The simplest approach would be to have Image take both an input_type and output_type parameter.

Some thoughts:

* For the sake of consistency, should we name all of the parameters, even for components that only take one `type`, either `input_type` or `output_type`? So `components.Textbox` would take an `output_type` instead of `type`

  * Disadvantage: longer parameter name
  * Advantage: consistent API (no need to look up what the parameter is called) + the purpose of parameter is clearer

* Of course, we shouldn't rename the parameters in `inputs.Textbox` or `outputs.Textbox` for reverse compatibility

Cc @FarukOzderim @aliabid94 @pngwn

I feel like having different types for input and output could complexify it. And would not be so different to having inputs and outputs independently. Why not combine them together and take the same type for both input and output? type=pilmeans input_type and output_type is pil

Btw we could allow users to have different types for inputs and outputs with optional parameters as well.

abidlabs commented 2 years ago

I feel like having different types for input and output could complexify it. And would not be so different to having inputs and outputs independently. Why not combine them together and take the same type for both input and output? type=pil means input_type and output_type is pil Btw we could allow users to have different types for inputs and outputs with optional parameters as well.

So to clarify, what this would look like is: we have a single type parameter, which applies to both the input type and output type by default. If someone needs to have different input type and output type, they use the optional input_type or output_type parameters?

I think this is fine as it simplifies the most common use cases, but allows flexibility in case we need separate input and output types. (We definitely do need the optional input_type and output_type parameters, as these cases do come up, and for backwards compatibility). cc @aliabid94

omerXfaruq commented 2 years ago

Yeah!

Btw I think having just output_type would be enough, and type would work as input_type, but it works both ways!

abidlabs commented 2 years ago

You've probably seen this @FarukOzderim but the CircleCI tests aren't running because of circular imports

abidlabs commented 2 years ago

Ok so as I've been reviewing the code, I'm realizing the approach of using type to refer to both input type and output type by default is not going to work @FarukOzderim.

The reason is the "auto" parameter value we use for the output type for many of the components (e.g. Image, Label, and many others). "auto" means that the Output component's type should be determined based on inspecting the value that is provided to it. For example, if a "pil" image is provided, then it understands it should apply a postprocessing function that converts "pil" images to base64. This is super handy is it allows users to basically "not worry" about output types and only provide "type" to output components if they want to restrict the output component in some way. That is why for all of the output components that support the "auto" type, it is the default.

Now "auto" can be an output type, but there is no way to make it an "input" type since we cannot know a priori what format a user expects the input data to look like for their prediction function. They have to tell us.

If type refers to both "input_type" and "output_type", this leads us to several problems...

The only (clean) solution I can think of is to have separate input_type (which doesn't allow "auto") and output_type (which does allow "auto")

Let me know you wanna sync about this @FarukOzderim. Happy to review the rest of the code, but we should get this sorted out first.

aliabid94 commented 2 years ago

How about getting rid of output_type? It can be "auto" always so we determine it ourselves, and then we just have kwarg "type" which refers to the desired input type.

abidlabs commented 2 years ago

Huh that actually could work. I skimmed through the output docs and we should always be able to tell what the right output type is by looking at the returned value

The only exception is the Video output which has an interesting type parameter. But that parameter should really be called something else, like “output_format”

omerXfaruq commented 2 years ago

You've probably seen this @FarukOzderim but the CircleCI tests aren't running because of circular imports

The complete version is on the gradio-dash-fe branch. We shall continue branching or updating from that branch.

omerXfaruq commented 2 years ago

You've probably seen this @FarukOzderim but the CircleCI tests aren't running because of circular imports

The complete version is on the gradio-dash-fe branch. We shall continue branching or updating from that branch.

Never mind it, I am going to merge that branch here.

omerXfaruq commented 2 years ago

How about getting rid of output_type? It can be "auto" always so we determine it ourselves, and then we just have kwarg "type" which refers to the desired input type.

On which components would you like to have this feature, not necessary to have it on all of them I suppose?

Only these?

abidlabs commented 2 years ago

It should be all of the components whose output forms currently have a type parameter. It looks like that is:

We no longer need this parameter and instead should just use the "auto" logic to infer what the output type is.

aliabid94 commented 2 years ago

default_value seems like a good name. And agree with removing type for the components @abidlabs listed above

omerXfaruq commented 2 years ago

Btw I think we should remove support for text shortctus and create prebuilt components for that. Related discussion #731

omerXfaruq commented 2 years ago

I am merging this PR to work with smaller PRs in the future, please highlight anything if not discussed yet.

abidlabs commented 2 years ago

Great job, this was a herculean effort!