Skip to content

Progress indicator bar#997

Merged
aliabid94 merged 12 commits into
blocks-devfrom
progress_bar
Apr 15, 2022
Merged

Progress indicator bar#997
aliabid94 merged 12 commits into
blocks-devfrom
progress_bar

Conversation

@aliabid94
Copy link
Copy Markdown
Contributor

Created a progress indicator that can be attached to an event listener API call. Still a little work to show ETA and queue position, but mostly functional, try out demo/calculator.py. I think there will be opinions on how this is implemented so thought to open PR now to get feedback.

@aliabid94 aliabid94 requested review from abidlabs, omerXfaruq and pngwn and removed request for omerXfaruq and pngwn April 14, 2022 08:38
Copy link
Copy Markdown
Contributor

@omerXfaruq omerXfaruq left a comment

Choose a reason for hiding this comment

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

Beautiful design!

Comment thread gradio/components.py
Comment thread gradio/blocks.py Outdated
Comment thread gradio/blocks.py
Comment thread gradio/blocks.py
@omerXfaruq
Copy link
Copy Markdown
Contributor

How do we handle parallel events or actions in the frontend?
Do we wait until the previous event's processing is completed before sending another event to Backend server? I feel like that would be the cleanest, but might decrease responsiveness..

@omerXfaruq
Copy link
Copy Markdown
Contributor

omerXfaruq commented Apr 14, 2022

Dropping Queueing issue here for design reference: #572
While designing queueing we need to take autoscaling into account. We might not design it yet, but may brainstorm.

We should also think of communication of queuing with autoscaling. We will probably collaborate with @cbensimon 🤗

Comment thread demo/calculator/run.py Outdated
@abidlabs
Copy link
Copy Markdown
Member

Nice design, LGTM.

It would be great to see an example of a Blocks demo that uses Status Tracker for some or all of the events.

Copy link
Copy Markdown
Member

@pngwn pngwn left a comment

Choose a reason for hiding this comment

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

Left a few notes but implementation looks good 👍

I think we need to discuss how this should work, though. In my opinion we should be handling concurrent requests whereas this implementation will only handle one at a time. We also have awareness on which components will be populated as a result of the API request and should be outputting the loader visuals into the affected components. (This is also the basis on which @gary149 is deisgning as far as I'm aware as well, so it would be good to get that mechanism implemented as part of this and we can tweak the design later).

I think we should be tracking the loading status across components. So a map of some sort, keyed by the function ID which will set a prop on the relevant components of 'idle', 'pending' or 'failed'. Each component can then decide what to do for each loading state. We have all of the information for this functionality.

The current visuals will work well for most components (it just needs to be adapter to fill the component width + height as an overlay).

Happy to go over this in-person if needed.

Comment thread ui/packages/app/src/Blocks.svelte Outdated
Comment thread ui/packages/app/src/components/StatusTracker/StatusTracker.svelte Outdated
Comment thread ui/packages/app/src/Blocks.svelte Outdated
let handled_dependencies: Array<number[]> = [];
let status_tracker_values: Record<number, string> = {};

let setStatus = (dependency_index: number, status: string) => {
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.

We should use snake_case for consistent casing:

Suggested change
let setStatus = (dependency_index: number, status: string) => {
let set_status = (dependency_index: number, status: string) => {

Comment thread ui/packages/app/src/Blocks.svelte Outdated
Comment thread ui/packages/app/src/Blocks.svelte Outdated
Comment thread ui/packages/app/src/Blocks.svelte Outdated
});
})
.catch((error) => {
setStatus(i, "error");
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.

Suggested change
setStatus(i, "error");
set_status(i, "error");

Comment thread ui/packages/app/src/components/StatusTracker/StatusTracker.svelte Outdated
Comment thread ui/packages/app/src/components/StatusTracker/StatusTracker.svelte Outdated
Comment thread ui/packages/app/src/components/StatusTracker/StatusTracker.svelte Outdated
@pngwn
Copy link
Copy Markdown
Member

pngwn commented Apr 14, 2022

@aliabid94 Scratch some of that last comment, i misread the implementation at first.

@aliabid94 aliabid94 merged commit 1c2f430 into blocks-dev Apr 15, 2022
@aliabid94 aliabid94 deleted the progress_bar branch April 15, 2022 09:20
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.

4 participants