-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Add Timer component #8505
Add Timer component #8505
Conversation
🪼 branch checks and previews
Install Gradio from this PR pip install https://gradio-builds.s3.amazonaws.com/de843f3a33d01c19e810a15ee4263fd6b7929449/gradio-4.37.1-py3-none-any.whl Install Gradio Python Client from this PR pip install "gradio-client @ git+https://github.com/gradio-app/gradio@de843f3a33d01c19e810a15ee4263fd6b7929449#subdirectory=client/python" Install Gradio JS Client from this PR npm install https://gradio-builds.s3.amazonaws.com/de843f3a33d01c19e810a15ee4263fd6b7929449/gradio-client-1.2.0.tgz |
🦄 change detectedThis Pull Request includes changes to the following packages.
With the following changelog entry.
Maintainers or the PR author can modify the PR title to modify this entry.
|
…gradio into timer_event_listener
Hmm I'm finding this syntax unnecessarily complicated in this PR and internal message: with gr.Blocks() as demo:
def show_plot_1():
...
def show_plot_2():
...
with gr.Timer(5) as timer:
plot_1 = gr.LinePlot(value=show_plot_1)
plot_2 = gr.LinePlot(value=show_plot_2)
That being said, I do see the value in having a separate So rewriting your example code: with gr.Blocks() as demo:
def show_plot_1():
...
def show_plot_2():
...
timer = gr.Timer(5)
plot_1 = gr.LinePlot(value=show_plot_1, every=timer)
plot_2 = gr.LinePlot(value=show_plot_2, every=timer) |
This addition to the API does feel useful to me ✔️ |
I like this suggestion. Also, I think right now the timer only triggers For a use case where you want to start the schedule based on a button click or tab select, you have to do something like this with gr.Blocks() as demo:
timer = gr.Timer(5, active=False)
plot = gr.LinePlot(value=get_data, every=timer)
button = gr.Button("Start")
button.click(lambda: gr.Timer(active=True), None, timer) If we let it trigger any event, the code could be rewritten like the following which feels more natural: with gr.Blocks() as demo:
plot = gr.LinePlot()
button = gr.Button("Start")
button.click(get_data, None, plot, every=gr.Timer(5)) |
Okay sure. Originally my logic for using context managers for Timer and InputSet had been that in a dashboard, every component is tied to the same timer and set of inputs, and its annoying to have to set the timer and the inputs for each component, but that's something we can design some other time.
A little confused by this syntax. Does the syntax above start the given timer, and then bind this same event to the tick of the Timer? If it gets clicked again, it does nothing? Without any button control, the syntax is very clear: plot = gr.Plot()
timer = gr.Timer(5)
timer.tick(get_data, None, plot) or simply: plot = gr.Plot(get_data, every = gr.Timer(5)) If you are adding a button to start the timer, then you probably want to be able to stop it too, so you need access to the timer object. In that case, you are suggesting: plot = gr.Plot()
timer = gr.Timer(5)
button.click(get_data, None, plot, every=timer)
button2.click(lambda: gr.Timer(active=False), outputs=timer) correct? Imo that's messy compared to: timer = gr.Timer(5)
plot = gr.Plot(get_data, every=timer)
button.click(lambda: gr.Timer(active=True), outputs=timer)
button2.click(lambda: gr.Timer(active=False), outputs=timer) The second option is much more explicit, takes the same amount of code, and starting and stopping use the same syntax. I don't think it makes sense to add |
Ok let's stick with the original proposal to have every only apply to component value functions. |
…gradio into timer_event_listener
Co-authored-by: Abubakar Abid <abubakar@huggingface.co>
I believe everything has been addressed, could use a re-review cc @abidlabs, also @pngwn if you can look at my approach to avoid calls stacking up when the browser is minimized - this was happening because window.dispatchEvent was triggering while minimized, but window.addEventListener("gradio") was not issuing callbacks until the window was restored. Fixed by only firing ticks if the document is visible |
@@ -453,11 +453,7 @@ def predict( | |||
client.predict(5, "add", 4, api_name="/predict") | |||
>> 9.0 | |||
""" | |||
inferred_fn_index = self._infer_fn_index(api_name, fn_index) | |||
if self.endpoints[inferred_fn_index].is_continuous: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we still need this when the Client connects to Spaces running older versions of Gradio?
@@ -398,7 +427,7 @@ def event_trigger( | |||
preprocess: If False, will not run preprocessing of component data before running 'fn' (e.g. leaving it as a base64 string if this method is called with the `Image` component). | |||
postprocess: If False, will not run postprocessing of component data before returning 'fn' output to the browser. | |||
cancels: A list of other events to cancel when this listener is triggered. For example, setting cancels=[click_event] will cancel the click_event, where click_event is the return value of another components .click method. Functions that have not yet run (or generators that are iterating) will be cancelled, but functions that are currently running will be allowed to finish. | |||
every: Run this event 'every' number of seconds while the client connection is open. Interpreted in seconds. | |||
every: Will be deprecated in favor of gr.Timer. Run this event 'every' number of seconds while the client connection is open. Interpreted in seconds. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does every
for events behave differently than in components? We should just have the same behavior here, where you can provide a timer or a float?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want to encourage the use of every
added to an event listener which is a very weird API imo, so I'm only supporting floats for the sake of backwards compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not? Its quite an intuitive api -- once the event is triggered, it should rerun every X seconds. Its much easier than creating a gr.Timer object and setting it to active, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be the 5th (!!!) way to support a continuous event, we already have:
- using a direct timer with a tick listener
- using every=float in a Component, which creates a timer under the hood
- using every=Timer in a Component with an existing explicit timer
- setting every=float in an event listener, to specifically say, this event also start a timer that binds to this same event (I would like to deprecate this one)
We keep coming up way too many ways to do the same thing and it makes the API messier and messier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But these are not the same thing. Using every
in a Component only allows you to rerun a function when the demo first loads. Whereas using every
in an event listener allows you to rerun a function when an event is triggered.
In fact, I would argue that this would make things more consistent. Just like every
in a Component can be either a float or a Timer, every
in an event listener can be a float or a Timer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Synced with @aliabid94 and I'm convinced that its not a good idea to add yet another approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few nits, but can otherwise confirm that this works great. Great stuff @aliabid94!
all good from my end! |
gr.Timer
, a Component meant to replaceevery
, e.g.timer = gr.Timer(5)
for a 5 second regular timer. Can be used via:tick
event listener, e.g.timer.tick(fn=..., inputs=[], outputs=[])
every=
that can accept a timer `gr.Textbox(value=lambda: ..., every=timer)inputs=
kwarg to make function values more useful, e.g.gr.Textbox(value=lambda a, b: ..., inputs=[text1, text2])
. The component will automatically re-update if any of the the input components change.Can all be combined for maximum "dashboard" shorthand:
gr.Textbox(value=lambda a, b: ..., inputs=[text1, text2], every=timer)
, which sets the value of the component as a function of the inputs, updating every timer tick and also when the inputs change.See demo file that uses all these concepts:
Fixes: #8582
Fixes: #6577
Fixes: #8463
Fixes: #7051
Fixes: #6841