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

Basic async support #2033

Open
septatrix opened this issue Aug 4, 2021 · 19 comments
Open

Basic async support #2033

septatrix opened this issue Aug 4, 2021 · 19 comments

Comments

@septatrix
Copy link

This is a resumption of #85 which was closed more than 7 years ago when asyncio was at a completely different stage in the python ecosystem. The goal of this issue is basically to enable users to do the following:

@click.command()
async def cli():
    await asyncio.sleep(1)
    click.echo("Hello World")

if __name__ == "__main__":
    cli()

Currently a workaround is to wrap every such function with a decorator like the following:

def make_sync(func):
    @functools.wraps(func)
    def wrapper(*args, **kwargs):
        return asyncio.run(func(*args, **kwargs))
    return wrapper

@click.command()
@make_sync
async def cli():
    ...

The unclean aspect of the decorator approach is that it requires additional code which could be redundant as click is able to simply infer via introspection that a function is a coroutine and handle accordingly.
This issue is only about the bare minimum to get such simple commands working. What this issue is not about is async-ifying most of the API like it is done in asyncclick (a fork which also makes e.g. ctx and autocompletion async).

Arguments which have been brought up in the past by @mitsuhiko are that (1) this does not belong in click, (2) one would need to get access to the loop anyhow for complex use cases and (3) that there would inevitably be some people demanding support for twisted, trio etc. I would like to address each of them:

  1. I see no reason why this does not belong in click. async/await is a first-class language feature in python and should be supported where feasible. Many CLI tools perform I/O, be it networking, file access or spawning subprocesses. All of these things can benefit from coroutines.
  2. This is not really an argument as one can still get the current loop using asyncio.get_event_loop()/asyncio.get_running_loop() - previous cases where the loop itself may have been passed as an argument are long made redundant.
  3. Those people can still use asyncclick which supports different event loops. However depending on the implementation it could be made possible to allow subclassing and overwriting some methods for increased compatibility.

Another reason why one might not want this might be decreased maintainability. However as the simplest implementations is 3 lines long I am not sure if this counts. Python 3.6 support would add another 2 lines.
On the other hand there are confused users when they get exceptions like sys:1: RuntimeWarning: coroutine 'main' was never awaited due to click lacking async support and users who need to get creative when the above decorator approach does not work anymore (when using some more involved type hints).

Other pallets projects which turned down async support years ago now support it like Flask (since 2.0) and Jinja (since 2.9). They too only implement a minimum to work with it but that is sufficient for most people.

@mitsuhiko
Copy link
Contributor

I agree with this generally. The ecosystem is in a vastly different space now than it was back then. That said, native support is a bit tricker because click allows to invoke things recursively and it's unclear if the expectation is that async -> sync is happening at any level or toplevel only.

The wrapping that is currently recommended effectively makes the async stop early and I'm not sure this is correct to do for a first class version.

@makkus
Copy link

makkus commented Aug 16, 2021

That said, native support is a bit tricker because click allows to invoke things recursively and it's unclear if the expectation is that async -> sync is happening at any level or toplevel only.

I agree that this is going to be tricky to get right, and would probably require a lot of thought. Personally, if this was implemented, I would expect async to be available at every stage throughout click. Even in the 'assemble sub-commands' stage, when calculating completions, when doing validation, etc.

I was using asyncclick for a project before, and in general it worked very well (I esp. liked that it used anyio under the hood to support multiple event loops). The one problem that was tricky to get implemented was that I wanted to dynamically discover sub-commands according to some json files on the filesystem, and I wanted to do all of this async. I kinda got it working, but don't ask me to show you the code... If I did that again, I think I'd just use a thread-pool, 'normal' click, and be done with it.

@davidism
Copy link
Member

davidism commented Aug 16, 2021

