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
33.6k stars 2.54k forks source link

js without fn is executed only if fn is explicitely set to None #6729

Open thiswillbeyourgithub opened 10 months ago

thiswillbeyourgithub commented 10 months ago

Describe the bug

I notice I don't have the same behavior depending on if I comment fn=None,.

Have you searched existing issues? 🔎

Reproduction

import gradio as gr

js = """
() => {
alert("js executed");
}
"""

with gr.Blocks() as demo:
    with gr.Column():
        b1 = gr.Button("click me")
        b2 = gr.Button("click me")
        b3 = gr.Button("click me")

    b1.click(js=js, fn=None)  # works
    b2.click(js=js)  # does not work
    gr.on(
            triggers=[b3.click],
            js=js,
            fn=None,  # needed
            )
demo.launch()

Screenshot

No response

Logs

No response

System Info

gradio==4.8.0
python version: 3.9.18

Severity

I can work around it

pngwn commented 10 months ago

This seems strange.

cc @dawoodkhan82

dawoodkhan82 commented 10 months ago

This is because in the creation of an event trigger we set the fn to be "decorator" by default. So None has to explicitly set in order for the js function to be called. We can set None to be the default, but that would break event decorators.

fn: Callable | None | Literal["decorator"] = "decorator"

Any ideas around this @abidlabs?

abidlabs commented 10 months ago

I'm a little confused. So the js parameter only works if there's no backend function? I thought we could do both -- have a backend function, and a js function

pngwn commented 10 months ago

Yeah. You should be able to have any combination of fe and be function as far as I'm aware.

dawoodkhan82 commented 10 months ago

Yes, you can already do both (have a backend function, and a js function). But if you only want to have a js function run, you need to explicitly set fn=None. Which I think is fine.

pngwn commented 10 months ago

But why does the be function being the default prevent the fe function from running?

dawoodkhan82 commented 10 months ago

But why does the be function being the default prevent the fe function from running?

The event_trigger does not get created properly, because fn is set to "decorator" and so it hits this line: https://github.com/gradio-app/gradio/blob/a3cf90e57bc76c231ffc6c9abc8170ef7298f991/gradio/events.py#L234

From my understanding, since there is no actual decorator, it doesn't hit the wrapper function to create the event_trigger.

dawoodkhan82 commented 10 months ago

Synced with @abidlabs. The best solution here seems to be to set fn=None by default, and figure out how to detect if the fn is actually decorator. Will take a look at if this is possible.

abidlabs commented 2 weeks ago

Hi, apologies for the late follow up. We haven't had a chance to look into this issue recently, but the Gradio codebase has changed quite significantly since this issue was created, particularly with the release of Gradio 5. Could you let us know if this is still an issue in the latest version of Gradio (pip install --upgrade gradio)? Thanks!

thiswillbeyourgithub commented 2 weeks ago

Hi, apologies for the late follow up. We haven't had a chance to look into this issue recently, but the Gradio codebase has changed quite significantly since this issue was created, particularly with the release of Gradio 5. Could you let us know if this is still an issue in the latest version of Gradio (pip install --upgrade gradio)? Thanks!

Following up at all is great! Thanks a lot.

I've just tested my reproduction code on version 5.1.0 and confirm the bug is still here.

abidlabs commented 2 weeks ago

Thank you for confirming!