-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[REF-723+] Upload with progress and cancellation #1899
Conversation
Address #1896 REF-723
Process delta and queued events returned in response to an upload REST event. Allows upload to reliably return deltas in prod mode, with redis. Fix: REF-1037
The upload handler may yield multiple deltas and events, and these should not wait until the entire upload handler completes, since it may take some time to send the uploaded files to disk/s3 and it would be nice to send updates to the frontend. Since axios does not support streaming output natively in the browser, we reuse the onDownloadProgress functionality to get access to the partial response, split the JSON structures, and hand them off to the websocket.on("event") handler, to be processed as-if they came from the websocket itself. applyRestEvent no longer waits for the upload to complete, and all events from the backend upload handler are marked as final so that upload events and chained events do not block the websocket event queue.
(now that py37 is dropped, we can use `get_args`)
Instead of including them in the filename, pass these identifiers in the headers, which simplifies processing.
Add a test for canceling an upload, and ensure that it reported some progress along the way.
If the handler takes the lock outside the processing function, it ends up dropping the lock while streaming and the updates never make it into redis.
Simplify logic of existing negative test.
b60d388
to
863b479
Compare
If the upload completes and triggers the handler before the progress event is sent, then progress will be after the completed upload marker.
if isinstance(func, EventHandler): | ||
if func.is_background: | ||
raise TypeError( | ||
f"@rx.background is not supported for upload handler `{handler}`.", |
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.
What's the reason for excluding background tasks as upload handler?
Does it cause issues somewhere?
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.
The problem is that a background task relies on websocket to send the deltas to the frontend, but the upload event comes via REST and might hit an app instance that is not managing the websocket associated with the session that made the upload. So if the upload handler is a background task, and the response is handled by a different instance the deltas wont be sent and the user will file a bug or just be confused why things work in dev mode and not work with redis.
Ultimately want to solve this with REF-1073, but it's non-trivial.
let eventSent = false; | ||
if (event.handler == "uploadFiles") { | ||
eventSent = await uploadFiles(event.name, event.payload.files); | ||
// Start upload, but do not wait for it, which would block other events. |
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.
Nice!
reflex/components/forms/upload.py
Outdated
DEFAULT_UPLOAD_ID: str = "default" | ||
|
||
|
||
def upload_file_for(id_: str = DEFAULT_UPLOAD_ID) -> BaseVar: |
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.
Just my preference - but can we remove the for
and have this just be upload_file
or get_upload_file
. I think it matches the rest of our API better as we don't do this in other places
reflex/components/forms/upload.py
Outdated
upload_file: BaseVar = upload_file_for() | ||
|
||
|
||
def selected_files_for(id_: str = DEFAULT_UPLOAD_ID) -> BaseVar: |
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.
Similarly can this become selected_files
or get_selected_files
clear_selected_files: EventSpec = clear_selected_files_for() | ||
|
||
|
||
def cancel_upload(upload_id: str) -> EventSpec: |
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.
It will match this API also, since this doesn't have the for
These classes are used as decorator over a function returning a BaseVar or EventSpec, the resulting name can be used as either a function, in which case it passes through to the decorated function, or as a value, in which case it will take on the apparent value of calling the function with no arguments.
19201a9
to
b11ac48
Compare
Address #1896
REF-723: Progress Display for rx.upload
REF-1037: In prod mode, upload sometimes cannot send delta
REF-611:
files: List[rx.UploadFile]
does not work withfrom __future__ import annotations
Improvements
on_upload_progress
kwarg torx.upload_files
and receive a callback as the file is uploadingrx.upload
(and related functions) now accept anid
orupload_id
kwarg, which allows multiple upload forms to co-exist on the same page and be serviced by different handlersrx.cancel_upload(id)
allows for in-progress uploads to be cancelled by an event trigger (like on_click or yield from event handler).file.path
instead offile.name
to support directory uploadslist[rx.UploadFile]
annotation in upload handlertoken
andhandler
via headers instead of munging them into the filename.Sample Code
☝️ Separate, independently operating upload forms!!