Supporting "everything" runs into the same problem as Flask: everything about Click's API is sync, so we'd have to essentially duplicate every object to have async versions of all their methods. (We can't just make everything async def internally because that would break all existing subclasses, extensions, etc.) And it's usually expected that both sync and async can be intermixed, which runs into the problem that we need to at any point in Click's dynamic resolution be able to switch execution contexts, or choose the right function based on the current execution context. Doing this sort of detection and switching also adds overhead. For a CLI app, responsiveness from the start is important, so this is not ideal.

For example, if type validation can be async, that must mean that a custom ParamType can define either or both of def convert and async def async_convert, then we need to decide which to run. Do we run convert because the command it's part of doesn't have an async def callback? Or do we always prefer starting an async loop if both are defined? What if a previous option in the processing pipeline has an async def callback, do we now prefer async because it already had to start a loop, even if the command is sync?

While I'm generally fine with the idea of supporting async, I really need to understand what the common use case we should solve for is. What specifically do people want to do that they can't do right now? Not "type conversion" but "I have to do this specific type conversion that absolutely requires async to be useful."

@mitsuhiko
Copy link
Contributor

Supporting "everything" runs into the same problem as Flask: everything about Click's API is sync, so we'd have to essentially duplicate every object to have async versions of all their methods.

I think the problem here is not as bad as in Flask. There is only a sync entrypoint to the call chain and a "make async code silently block" behavior is more than possible since nobody expects click to be involved in async call chains. One could then imagine a secondary entrypoint which does not "upgrade" a call to sync or even force a call to be async at all times.

I'm kinda curious where the general opinion is in 2021 now about how async is working out in Python. Are we moving into a world where almost every single thing is turning into async or is a situation evolving where some things are clearly sync and others async?

@ThiefMaster
Copy link
Member

I have to admit, my first thought was "why do you need async in click" because in the end parsing command line args isn't something that feels very async... sure, you might have some edge cases where you do I/O while parsing arguments, but in the end that can be done sync as well...

@mitsuhiko
Copy link
Contributor

mitsuhiko commented Aug 16, 2021

So the reason I stumbled upon this issues is that I had to invoke a completely unnecessary async call chain from a click function. But alas, apparently almost everything is turning into async now so I guess this war is lost :P

Maybe a utility decorator to wrap a function so it starts an async function and blocks on it is a reasonable thing to ship at this point.

@makkus
Copy link

makkus commented Aug 16, 2021

I have to admit, my first thought was "why do you need async in click" because in the end parsing command line args isn't something that feels very async... sure, you might have some edge cases where you do I/O while parsing arguments, but in the end that can be done sync as well...

I can only speak for myself, but if I have an async code-base that needs a command-line interface, being able to use call my existing async functions directly would be great (for quite a few other purposes than 'just' parsing args). Not for very simple cases, but as soon as I use the more advanced stuff that click allows for (like the dynamic sub-command creation, shell-completion hints...), it would certainly make life easier.

I'm not arguing that click should support that use-case, because as others on this thread I suspect the effort (and possibly complexity) involved in supporting this comprehensively would be quite high, and therefor not a good trade-off to make. It might make more sense to create a new 'async-from-the-ground-up' cli toolkit, dunno.

But, if click supported this, I would definitely find a few good use-cases for it, I'm 100% certain!

As for only shipping a few utility methods, decorators: that'd probably be nice, but personally I don't think I'd use those. I'd either use asyncclick, or threads.

@septatrix
Copy link
Author

I am not suggesting complete support for async throughout the whole codebase but only for the command functions themselves.
They could be wrapped similar to the currently recommended decorator. Such that users new to click who need to use an async command are not surprised by exceptions and do not need to look for the decorator workaround.
Again: Users who need async in all the other places (completion, arg parsing etc.) are probably better of with using the existing asyncclick. However for users who call some async functions do not have to spin up their own event loop for each such command.

This would make click <-> asyncclick similar to the relationship between Flask <-> Quart.

Alternatively to make this a bit more clear we could add a new AsyncCommand class (and corresponding @async_command) decorator which perform this wrapping explicitly instead of adding it implicitly to the current Command invocation method.

@pocek
Copy link
Contributor

pocek commented Oct 12, 2021

Well, currently a basic solution can be something like this:

class MyContext(click.Context):
    def invoke(self, *args, **kwargs):
        r = super().invoke(*args, **kwargs)
        if inspect.isawaitable(r):
            loop = asyncio.get_event_loop()
            return loop.run_until_complete(executor.submit(r))
            # or just
            # return loop.run_until_complete(r)
            # (see later)
        else:
            return r

click.BaseCommand.context_class = MyContext

...and one can has async commands anywhere, but - as was already noted - there's a lot more in click that is sync and you are out of luck there. But, from my experience, you can get pretty far with just that.

I'm sure you recognize that this is not a solution when there is already an event loop running. If one uses click in a context in which it can spin the loop (be a loop owner/controller), then it's kind of fine.

A little bit of novelty here is that executor thing (not shown in detail here). It has a crucial role of passing coro through a queue to an executor task which works like this:

while True:
    get (coro, future) from the queue
    await on coro
    set result on the future

The point of this dance is to make contextvars work across click (sub)command invokes. I guess this is much less popular problem, because not many people want to run multiple async click command chains simultaneously in a single process (it happens that I do) - in such cases anything touching click.globals in its current form is of no use.

@chobeat
Copy link

chobeat commented Feb 25, 2022

I think this discussion is kinda stranded but I would like to add that it would be great if the solution could make CLIRunner run well with asyncio too. I've been using click with home-baked async commands for a while but I can't really test properly because CLIRunner and my loop (injected by pytest-asyncio) don't really go well together.

@davidism
Copy link
Member

The most that we would be adding is having Click automatically start an event loop if a command callback is marked async def, so that await could be written directly in the command callback. There is nothing stopping you from using async library code in your commands now, you just need to do value = asyncio.run(coroutine) instead of await coroutine().

@chobeat
Copy link

chobeat commented Feb 25, 2022

that's already what I'm doing but then it conflicts with pytest-asyncio because CLIRunner does weird stuff and the coroutine is never awaited

@davidism
Copy link
Member

davidism commented Feb 25, 2022

CLIRunner does weird stuff and the coroutine is never awaited

I can't reproduce that issue. asyncio.run would await the coroutine in that case, not the Click command or runner. It sounds like you tried to use async def for the command still, that's what's not supported. You don't need it if you do asyncio.run.

@lewoudar
Copy link

lewoudar commented Mar 9, 2022

Hi everyone, I don't read all the conversation but if you want to add async support, don't forget that asyncio is not the only coroutine library, I'm thinking about trio. You can also take inspiration from the asyncclick project.
Edit: The issue author already talks about asyncclick, sorry for that :)

@makkus
Copy link

makkus commented Mar 9, 2022

@lewoudar asyncclick and trio are mentioned multiple times in this thread.

@lewoudar
Copy link

lewoudar commented Mar 9, 2022

@makkus yeah, sorry, but I was afraid when I look at his first example dealing with asyncio

@davidism
Copy link
Member

davidism commented Mar 9, 2022

If something were to get in, it would presumably work similar to Flask, where there is a method that handles spawning the event loop for coroutines, and can be overridden to use something besides asyncio.

@lewoudar
Copy link

lewoudar commented Mar 9, 2022

Thanks for the clarification @davidism :)

@romanzdk

This comment has been minimized.

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

No branches or pull requests

9 participants