beeware / toga

A Python native, OS native GUI toolkit.
https://toga.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
4.27k stars 661 forks source link

Document passing arguments to event handlers #2099

Open Cimbali opened 1 year ago

Cimbali commented 1 year ago

Summary by @freakboy3742

We are regularly asked either (a) how to "pass in arguments to event handlers", or (b) for a feature addition to do the same. This isn't something that needs additional functionality - it just needs a better understanding of how Python can be used.

We should add a topic guide describing how to pass in additional details to an event handler, including:

Originally framed in the form of a feature request, asking for the ability to pass in coroutines to add_background_task; details follow. The comments following the ticket give a starting point for a discussion in the topic guide.


Original post by @Cimbali

What is the problem or limitation you are having?

Currently add_background_task accepts generators and coroutine functions (i.e. functions that return a coroutine). This makes passing an asynchronous function that takes arguments quite awkward.

Suppose I have an async function (“coroutine function”) that takes some arguments:

async def func(arg1, arg2):
    pass

With asyncio, the idiom is call the function, have it return a coroutine, and let asyncio take care of running it:

asyncio.run(func('a', 'b'))

In toga, the idiom requires toga to call the function, which means we are now fixing the prototype of the function:

app.add_background_function(func)  # Does not fit func’s prototype!

Describe the solution you'd like

If add_background_function supported passing in coroutines, we could do:

app.add_background_function(func('a', 'b'))

Describe alternatives you've considered

If I want to pass several arguments to an async function run in the background, the most concise I can do is:

async def wrapper(app):
    return await func('a', 'b')

app.add_background_function(wrapper)

Which in more complex code becomes quite annoying, and means a function closure for every call, plus an additional await.

Additional context

Note you also can’t do:

def wrapper(app):
    return func('a', 'b')

This also returns a coroutine which is awaitable (and asyncio.run(wrapper(app)) woulc work as expected) but the wrapper isn’t decorated as a coroutine function.

freakboy3742 commented 1 year ago

The request as you've made it doesn't entirely make sense.

Event handlers have a very specific prototype:

def my_handler(widget, **kwargs):
    ...

There's no capacity to pass in arguments to a handler; and background tasks are no different to any other handler. This stands to reason - where do the extra arguments come from? You're not in control of invoking the handler, so there's no way to pass additional arguments in when it is invoked.

However - that's not what you're describing here. What you're describing is providing additional context at the time the background handler (or any event handler for that matter) is defined.

This you definitely can do. One approach is to use a factory method:

    def make_handler(self, arg1, arg2):
        async def work_handler(app, **kwargs):
            print("HELLO", arg1, arg2)
        return work_handler

then invoking:

self.add_background_handler(self.make_handler('a', 'b'))

Calling make_handler() creates and returns a co-routine that can be used as a handler, and that co-routine has a closure over the arguments that were used to construct it.

An alternative spelling of the same approach is functools.partial

from functools import partial

async def my_handler(arg1, arg2, app, **kwargs):
    print("Hello", arg1, arg2, app)

self.add_background_task(partial(my_handler, "a", "b"))

functools.partial effectively does the same thing as make_handler above, but without needing to define a nested factory function. It creates a function (or, in this case, coroutine) that has 2 arguments "pre-called", and returns a function that has all the unspecified arguments available for use. For this reason, argument order matters a lot - you have to partial arguments in order (unless you're using keyword arguments).

Alternatively, if what you want to do is schedule a coroutine... then you can do that with raw asyncio primitives, without using add_background_task at all. asyncio.run() isn't an option because it creates and destroys an event loop, and the app already has one - but you can use ensure_future:

async def my_handler(arg1, arg2):
    print("Hello", arg1, arg2)

asyncio.ensure_future(my_handler("a", "b"))

Toga's event loop is fully integrated with Python's event loop, so all of Python's event loop handling APIs will work, with he caveat that an event loop is already running.

The proposed syntax of add_background_task(my_hander('a', 'b')) doesn't make any sense, because what is being provided isn't a handler - it's a co-routine. We could potentially modify add_background_task to accept (and make a special case of co-routines) - but that would only work for co-routines, and it would essentially be duplicating functionality you can already perform with built-in asyncio methods.

This is a question that comes up a lot, so I'm going to convert this to a documentation ticket. We clearly need a topic guide to explain how to get extra context into a handler, and the other out-of-the-box options that exist for doing background work.

mhsmith commented 1 year ago

@freakboy3742: At one point you use the name add_background_handler, and a lot of the rest of your comment seems to assume the method has that name. So if even you can't keep this straight, then the documentation definitely needs some improvement. 😄

And I think maybe some renamed or additional methods would be a good idea, because what the method accepts isn't a Task in the asyncio sense either.

Cimbali commented 1 year ago

Alternatively, if what you want to do is schedule a coroutine... then you can do that with raw asyncio primitives […] Toga's event loop is fully integrated with Python's event loop, so all of Python's event loop handling APIs will work, with he caveat that an event loop is already running.

I think this is the point worth documenting the most. All other gui toolkits require you to use their “schedule” or “schedule later” functionality to have calls to expensive functions play well with their rendering thread, which is probably we users look to your API on how to do that.

There's no capacity to pass in arguments to a handler; and background tasks are no different to any other handler. This stands to reason - where do the extra arguments come from? You're not in control of invoking the handler, so there's no way to pass additional arguments in when it is invoked.

Say a use case is this: you have a set of buttons, all of which can cause an “intensive” task (let’s say a download so it’s IO-intensive)

Now you want to start the download, so options are:

  1. you have a background handler in your app waiting on a asyncio.Queue() of URLs
    • You need a queue and not a simpler state, otherwise clicking 2 buttons in quick succession doesn’t register
    • In order to do several downloads at the same:
      • you probably need a second queue to properly await them (to hold the return values of ensure_future())
      • your finished downloads then to be handled in a callback or a second handler
    • your handler is waiting there all of the time, which means if there is an uncaught error now your functionality is broken
    • at some point this all looks like boilerplate task-scheduling code for a specific purpose
  2. you can queue a download(url) task
    • no locality / state / concurrency / resilience issues to handle, they are all out of the box
    • resulting code is much simpler
    • requires passing an argument when it is invoked

Roughly the missing piece of the puzzle for me ws that you can just do the second with asyncio and it plays nice with toga.

freakboy3742 commented 1 year ago

@freakboy3742: At one point you use the name add_background_handler, and a lot of the rest of your comment seems to assume the method has that name. So if even you can't keep this straight, then the documentation definitely needs some improvement. 😄

🤦

And I think maybe some renamed or additional methods would be a good idea, because what the method accepts isn't a Task in the asyncio sense either.

That's a good point. I'm not opposed to a rename in the interests of clarity; what did you have in mind in terms of renames and/or additional methods? Do you think my accidental flub of add_background_handler() a better option?

freakboy3742 commented 1 year ago

I think this is the point worth documenting the most. All other gui toolkits require you to use their “schedule” or “schedule later” functionality to have calls to expensive functions play well with their rendering thread, which is probably we users look to your API on how to do that.

Agreed that this is what most GUI frameworks do; however, most GUI frameworks don't come from languages that have native async handling. :-)

There's no capacity to pass in arguments to a handler; and background tasks are no different to any other handler. This stands to reason - where do the extra arguments come from? You're not in control of invoking the handler, so there's no way to pass additional arguments in when it is invoked.

Say a use case is this: you have a set of buttons, all of which can cause an “intensive” task (let’s say a download so it’s IO-intensive)

  • running them in the handler directly causes the UI to freeze
  • inside your button click handler you get all the info (e.g. the URL to download from)

I'm extremely familiar with the use case - in fact, just last week I gave a presentation at PyCon AU in which "you want to download something" is the exact example that I use for a long lived task blocking the GUI event loop.

However your description of the mechanics of a download handler misses my point - when you say "passing an argument into a handler", do you mean "at the point it is constructed", or "at the point it is invoked". Your theoretical example of app.add_background_function(func('a', 'b')) isn't passing in the arguments when the function is invoked - they're being passed in when the function is created.

To make the point more obvious: let's say the download URL is coming from a text input - are you binding the value of the text input when you queue the download, or reading the value that is there when the download starts? The two values need not be the same. If the case of "queue this URL for download", you almost certainly want to bind the value at time of queuing; but in the case of "start a download when the button is pressed", you want to read the value of the text input at the time the button is pressed, not use a pre-bound value.

All this by way of saying - there's a reason that the argument to Toga handlers is a reference to a function, not a co-routine; and if you want to schedule a co-routine (which is an invoked async function) for execution, asyncio.ensure_future() exists.

(Also, FWIW: You've described a moderately complex queue-based download system; a much simpler approach would be to use an async request API such as httpx)

Cimbali commented 1 year ago

However your description of the mechanics of a download handler misses my point - when you say "passing an argument into a handler", do you mean "at the point it is constructed", or "at the point it is invoked". Your theoretical example of app.add_background_function(func('a', 'b')) isn't passing in the arguments when the function is invoked - they're being passed in when the function is created.

In this example, arguments are passed in when the coroutine function is invoked, which is when the coroutine is created. I’m sorry the python terminology is a bit confusing. I’ve written out the example a bit more below to see if that makes more sense?

you want to read the value of the text input at the time the button is pressed, not use a pre-bound value

Definitely something that can be done in the button click handler.

You've described a moderately complex queue-based download system; a much simpler approach would be to use an async request API such as httpx

The request is not what is making up the complexity, but handling starting downloads, awaiting them, properly handling errors -- this is all boilerplate stuff and shouldn’t have to be written out every time.


This is the coroutine-as-task model I would like to use

def App(toga.App):
    def startup(self):
        self.textbox = toga.TextInput()
        self.button = toga.Button('text=Download!', on_press=self.click_handler)
        # window setup etc.
        self.client = httpx.AsyncClient()

    def click_handler(self, widget):
        url = self.textbox.value
        self.add_background_task(self.download(url))  # here we start 1 coroutine per download. NB. asyncio idiom that does not work with toga, use asyncio.create_task() instead.

    async def download(self, url):
        async with self.client.stream('GET', url) as response:
            async for chunk in response.aiter_bytes():
                 process(chunk)

This is the handler model as I understand it (?)

def App(toga.App):
    def startup(self):
        self.textbox = toga.TextInput()
        self.button = toga.Button('text=Download!', on_press=self.click_handler)
        # window setup etc.
        self.client = httpx.AsyncClient()
        self.queue = asyncio.Queue()
        self.add_background_task(App.downloader)  # here we start 1 unique handler in startup()

    def click_handler(self, widget):
        url = self.textbox.value
        self.queue.put_nowait(url)  # provision queue size well or risk QueueEmpty! Can’t async put as we are in a synchronous callback.

    async def downloader(self):
        while True:
            url = await self.queue.get()
            try:
                async with self.client.stream('GET', url) as response:
                    async for chunk in response.aiter_bytes():
                         process(chunk)
            except:
                # If we don’t do this ourselves, we prevent all future downloads from working
                pass  # probably print to stderr or something
            finally:
                self.queue.task_done()

Note that the second version does not support concurrent downloads. To do that, you’d have to create futures out of every download (which is what download() in the first example does). In that case, downloader just becomes a boilerplate scheduler. Basically for this use case I don’t think a handler is a useful idiom.


If I may suggest, one good way of documenting this would be to print an elightening error message when calling add_background_task() with a coroutine. Currently it errors out with “coroutine is not callable” which is not very helpful. I think it would be helpful to check whether the argument is a coroutine, and if so, print an error message along the lines of “add_background_task() is for long-running handlers that only take the toga.Application instance as argument. To schedule tasks you can use asyncio.create_task() directly“, maybe with a link to the docs too.

mhsmith commented 1 year ago

Thanks for pointing out create_task; that's much easier to remember than ensure_future.

In fact, since app.add_background_task(my_task) is basically equivalent to asyncio.create_task(my_task(app)), but less flexible, should we just deprecateadd_background_task?

Cimbali commented 1 year ago

That would definitely clear up the confusion :)

freakboy3742 commented 1 year ago

Deprecating add_background_task() is definitely an option, and I can see the appeal. However, it would rule out the other way that add_background_task can currently be used - passing in a generator that yields periodically to release control. However, maybe the approach is to expose toga.handlers.long_running_task() (possibly with a different name?) that is used internally to convert generators into a coroutine.

mhsmith commented 1 year ago

it would rule out the other way that add_background_task can currently be used - passing in a generator that yields periodically to release control.

Maybe we should deprecate that too, now that Python has built-in async support (#2721).

freakboy3742 commented 1 year ago

Yeah - that's probably the best option. Clearly documenting asyncio.create_task() as a HOWTO, and deprecating App.add_background_task() means one less thing for us to maintain, and avoids the problem of people passing blocking function handlers to the background task. Supporting generators is a nice trick, but definitely not essential given that we've got a pure async workaround.

freakboy3742 commented 1 year ago

One related data point: #1415 is a request for "run later" capability. This can be achieved with asycnio.get_event_loop().call_later() or asycnio.get_event_loop().call_at(); but there's no asyncio.call_later() analog, and asyncio.get_event_loop().create_task() is considered a "low level" API.

I'm not sure if this means we should have App.create_task() and App.call_later() that are thin proxies around the underlying asyncio calls, or we live with the discrepancy and treat that as a "documenting how to use asyncio" problem.

(or, I guess, the third option: an opportunity to contribute upstream to CPython and add an asyncio.call_later() shim...)

mhsmith commented 9 months ago

One related data point: #1415 is a request for "run later" capability. This can be achieved with asycnio.get_event_loop().call_later() or asycnio.get_event_loop().call_at()

Now that the event loop is available as a property on the App object, this can be simplified to self.loop.call_later and self.loop.call_at, where self is the App. It would be worth adding examples of those to the documentation.

We should also have an example of how to integrate blocking functions which unavoidably must be run on a separate thread. In the course of answering some StackOverflow questions (1, 2), I've come up with a pretty clean way of doing that:

    async def my_task(self):
        while whatever:
            value = await self.loop.run_in_executor(
                None, some_slow_function, arg1, arg2, ...
            )
            self.label.text = value

Python 3.9 adds a to_thread function which is even simpler.

Related:

mhsmith commented 1 month ago

2720 deprecated add_background_task, but there are a number of other cases discussed above which still need to be documented better.