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

Removing .update and get_config, attempt 2 #5240

Merged
merged 79 commits into from
Sep 19, 2023
Merged

Removing .update and get_config, attempt 2 #5240

merged 79 commits into from
Sep 19, 2023

Conversation

aliabid94
Copy link
Collaborator

Trying a different approach (much simpler!!!) to removing get_config and update.

get_config is removed, the config used is simply any attribute that is in the Block that shares a name with one of the constructor paramaters.

update is not removed for backwards compatibility, but deprecated. Instead return the component itself. Created a updateable decorator that simply checks to see if we're in an update, and if so, skips the constructor and wraps the args and kwargs in an update dictionary. easy peasy.

@gradio-pr-bot
Copy link
Collaborator

gradio-pr-bot commented Aug 15, 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
📓 Notebooks matching!

Install Gradio from this PR

pip install https://gradio-builds.s3.amazonaws.com/e7abb0c95d7a0cd37048170982018d5f440a5134/gradio-3.44.4-py3-none-any.whl

Install Gradio Python Client from this PR

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

@gradio-pr-bot
Copy link
Collaborator

gradio-pr-bot commented Aug 15, 2023

🦄 change detected

This Pull Request includes changes to the following packages.

Package Version
gradio minor

With the following changelog entry.

⚠️ Warning invalid changelog entry.

Changelog entry must be either a paragraph or a paragraph followed by a list:

<type>: <description>

Or

<type>:<description>

- <change-one>
- <change-two>
- <change-three>

If you wish to add a more detailed description, please created a highlight entry instead.

⚠️ The changeset file for this pull request has been modified manually, so the changeset generation bot has been disabled. To go back into automatic mode, delete the changeset file.

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.

@abidlabs
Copy link
Member

We should update this section in the Guides: https://www.gradio.app/guides/blocks-and-event-listeners#updating-component-configurations

Copy link
Collaborator

@freddyaboulton freddyaboulton left a comment

Choose a reason for hiding this comment

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

The new update logic is awesome. Definitely much simpler and I think will work well with custom components.

I see a shortcoming with the proposed get_config change in that it won't get the props from a parent class. For example, Chatbot currently doesn't list the selectable prop in the config but before it did. Also there are some component props that are not listed in the init constructor like bokeh_version for the plots. Will think about this today.

gradio/blocks.py Outdated
return hasattr(context.thread_data, "blocks")


def updateable(fn):
Copy link
Collaborator

@freddyaboulton freddyaboulton Aug 16, 2023

Choose a reason for hiding this comment

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

Maybe we can automatically wrap the init with updateable in a metaclass so that custom component authors don't even have to worry about this when creating their own components 👀

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

was trying to think how to do this. Not familiar with metaclasses, will take a look!

@abidlabs
Copy link
Member

I see a shortcoming with the proposed get_config change in that it won't get the props from a parent class. For example, Chatbot currently doesn't list the selectable prop in the config but before it did. Also there are some component props that are not listed in the init constructor like bokeh_version for the plots. Will think about this today.

Good point. Just to add -- this is actually a potential bug currently in gradio because we use the properties in the config to recreate components when using gr.load() with Spaces. So right now, if you were to gr.load() a Space that has a gr.Plot, it would raise an unused kwarg warning because it would try to pass in bokeh_version. We're planning on turning these warnings into exceptions in 4.0, so it could be problematic independent of this PR.

@abidlabs
Copy link
Member

abidlabs commented Aug 16, 2023

Agree with @freddyaboulton's suggestions. Tested .update() out and just wanted to confirm a few things:

update is not removed for backwards compatibility, but deprecated

I don't think its been deprecated yet just fyi. I don't see any warnings when using the Component.update() methods

In your previous PR, you had implemented:

Any argument that is in the constructor that becomes a component property will be considered part of the config and sent to the backend (open to other logic here)

Just to confirm, sending configs to the backend so that they affect the preprocessing/postprocessing is not part of this PR, right? E.g.

import gradio as gr

def update_image_type():
    print("updated")
    return gr.Image(type="filepath")

with gr.Blocks() as demo:
    i = gr.Image()
    t = gr.Textbox()
    btn = gr.Button()
    btn.click(update_image_type, None, i)
    i.change(lambda x:x, i, t)
    
demo.launch()

does not work

  1. Btw it would be great to use the blocks_essay demo as part of a functional test to ensure that updating component properties works as expected as we're working on it. Updating props isn't something that we test e2e despite it being a very import flow

@aliabid94
Copy link
Collaborator Author

aliabid94 commented Aug 16, 2023

I see a shortcoming with the proposed get_config change in that it won't get the props from a parent class. For example, Chatbot currently doesn't list the selectable prop in the config but before it did. Also there are some component props that are not listed in the init constructor like bokeh_version for the plots. Will think about this today.

We could still have components implement their own get_configs, which return the extra parameters plus a call to super()? Something like

def get_config(self):
  return {
    "selectable": self.selectable,
    ...super().get_config()
  }

I don't think its been deprecated yet just fyi.

Yes sorry not planning on making the deprecation as part of this PR because of the next comment

Just to confirm, sending configs to the backend so that they affect the preprocessing/postprocessing is not part of this PR, right?

Yep, I'll do this in a subsequent PR, trying to keep things smaller and more testable. So for that reason, not deprecating or even adding this to the guide yet.

@freddyaboulton
Copy link
Collaborator

We could still have components implement their own get_configs, which return the extra parameters plus a call to super()?

Yea I think that makes sense! Although in the specific case of selectable, I think it should be added to the config automatically if the component is selectable without users having to worry about it.

Copy link
Member

Choose a reason for hiding this comment

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

I need to make the changeset thing support multiline comments for fix + feat. I'll do that tomorrow, should be quick.

@aliabid94
Copy link
Collaborator Author

For example, Chatbot currently doesn't list the selectable prop in the config but before it did. Also there are some component props that are not listed in the init constructor like bokeh_version for the plots

Overrode get_config in these two cases to restore these config variables. Scanning through the rest of the components I couldn't find any other cases where the config had additional fields.

@abidlabs
Copy link
Member

I'm still seeing an error with gr.LinePlot -- you can see it erroring in demo/blocks_multiple_event_triggers/run.py.

Once the plot components are fixed and tested -- should be good to merge in!

@abidlabs
Copy link
Member

Still seeing one issue @aliabid94 -- will Slack you

@abidlabs abidlabs added the v: patch A change that requires a patch release label Sep 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v: patch A change that requires a patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants