Skip to content

Respect Upstream Queue when loading interfaces/blocks from Spaces#2294

Merged
freddyaboulton merged 6 commits into
mainfrom
1316-respect-queue-upstream
Sep 21, 2022
Merged

Respect Upstream Queue when loading interfaces/blocks from Spaces#2294
freddyaboulton merged 6 commits into
mainfrom
1316-respect-queue-upstream

Conversation

@freddyaboulton
Copy link
Copy Markdown
Collaborator

@freddyaboulton freddyaboulton commented Sep 19, 2022

Description

Closes: #1316

The approach taken is to open up a websocket connection to the space for each prediction request sent to the loaded space. The main limitation of this approach is that the loaded app doesn't display information about the request's position in the original app's queue. However, there are a couple of reasons why I went with this approach:

  1. We can't send updates to the front-end if the front-end isn't running, e.g. when using the interface or app as a function.
  2. The app that's loading the interface may have its own queue, and if so, it doesn't make sense to display another app's queue. Imagine you're a user of an app with a queue that's loading another app with a queue. You'd see your position decrease until your request is being processed, but then you'd see your queue position jump back up again when the upstream queue gets the request sent from the downstream. I think that will be confusing to users who have no idea about how the downstream app they are using is implemented. In short, passing updates from the upstream queue to the downstream app would only make sense for gr.Interface.load().launch workflows but not more complex uses of gr.Interface.load.

How to test,

Launch the following app:

import gradio as gr

io = gr.Interface.load("spaces/freddyaboulton/saymyname")
print(io("foo"))
io.launch(enable_queue=True)

Then go to this space: https://huggingface.co/spaces/freddyaboulton/saymyname

Launch three simultaneous requests but make sure the first two are on the HF space. On the app running locally, it should take around ~15 seconds to complete the request.

respect_upstream_spaces

Checklist:

  • I have performed a self-review of my own code
  • My code follows the style guidelines of this project
  • I have commented my code in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@github-actions
Copy link
Copy Markdown
Contributor

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

Comment thread gradio/external.py Outdated
Comment thread gradio/external.py Outdated
@freddyaboulton freddyaboulton marked this pull request as ready for review September 19, 2022 23:23
Comment thread gradio/external.py Outdated
@abidlabs
Copy link
Copy Markdown
Member

Thanks @freddyaboulton this looks great! Tested both with the downstream app enabling queue explicitly and with the downstream app not enabling queuing, like this:

import gradio as gr

io = gr.Interface.load("spaces/freddyaboulton/saymyname")
print(io("foo"))
io.launch()

In both cases, the upstream queue is respected. Also tested with some upstream apps that don't have queue.

The one wrinkle that we should address is that in a Blocks demo, the upstream app may enable queuing for some functions, but not all. This can happen by enabling the queue by default, but then disabling for some specific functions, or vice versa. The current implementation will only look at the default queuing value in the upstream app. Instead, when we iterate through the dependencies, it would be good to check for that specific function, queuing is enabled.

Comment thread gradio/external.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe older versions of Gradio did not include the version in the config, and so it would be good to check for that first so that a KeyError is not thrown

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point!

Comment thread gradio/external.py Outdated
Comment thread test/test_external.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused, as there's as AsyncMock in the previous tests which are not skipped?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that websockets.connect returns an async context manager and mock doesn't really work with async context managers in 3.7. I can make the reason a bit more precise.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it thanks for clarifying! Good to know

Comment thread test/test_external.py Outdated
@abidlabs
Copy link
Copy Markdown
Member

Good stuff! Left some more comments, mostly nits. The main thing is to respect upstream queue per-function as opposed to the entire app, as I mentioned above.

@freddyaboulton freddyaboulton force-pushed the 1316-respect-queue-upstream branch from e23941c to a9831c5 Compare September 20, 2022 18:54
@freddyaboulton
Copy link
Copy Markdown
Collaborator Author

Thanks for the review @abidlabs and good catch about honoring the queue per function. Made that change - this should be good for another look!

@abidlabs
Copy link
Copy Markdown
Member

Tested and LGTM @freddyaboulton! This is awesome

@freddyaboulton freddyaboulton force-pushed the 1316-respect-queue-upstream branch from 49bc4f2 to f6b8756 Compare September 21, 2022 17:01
@freddyaboulton freddyaboulton merged commit 11379b9 into main Sep 21, 2022
@freddyaboulton freddyaboulton deleted the 1316-respect-queue-upstream branch September 21, 2022 17:18
@omerXfaruq
Copy link
Copy Markdown
Contributor

@freddyaboulton this looks pretty good! I think adding some tests to the queue would be great using a similar approach.

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.

Queue upstream when loading apps via gr.Interface.load()

3 participants