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

Provide status updates on file uploads #6307

Merged
merged 7 commits into from
Nov 8, 2023
Merged

Conversation

freddyaboulton
Copy link
Collaborator

@freddyaboulton freddyaboulton commented Nov 6, 2023

Description

Closes #6218

  • Adds an optional upload_id query parameter to the /upload route. When specified, you can listen to progress updates on the upload_progress route. I originally wanted the /upload route to return the upload_id you can use to listen to updates but we can't send intermediate information from the /upload route without closing the request and the /upload route needs the request to stay open to stream the files.
  • Adds a UploadProgress.svelte class in @gradio/upload that streams and displays progress updates from the server.
  • The upload_id parameter is added to the @gradio/client but not the python client.

Try it here: https://huggingface.co/spaces/freddyaboulton/file_component

file_upload_preview

Questions:

  • There's a bit of flicker if the uploaded file is really small. Should we only show progress for large files? My thought is no because it depends on the network speed.

🎯 PRs Should Target Issues

Before your create a PR, please check to see if there is an existing issue for this change. If not, please create an issue before you create this PR, unless the fix is very small.

Not adhering to this guideline will result in the PR being closed.

Tests

  1. PRs will only be merged if tests pass on CI. To run the tests locally, please set up your Gradio environment locally and run the tests: bash scripts/run_all_tests.sh

  2. You may need to run the linters: bash scripts/format_backend.sh and bash scripts/format_frontend.sh

@gradio-pr-bot
Copy link
Collaborator

gradio-pr-bot commented Nov 6, 2023

🪼 branch checks and previews

Name Status URL
Spaces ready! Spaces preview
Website ready! Website preview
Storybook ready! Storybook preview
Visual tests all good! Build review
🦄 Changes detected! Details

Install Gradio from this PR

pip install https://gradio-builds.s3.amazonaws.com/5840b480bbd93504393af02878a52f709ab71d73/gradio-4.1.1-py3-none-any.whl

Install Gradio Python Client from this PR

pip install "gradio-client @ git+https://github.com/gradio-app/gradio@5840b480bbd93504393af02878a52f709ab71d73#subdirectory=client/python"

@gradio-pr-bot
Copy link
Collaborator

gradio-pr-bot commented Nov 6, 2023

🦄 change detected

This Pull Request includes changes to the following packages.

Package Version
@gradio/client minor
@gradio/upload minor
gradio minor
  • Maintainers can select this checkbox to manually select packages to update.

With the following changelog entry.

Provide status updates on file uploads

Maintainers or the PR author can modify the PR title to modify this entry.

Something isn't right?

  • Maintainers can change the version label to modify the version bump.
  • If the bot has failed to detect any changes, or if this pull request needs to update multiple packages to different versions or requires a more comprehensive changelog entry, maintainers can update the changelog file directly.

@freddyaboulton
Copy link
Collaborator Author

I think this is in good enough shape to get some thoughts from people!

@freddyaboulton freddyaboulton marked this pull request as ready for review November 6, 2023 20:41
@abidlabs
Copy link
Member

abidlabs commented Nov 6, 2023

This is awesome @freddyaboulton!

@abidlabs
Copy link
Member

abidlabs commented Nov 6, 2023

I do notice the flicker as well when I test it out -- my thinking is that this means we should change the UI a bit. One idea would be instead of a flicker that covers the whole component, a % that shows up in place of the file size while the file is being uploaded

@freddyaboulton freddyaboulton requested review from abidlabs, aliabid94, aliabd and dawoodkhan82 and removed request for whitphx November 6, 2023 20:59
@aliabid94
Copy link
Collaborator

aliabid94 commented Nov 6, 2023

There's a bit of flicker if the uploaded file is really small. Should we only show progress for large files? My thought is no because it depends on the network speed.

We had this problem with StatusTracker where quick functions had a flicker of showing the progress animation and then disappearing. I solved this by adding a transparency animation of 0.1s, so the progress bar quickly comes into view and leaves the view, but for very quick functions it basically never has time to transition into view at all. See here:

transition: opacity 0.1s ease-in-out;

@pngwn
Copy link
Member

pngwn commented Nov 6, 2023

Another approach is to set a threshold, or 0.2s or so. If something takes less than the threshold, do nothing, otherwise show the animation.

@aliabid94
Copy link
Collaborator

Another approach is to set a threshold, or 0.2s or so. If something takes less than the threshold, do nothing, otherwise show the animation.

if it ends up taking 0.21 seconds, you'd still get a flicker though right?

@pngwn
Copy link
Member

pngwn commented Nov 6, 2023

Yah you would (although you could also animate it). But the same is kinda true with an animation, you can still get flashes if the timing is wrong, they are just a little gentler. I don't think there is a great way to solve it though.

There are APIs (in chrome) to check for networks speed / datasaving mode, but they aren't super informative.

Copy link
Collaborator

@aliabid94 aliabid94 left a comment

Choose a reason for hiding this comment

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

Code looks good to me!
I was thinking about the design of the progress update requests being called directly from UploadProgress.svelte. In my mind, it would have been cleaner if the client handled tracking the upload progress (bc it's the client's responsibility to handle communication with the backend) and the client.upload method can take a callback function that it calls with upload progress updates. Don't feel too strongly about it though.

@freddyaboulton
Copy link
Collaborator Author

Thanks for the reviews all! Implemented the opacity transition to reduce flicker. Going to merge but @hannahblair is going to open a PR to improve the progress bar styling later this week!

@freddyaboulton freddyaboulton merged commit f1409f9 into main Nov 8, 2023
@freddyaboulton freddyaboulton deleted the file-upload-progress branch November 8, 2023 00:02
@pngwn pngwn mentioned this pull request Nov 8, 2023
@abidlabs
Copy link
Member

abidlabs commented Nov 8, 2023

Perhaps we should have avoided merging this in until the UI was ready as it blocks doing a release from main? (not a huge deal)

@pngwn
Copy link
Member

pngwn commented Nov 8, 2023

Yeah. We should always keep main releasable (it also muddies the change log).

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.

Improve /upload experience while file is still loading (?)
5 participants