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 bug with gr.update and interactive=True #2639

Merged
merged 9 commits into from
Nov 15, 2022

Conversation

freddyaboulton
Copy link
Collaborator

@freddyaboulton freddyaboulton commented Nov 10, 2022

Description

Fixes #2636

The original bug is fixed by modifying Block.get_specific_update to not pass "mode" as a kwarg to update as that's not a user facing argument.

In addition, I made it so that the default value of interactive in update methods is True. This is because if an update method sets interactive=False the user has to explicitly set interactive=True again in a separate update call if they want to make the component interactive.

For example, in the original repro, if the default value of interactive in the textbox update method is None, after clicking on short the textbox will never be interactive again. I figured this might be too confusing for users. Wondering what others thoughts are. See the example below:

Cant type in "long" after clicking on "short" if the default value of interactive is None

interactivity

Can type in "long" a

interactivity_2
fter clicking on "short" if the default value of interactive is None

Repro code:

```python

def change_textbox(choice):
    if choice == "short":
        return gr.Textbox.update(lines=2, visible=True, interactive=False)
    elif choice == "long":
        return gr.Textbox.update(lines=8, visible=True)
    else:
        return gr.Textbox.update(visible=False)


with gr.Blocks() as demo:
    radio = gr.Radio(
        ["short", "long", "none"], label="What kind of essay would you like to write?"
    )
    text = gr.Textbox(lines=2, interactive=True)
    gr.Textbox(value="Foo")

    radio.change(fn=change_textbox, inputs=radio, outputs=text)

demo.launch()

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.

@github-actions
Copy link
Contributor

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

@abidlabs
Copy link
Member

abidlabs commented Nov 10, 2022

Thanks for the fix @freddyaboulton!

I'm trying to understand this part:

In addition, I made it so that the default value of interactive in update methods is True. This is because if an update method sets interactive=False the user has to explicitly set interactive=True again in a separate update call if they want to make the component interactive.

For example, in the original repro, if the default value of interactive in the textbox update method is None, after clicking on short the textbox will never be interactive again. I figured this might be too confusing for users. Wondering what others thoughts are. See the example below:

Why are passing in a non-None value of interactive by default? Can't we leave the interactivity of the component to be whatever it was before the gr.update? That's how we do it for every other property. If I understand what you're saying, it would be very confusing to a user who is changing one property to notice that the interactivity of the component has also changed as a result of .gr.update()

@freddyaboulton
Copy link
Collaborator Author

freddyaboulton commented Nov 10, 2022

Why are passing in a non-None value of interactive by default? Can't we leave the interactivity of the component to be whatever it was before the gr.update?

@abidlabs I thought it was maybe a bit confusing that in the first gif, the "long" version of the textbox is not interactive after you click on the "short" button.

If we keep the default value of interactive to None, then the demo author will have to set the "long" textbox to be interactive manually:

import gradio as gr


def change_textbox(choice):
    if choice == "short":
        return gr.Textbox.update(lines=2, visible=True, interactive=False)
    elif choice == "long":
        return gr.Textbox.update(lines=8, visible=True, interactive=True)
    else:
        return gr.Textbox.update(visible=False)


with gr.Blocks() as demo:
    radio = gr.Radio(
        ["short", "long", "none"], label="What kind of essay would you like to write?"
    )
    text = gr.Textbox(lines=2, interactive=True)
    gr.Textbox(value="Foo")

    radio.change(fn=change_textbox, inputs=radio, outputs=text)

demo.launch()

Which I think is ok but I'm worried it will confuse users who are used to thinking of output components as being interactive by default.

@freddyaboulton freddyaboulton marked this pull request as ready for review November 11, 2022 16:35
gradio/blocks.py Outdated
Comment on lines 265 to 266
mode = generic_update.get("mode", None)
generic_update.pop("mode", None)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
mode = generic_update.get("mode", None)
generic_update.pop("mode", None)
mode = generic_update.pop("mode", None)

@abidlabs
Copy link
Member

Thanks for the fix @freddyaboulton! I tested and it works. However, I think there is another way to do it which avoids an unnecessary method call.

Right now, we have the following chain of methods that are being called:

  • User calls in gr.Textbox.update(...), which returns a dict with key __type__ set to update and replaces the interactive key with mode
  • In postprocess_update_dict() (in blocks.py), we call get_specific_update() which is meant to convert a "generic update" (i.e. gr.update()) to a component-specific update() dict
  • However, the component-specific update() cannot handle the mode key anymore (since it is expecting the interactive parameter), so we see an error

Although this fixes the problem by extracting the mode key before calling the component-specific update(), the component-specific update() is still being called twice.

It would be sufficient to add an if condition to postprocess_update_dict() as follows to only call the component-specific update() if indeed we have a generic update, like this:

def postprocess_update_dict(block: Block, update_dict: Dict, postprocess: bool = True):
    """
    Converts a dictionary of updates into a format that can be sent to the frontend.
    E.g. {"__type__": "generic_update", "value": "2", "interactive": False}
    Into -> {"__type__": "update", "value": 2.0, "mode": "static"}

    Parameters:
        block: The Block that is being updated with this update dictionary.
        update_dict: The original update dictionary
        postprocess: Whether to postprocess the "value" key of the update dictionary.
    """
    if update_dict.get("__type__", "") == "generic_update":
        update_dict = block.get_specific_update(update_dict)
    if update_dict.get("value") is components._Keywords.NO_VALUE:
        update_dict.pop("value")
    update_dict = delete_none(update_dict, skip_value=True)
    if "value" in update_dict and postprocess:
        update_dict["value"] = block.postprocess(update_dict["value"])
    return update_dict

@freddyaboulton
Copy link
Collaborator Author

@abidlabs Thank you for the very thoughtful review! I followed your suggestion. Should be good for another review now.

Was there a reason we were calling the component specific update twice? Was it a way to create a copy of the dict before deleting keys? I wonder if I should also copy the dict in postprocess_update_dict to match the previous behavior as closely as possible.

@abidlabs
Copy link
Member

Was there a reason we were calling the component specific update twice?

No I'm pretty sure I know what happened, basically it was a mistake I made when I was refactoring the code

@abidlabs
Copy link
Member

@freddyaboulton you can completely revert all changes to the get_specific_update() method, right?

@freddyaboulton freddyaboulton force-pushed the 2636-fix-interactive-update branch 2 times, most recently from 93b71e0 to f77b67c Compare November 14, 2022 22:12
@freddyaboulton
Copy link
Collaborator Author

@abidlabs Yes, you are right! Reverted the changes to get_specific_update and updated the unit tests!

@abidlabs
Copy link
Member

Thank you @freddyaboulton for fixing this, looks great!

@freddyaboulton freddyaboulton merged commit e6336d6 into main Nov 15, 2022
@freddyaboulton freddyaboulton deleted the 2636-fix-interactive-update branch November 15, 2022 16:53
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.

gr.Update doesn't work for the interactive property
2 participants