[MOD-3267] progress bar for volume/nfs put/get #1969
Conversation
…rom/add-volume-file-upload-progress-bar
kramstrom
changed the title
kramstrom
changed the title
| def _advance_sub_task(self, task_id: TaskID, advance: float): | ||
| self._download_progress.update(task_id, advance=advance) | ||
|
|
||
| def progress( |
There was a problem hiding this comment.
Perhaps we should wrap this in a try-catch block and just log warnings instead of crashing if the progress handler breaks, unintuitive for the user but at least their upload/downloads wouldn't stop for some UI crash
There was a problem hiding this comment.
Hmm possibly. If the failure case is a bug then I'd lean on hard-failing to surface them more prominently. I don't there's really are other failure cases because this is all in-memory or writing to stdout, which have a very low probability of a transient fault.
kramstrom
changed the title
|
Nice! |
|
Fantastic work |
|
Looks really nice! Apologies if this is a dumb question as I just skimmed the diff so far, but will this avoid creating thousands of progress bars if people upload a directory with many files? |
Not at all! Since the progress bars are wrapped in a |
👌 perfect! |
| def _advance_sub_task(self, task_id: TaskID, advance: float): | ||
| self._download_progress.update(task_id, advance=advance) | ||
|
|
||
| def progress( |
There was a problem hiding this comment.
Hmm possibly. If the failure case is a bug then I'd lean on hard-failing to surface them more prominently. I don't there's really are other failure cases because this is all in-memory or writing to stdout, which have a very low probability of a transient fault.
modal/_utils/blob_utils.py
Outdated
| @@ -165,6 +177,7 @@ async def perform_multipart_upload( | |||
| part_urls: List[str], | |||
| completion_url: str, | |||
| upload_chunk_size: int = DEFAULT_SEGMENT_CHUNK_SIZE, | |||
| progress_report_cb: Callable, | |||
There was a problem hiding this comment.
| progress_report_cb: Callable, | |
| progress_report_cb: Optional[Callable], |
because it's called by _blob_upload which itself has an optional param.
I think this might not be caught because this function is missing its return type annotation, which should be -> None
…rom/add-volume-file-upload-progress-bar
Adds progress bars when uploading and downloading files / directories to volumes and network file systems
Example screenshot of what it looks like when in progress
We still have some magic spinner for "post processing" once files are uploaded to the server but the server is doing some work. Added a TODO for that but it would require more work on the server to send progress reports which we don't currently have