-
Notifications
You must be signed in to change notification settings - Fork 34
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
[MOD-3267] progress bar for volume/nfs put/get #1969
Merged
kramstrom
merged 22 commits into
main
from
kramstrom/add-volume-file-upload-progress-bar
Jul 16, 2024
Merged
Changes from 17 commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
8ca2bd6
WIP: volume file upload progress bar
kramstrom 52632dc
Update progress
kramstrom 2bcb28c
Fix tests
kramstrom fac0510
Remove unused code
kramstrom 28fede4
optional
kramstrom a576316
make callable
kramstrom 13a2772
Merge branch 'main' of github.com:modal-labs/modal-client into kramst…
kramstrom 5bd2df1
add download progress bars to modal volume cli
kramstrom 1da6d9e
Merge branch 'main' into kramstrom/add-volume-file-upload-progress-bar
kramstrom 4bc02b6
import
kramstrom 8a99dad
type
kramstrom 941b08f
nfs uploader
kramstrom 909b4e3
lambda
kramstrom dc9336d
more progress bars!
kramstrom 47b0a63
complete tasks
kramstrom dde885c
remove
kramstrom cbcd50f
Merge branch 'main' into kramstrom/add-volume-file-upload-progress-bar
kramstrom 18c9b7b
Merge branch 'main' into kramstrom/add-volume-file-upload-progress-bar
kramstrom 3bec3a6
Merge branch 'main' into kramstrom/add-volume-file-upload-progress-bar
kramstrom 8659be8
optional stuff
kramstrom 28dd22d
Merge branch 'main' of github.com:modal-labs/modal-client into kramst…
kramstrom 568bdd5
complete tasks
kramstrom File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -53,6 +53,7 @@ def __init__( | |||||
segment_start: int, | ||||||
segment_length: int, | ||||||
chunk_size: int = DEFAULT_SEGMENT_CHUNK_SIZE, | ||||||
progress_report_cb: Optional[Callable] = None, | ||||||
): | ||||||
# not thread safe constructor! | ||||||
super().__init__(bytes_io) | ||||||
|
@@ -63,6 +64,7 @@ def __init__( | |||||
self._value.seek(self.initial_seek_pos + segment_start) | ||||||
assert self.segment_length <= super().size | ||||||
self.chunk_size = chunk_size | ||||||
self.progress_report_cb = progress_report_cb or (lambda *_, **__: None) | ||||||
self.reset_state() | ||||||
|
||||||
def reset_state(self): | ||||||
|
@@ -74,6 +76,12 @@ def reset_state(self): | |||||
def reset_on_error(self): | ||||||
try: | ||||||
yield | ||||||
except Exception as exc: | ||||||
try: | ||||||
self.progress_report_cb(reset=True) | ||||||
except Exception as cb_exc: | ||||||
raise cb_exc from exc | ||||||
raise exc | ||||||
finally: | ||||||
self.reset_state() | ||||||
|
||||||
|
@@ -100,9 +108,13 @@ async def safe_read(): | |||||
chunk = await safe_read() | ||||||
while chunk and self.remaining_bytes() > 0: | ||||||
await writer.write(chunk) | ||||||
self.progress_report_cb(advance=len(chunk)) | ||||||
chunk = await safe_read() | ||||||
if chunk: | ||||||
await writer.write(chunk) | ||||||
self.progress_report_cb(advance=len(chunk)) | ||||||
|
||||||
self.progress_report_cb(complete=True) | ||||||
|
||||||
def remaining_bytes(self): | ||||||
return self.segment_length - self.num_bytes_read | ||||||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
because it's called by I think this might not be caught because this function is missing its return type annotation, which should be |
||||||
): | ||||||
upload_coros = [] | ||||||
file_offset = 0 | ||||||
|
@@ -187,6 +200,7 @@ async def perform_multipart_upload( | |||||
segment_start=file_offset, | ||||||
segment_length=part_length_bytes, | ||||||
chunk_size=upload_chunk_size, | ||||||
progress_report_cb=progress_report_cb, | ||||||
) | ||||||
upload_coros.append(_upload_to_s3_url(part_url, payload=part_payload, content_type=None)) | ||||||
num_bytes_left -= part_length_bytes | ||||||
|
@@ -230,7 +244,9 @@ def get_content_length(data: BinaryIO): | |||||
return content_length - pos | ||||||
|
||||||
|
||||||
async def _blob_upload(upload_hashes: UploadHashes, data: Union[bytes, BinaryIO], stub) -> str: | ||||||
async def _blob_upload( | ||||||
upload_hashes: UploadHashes, data: Union[bytes, BinaryIO], stub, progress_report_cb: Optional[Callable] = None | ||||||
) -> str: | ||||||
if isinstance(data, bytes): | ||||||
data = io.BytesIO(data) | ||||||
|
||||||
|
@@ -253,9 +269,12 @@ async def _blob_upload(upload_hashes: UploadHashes, data: Union[bytes, BinaryIO] | |||||
part_urls=resp.multipart.upload_urls, | ||||||
completion_url=resp.multipart.completion_url, | ||||||
upload_chunk_size=DEFAULT_SEGMENT_CHUNK_SIZE, | ||||||
progress_report_cb=progress_report_cb, | ||||||
) | ||||||
else: | ||||||
payload = BytesIOSegmentPayload(data, segment_start=0, segment_length=content_length) | ||||||
payload = BytesIOSegmentPayload( | ||||||
data, segment_start=0, segment_length=content_length, progress_report_cb=progress_report_cb | ||||||
) | ||||||
await _upload_to_s3_url( | ||||||
resp.upload_url, | ||||||
payload, | ||||||
|
@@ -274,9 +293,9 @@ async def blob_upload(payload: bytes, stub) -> str: | |||||
return await _blob_upload(upload_hashes, payload, stub) | ||||||
|
||||||
|
||||||
async def blob_upload_file(file_obj: BinaryIO, stub) -> str: | ||||||
async def blob_upload_file(file_obj: BinaryIO, stub, progress_report_cb: Optional[Callable] = None) -> str: | ||||||
upload_hashes = get_upload_hashes(file_obj) | ||||||
return await _blob_upload(upload_hashes, file_obj, stub) | ||||||
return await _blob_upload(upload_hashes, file_obj, stub, progress_report_cb) | ||||||
|
||||||
|
||||||
@retry(n_attempts=5, base_delay=0.1, timeout=None) | ||||||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.