Update queue with using deque & Update requirements #2428
Conversation
websockets with version <=9.1 with raise TypeError: WebSocketCommonProtocol.__init__() got an unexpected keyword argument 'logger'
GLGDLY
changed the title
|
Thanks @GLGDLY ! We'll test this this week to compare performance with the current queue implementation. |
For my own massive test considering append, pop, remove and iterate first few items, there is likely to be a 30-40% performance improve comparing deque and list operations. It would be great if you can do some more tests based on actual usage with it, thank you! |
|
seems like there are some conflicts now, have you guys test about this deque part? If it is ok then I might resolve the conflicts later @freddyaboulton |
oh I see, as I have some more stuffs want to discuss and maybe make some more prs, which is not quite related to this pr, so is wondering what's the condition here lol |
|
What do you mean by condition @GLGDLY ? |
state? status? I don't quite know if I have used a correct wording🤔 |
ok thanks, I may come and see later😊 |
|
Hi @GLGDLY ! If you rebase this pr, we can review it! Thank you |
@freddyaboulton it is now rebased, thank you |
There was a problem hiding this comment.
Thanks for the PR @GLGDLY !
I tested this branch against version 3.9 with our queue batch benchmark and there aren't any noticeable regressions in performance.
Gradio 3.9
| Input type | Average time to complete prediction over 1000 requests |
|---|---|
| Audio | 1.1506862538333997 |
| Image | 0.9021098742118249 |
| Text | 0.8479320884290324 |
| Video | 2.458217331471334 |
This branch
| Input type | Average time to complete prediction over 1000 requests |
|---|---|
| Audio | 1.1911795553514513 |
| Image | 0.9411118013017318 |
| Text | 0.9387184648371455 |
| Video | 2.373436844774655 |
I tested locally as well and the time are very comparable.
The demos are deployed here and they look good to me: https://huggingface.co/spaces/gradio-pr-deploys/pr-2606-all-demos
Any comments before we merge this in @abidlabs ?
| return None | ||
| self.event_queue.append(event) | ||
| return len(self.event_queue) - 1 | ||
| return queue_len |
There was a problem hiding this comment.
Can we keep this as len(self.event_queue) - 1 please?
|
Thanks for benchmarking @freddyaboulton! I was hoping that there would be a more meaningful improvement, but I'm fine with accepting this PR since a deque does seem like the appropriate structure to use here. |
|
Thank you @GLGDLY ! |
Ok thank you😄 |
fix wrong position in changlog from the last pr gradio-app#2428 also
* Update queue.py * Update queue.py * Update CHANGELOG.md * Update CHANGELOG.md * Update queue.py * Update requirements.txt websockets with version <=9.1 with raise TypeError: WebSocketCommonProtocol.__init__() got an unexpected keyword argument 'logger' * fix issues after rebase * feat Allow auth with using queue * remove unused imports * Update CHANGELOG.md fix wrong position in changlog from the last pr #2428 also * revert test_queue_enabled_for_fn that is changed due to unintended mistake * remove comment * update tests and merge main thread Co-authored-by: Freddy Boulton <alfonsoboulton@gmail.com> Co-authored-by: Ali Abid <aabid94@gmail.com>
Description
The performance of queue instance might be improved with the use of double ended queue and
.popleft(), rather than the list used currently.Also, for the requirements part, websockets with version<=9.1 would raise
TypeError: WebSocketCommonProtocol.__init__() got an unexpected keyword argument 'logger', so it would be better to ensure its version to be >=10.0.Checklist: