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

Adds an every parameter to every component #2806

Merged
merged 33 commits into from
Dec 15, 2022
Merged

Adds an every parameter to every component #2806

merged 33 commits into from
Dec 15, 2022

Conversation

abidlabs
Copy link
Member

@abidlabs abidlabs commented Dec 13, 2022

As I was writing the BigQuery Guide, I realized that it would be very nice if every component had an every parameter that could be used if the component's initial value was a function. It would allow you to do something like this:

import gradio as gr

with gr.Blocks() as demo:
    df = gr.DataFrame(run_query, every=60*60)

demo.queue().launch() 

And in fact, it would basically make the need for a gr.Panel() abstraction completely unnecessary. Lmk if you guys think this is a good idea @freddyaboulton @aliabid94 @dawoodkhan82. Implementation was straightforward to feel free to push back if this is not a good idea for any reason

@abidlabs abidlabs changed the base branch from bigquery-guide to pd-orjson December 13, 2022 08:08
@abidlabs abidlabs marked this pull request as ready for review December 13, 2022 08:11
@freddyaboulton
Copy link
Collaborator

I like the idea @abidlabs ! Let me think about this some more. The one thing that I don't like is that you wouldn't be able to use that component in any other event because there is no way to cancel the initial load event running on a loop.

So something like this would behave buggy in my opinion, i.e. subsequent events created by slider change would show up alongside the intial load event, creating a flicker.

import pandas as pd
import gradio as gr
import random

def run_query(n_points=10):
    return pd.DataFrame({"data": [random.random() for _ in range(n_points)]})


with gr.Blocks() as demo:
    max_value = gr.Slider(minimum=1, maximum=20, step=2)
    df = gr.DataFrame(run_query, every=1)
    max_value.change(run_query, inputs=max_value, outputs=df, every=1)

demo.queue().launch()

component_every

@abidlabs
Copy link
Member Author

abidlabs commented Dec 13, 2022

You're right, we should expose the event in case a user wants to cancel it or do something else with it -- how about component.load_event becomes the event (we'll document this)

Base automatically changed from pd-orjson to main December 13, 2022 23:01
@abidlabs abidlabs mentioned this pull request Dec 13, 2022
@abidlabs
Copy link
Member Author

Let me know if you see any additional concerns @freddyaboulton. Otherwise, will go ahead and add this in & finalize the PR

@freddyaboulton
Copy link
Collaborator

You're right, we should expose the event in case a user wants to cancel it or do something else with it -- how about component.load_event becomes the event (we'll document this)

Makes sense @abidlabs !

@abidlabs
Copy link
Member Author

Ok sounds good, I'll implement it this way

@github-actions
Copy link
Contributor

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

@abidlabs
Copy link
Member Author

All righty, this should be all set for review. I had to change the way we do load events to expose the event, but everything should be backwards compatible.

Now, you can do something like this:

import pandas as pd
import gradio as gr
import random

def run_query(n_points=10):
    return pd.DataFrame({"data": [random.random() for _ in range(n_points)]})


with gr.Blocks() as demo:
    max_value = gr.Slider(minimum=1, maximum=20, step=2)
    df = gr.DataFrame(run_query, every=1)
    max_value.change(run_query, inputs=max_value, outputs=df, every=1, cancels=[df.load_event])

demo.queue().launch()

@abidlabs abidlabs marked this pull request as draft December 15, 2022 19:13
@abidlabs abidlabs assigned abidlabs and unassigned abidlabs Dec 15, 2022
@abidlabs abidlabs marked this pull request as ready for review December 15, 2022 19:13
@aliabid94
Copy link
Collaborator

I'm hesitant to expand our API in what feels like unintuitive to me. We've never attached event listeners via Component constructors before. I would prefer:

df = gr.Dataframe(value=fn)
df.every(60)

which is at least parallel to btn.click(), in that we attach event listeners via component methods.

@aliabid94
Copy link
Collaborator

aliabid94 commented Dec 15, 2022

I think right now, every is attached to the Blocks, right? so the existing way would be something like

with gr.Blocks() as demo:
  df = gr.Dataframe()
  demo.every(fn=fn, inputs=None, outputs=df, rate=60)

so would prefer our shorthand to be:

  df = gr.Dataframe(value=fn)
  evt = df.every(rate=60)

I prefer this because I don't like how much we've expanded the API for the shorthand - not only adding event listeners in the constructor, but also cancelling this event via Component.load_event

@abidlabs
Copy link
Member Author

Hmm, but we already do attach event listeners via Component constructors. When you do:

df = gr.Dataframe(value=fn)

It creates a .load() event listener that runs when the page loads. So instead of passing in every to the load() function, here you are passing every to the constructor. I see what you're saying there about it being a little awkward in the constructor, but I think we've already crossed that bridge by letting users pass in a function to the value parameter. And now this is just providing some more support for what I believe will be a useful case (running that function iteratively).

@abidlabs
Copy link
Member Author

Synced with @aliabid94, he'll think about this API a little more, so let's hold off merging for now

@aliabid94
Copy link
Collaborator

Ok I think this is fine for now, I still do think our API is becoming unintuitive but I can't think of a simpler way without major changes.
My only remaining concern is that to cancel, cancels=[df.load_event] is unintutive since the user never attached a load_event themselves. Can we allow cancels=[df]? Could be implemented as: if a user passes component to cancels, all attached event listeners are cancelled.

@abidlabs
Copy link
Member Author

abidlabs commented Dec 15, 2022

Ok but what if they only want to cancel a specific event?

Also, what does "all attached event listeners" mean? Any events in which that component is an input or output? I feel like that complicates things, or at least should be a separate PR where we can discuss that

@aliabid94
Copy link
Collaborator

my suggestion was, to cancels you can pass a specific event, or a component which cancels all attached events. yeah could be a separate PR with discussion.

@aliabid94
Copy link
Collaborator

I meant where the component is the triggering event, but I guess in the context of the dataframe default value function, the dataframe wouldn't even be the triggering component.

@abidlabs
Copy link
Member Author

Correct, it would only be the output component, so probably doesn't make sense. I've added the .load_event in the docstring for every in the meantime. Let me know if you have another feedback on the PR or if this is good to go!

@freddyaboulton
Copy link
Collaborator

I meant where the component is the triggering event, but I guess in the context of the dataframe default value function, the dataframe wouldn't even be the triggering component.

If we think passing the component into cancels is more clear than component.load_event, we can add some logic to set_cancel_events so that we extract the load event if the value of cancels is an instance of IOComponent.
Something like:

        cancels = [dep.load_event if isinstance(dep, IOComponent) else dep for dep in cancels]

I am ok with component.load_event given that it's documented, though.

@aliabid94
Copy link
Collaborator

I like that suggestion @freddyaboulton

@abidlabs
Copy link
Member Author

If we think passing the component into cancels is more clear than component.load_event, we can add some logic to set_cancel_events so that we extract the load event if the value of cancels is an instance of IOComponent.

Hmm I still feel like it is complicating user's experience -- consider a user who sees cancels[component] -- they'll have to look up what that does. whereas if they read cancels[component.load_event], it's more clear what's happening. But regardless, if we feel strongly, we can do that in a separate PR.

So unless there are other objections, I'll go ahead and merge this in?

@abidlabs abidlabs merged commit 4e73170 into main Dec 15, 2022
@abidlabs abidlabs deleted the comp-every branch December 15, 2022 22:07
@abidlabs
Copy link
Member Author

Thanks the feedback guys!

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