Display download progress for file downloads #2327
Conversation
legoktm
force-pushed
the
download-progress
branch
from
41d1174 to
ec75f0b
Compare
|
So right now I've implemented an exponential moving average based on https://stackoverflow.com/questions/2779600/how-to-estimate-download-time-remaining-accurately; however I missed the comment which says, "EMA will only work if the time-sampling rate is about the same. If, for example the download is updated every 1 MB rather than every 1 second and the speed fluctuates the output will most likely be nonsense". Unfortunately that's exactly what we're doing, we update on chunks rather than on a regular time interval. Will require some more 🤔 |
legoktm
force-pushed
the
download-progress
branch
2 times, most recently
from
cb0c67f to
1571296
Compare
|
I got a smoothed version of the download speed to work (by adjusting the SMOOTHING_FACTOR to be relative to the time period), but there's a more practical problem: we only update the speed after we receive a chunk, so if it stalls, we don't update the speed and it'll get stuck at e.g. 4MB/s. So I want to rework this a bit, I'll move the calculation stuff into the widget and we can use a QTimer to update the widget's speed on an actual time interval. |
legoktm
force-pushed
the
download-progress
branch
4 times, most recently
from
6ae75e9 to
0eb51f6
Compare
| self.update_display() | ||
|
|
||
| def proxy(self) -> ProgressProxy: | ||
| """Get a proxy that updates this widget.""" |
There was a problem hiding this comment.
This stuck out to me a bit, because it means that the widget, which is otherwise "naive"/encapsulated and needs no outside knowledge of the sdk or any components, now has an internal dependency on the ProgressProxy. What would you think about the widget either taking a ProgressProxy object in its constructor (clearer dependency graph +/ easier to test) or being completely agnostic to how it's receiving updates as long as it exposes a @pyqtSlot that takes an int parameter and updates itself accordingly?
# in the parent widget
progressbar = FileDownloadProgressBar(self.file.size)
proxy = ProgressProxy(progressbar.handle)
self.controller.on_submission_download(File, self.file.uuid, proxy)
There was a problem hiding this comment.
I wanted to encapsulate the construction of ProgressProxy in a single place so that if we e.g. wanted to add another signal, it only needs to be changed in one file. This isn't strictly true because we do have ProgressProxy(None) but it's close.
Also, my original design was that the SDK would have a Progress class that would be independent of PyQt but that didn't really work and I ended up needing the signal. What do you think if I moved the ProgressProxy class into the same file as the widget, since it's effectively tied together with that?
There was a problem hiding this comment.
What do you think if I moved the ProgressProxy class into the same file as the widget, since it's effectively tied together with that?
I've done this now so you can take a look at what it looks like.
| @@ -842,6 +850,7 @@ def _submit_download_job( | |||
| job.success_signal.connect(self.on_file_download_success) | |||
| job.failure_signal.connect(self.on_file_download_failure) | |||
|
|
|||
| job.progress = progress | |||
There was a problem hiding this comment.
I was trying to figure out if there was any way of logically gating this (eg if it's a db.File then it always should have a non-null progress or there's an error worth logging), but that may not be true.
There was a problem hiding this comment.
Right, I think we just treat it as ProgressProxy | None all the way through until we actually call methods on it.
legoktm
force-pushed
the
download-progress
branch
from
0eb51f6 to
78020fc
Compare
|
Thanks for all the feedback @rocodes!
Yes please, whenever you have time. |
legoktm
force-pushed
the
download-progress
branch
from
78020fc to
0abe286
Compare
|
(Still not 100% ready, but moving out of draft state so we can get notifications, etc.) |
legoktm
force-pushed
the
download-progress
branch
2 times, most recently
from
b672c75 to
1752ae0
Compare
Display a vanilla progress bar for file downloads that simply shows how much of the file has been downloaded so far. A new FileDownloadProgressBar widget replaces the existing animated spinner, and we inject a signal through to the SDK to pass the current progress back to the widget. The widget both displays the overall total progress as a percentage and also calculates the download speed by using an exponential moving average to smooth it out. A timer runs every 100ms to recalculate the speed. A new utils.humanize_speed() is used to translate the raw bytes/second into a human-readable version with a focus on keeping the length of the string roughly consistent so there's less visual shifting. Once the download has finished, an indeterminate progress bar is shown while decrypting since we don't (currently) have a way to report specific progress on that. TODO: * tests? Fixes #1104.
legoktm
force-pushed
the
download-progress
branch
from
1752ae0 to
a3a5504
Compare
Status
Work in progress
TODO:
Description
Display a vanilla progress bar for file downloads that simply shows how much of the file has been downloaded so far.
A new FileDownloadProgressBar widget replaces the existing animated spinner, and we inject a signal through to the SDK to pass the current progress back to the widget.
Fixes #1104.
Test Plan
NUM_SOURCES=10 --random-file-size 100000to generate 10 sources with 100MB attachmentsChecklist