Feature/file uploader abstractions #7025

kajarenc · 2023-07-17T16:40:00Z

Describe your changes

This PR change is a reworking of a process to upload files for st.file_uploader and st.camera_input.
It introduces the abstraction for UploadedFileManager, the default implementation continues to store files in memory.

Now the process of uploading file will consist from two steps

Issuing upload file URLs (happen via web socket communication, file_urls_request handler).
Upload a file to the URL issued in step 1.

GitHub Issue Link (if applicable)

Testing Plan

Explanation of why no additional tests are needed
Unit Tests (JS and/or Python) DONE
E2E Tests DONE
Any manual testing needed? DONE , testing app in Streamlit Cloud: https://file-uploader-test-k6mmsgecmepiecqfsnquhk.streamlit.app/

Contribution License Agreement

By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

frontend/lib/src/components/widgets/FileUploader/UploadFileInfo.ts

lib/streamlit/elements/file_uploader.py

lib/streamlit/runtime/memory_uploaded_file_manager.py

frontend/lib/src/FileUploadClient.test.ts

* Uploaded file manager protocol * Implement MemoryUploadFileManager * Fix failing tests, not related to file uploader * Add some tests for UploadFileRequestHandlerTest * formatter fixes * fix all tests in uploaded_file_request_handler.py * Tweak some comments and add some TODOs * Add another TODO --------- Co-authored-by: Karen Javadyan <kajarenc@gmail.com> Co-authored-by: Vincent Donato <vincent@streamlit.io>

…and FileUploadClient (#6754) * Start supporting configurable file upload URLs in StreamlitEndpoints and FileUploadClient * Clarify some comments and fix formatting * Remove widgetId from file upload related methods * Have csrfRequest assume it always gets an absolute URL

* Add new proto definitions for file upload URL requests/responses * Add server websocket handler for file_urls_request BackMsgs * Rename FileUrls* -> FileURLs* * Install uuid types * Mark resolver fields as readonly * Allow file URLs to be requested via websocket * Add temporary # type: ignore * Remove HTTP endpoints for fetching file URLs * Remove unused variable to appease eslint

* * add `file_delete_url` to `UploadedFileInfo` proto * add `get_upload_urls` to `UploadedFileManager` * `UploadedFileManager` now works with file_id to get_files. * Add todos * remove abstractmothod from get_upload_urls

* Have UploadedFileManager react client fetch and use upload URLs * Fix issues from rebase

* reimplement camera_input to work with new uploaded file manager * call delete endpoint when clear camera_input

* Change some types from Promise<number> to Promise<void> * Remove serverFileId and related fields from FileUploader and friends * Remove serverFileId from CameraInput * Remove more newly unused fields from CameraInput and FileUploader

* extract FileUrls to separate proto * use copy from * use FileURLS also in camera_input * fixes after review

* Run autoformatter on e2e/specs/st_file_uploader.spec.js * Fix some more small type errors * Fix watched cypress route in st_file_uploader.spec.js

docs and tests

* Fix a bunch of FileUploader js unit tests * Fix a bunch of type errors in tests

* Wait until file uploaded instead of fix amount of time * fix tests for CameraInput fix tests for DefaultStreamlitEndpoints * fix eslint errors * add uuid to NOTICES

kajarenc · 2023-07-27T15:32:49Z

lib/streamlit/runtime/state/session_state.py

@@ -179,6 +179,8 @@ def set_widget_metadata(self, widget_meta: WidgetMetadata[Any]) -> None:

    def remove_stale_widgets(self, active_widget_ids: set[str]) -> None:
        """Remove widget state for widgets whose ids aren't in `active_widget_ids`."""
+        # TODO(vdonato / kajarenc): Remove files corresponding to an inactive file
+        # uploader.


Not critical, we keep this for future improvement

vdonato

Some small comments, but overall this LGTM

I won't officially hit the +1 button given so much of this code is mine that I'm pretty sure that I'm not qualified to approve it 😆

vdonato · 2023-07-25T01:01:35Z

lib/streamlit/runtime/runtime.py

-    def _on_files_updated(self, session_id: str) -> None:
-        """Event handler for UploadedFileManager.on_file_added.
-        Ensures that uploaded files from stale sessions get deleted.
-
-        Notes
-        -----
-        Threading: SAFE. May be called on any thread.
-        """
-        if not self._session_mgr.is_active_session(session_id):
-            # If an uploaded file doesn't belong to an active session,
-            # remove it so it doesn't stick around forever.
-            self._uploaded_file_mgr.remove_session_files(session_id)


Hm, I feel like this might be something that we want to keep even in the new world. We still have the mechanism that calls remove_session_files when a session is shut down, but I'm slightly worried of the possibility that a file corresponding to a now nonexistent session is somehow uploaded so is never cleaned up without this.

Would it be possible to revive this callback function but change it to be called when no corresponding session (active or inactive) exists for a given session_id? This happens when self._session_mgr.get_session_info(session_id) is None

Hey!
We check that session is active after the file is fully uploaded, and just before storing it into storage.
https://github.com/streamlit/streamlit/pull/7025/files#diff-3e1711ee513b43ba25f0f9118bfe0135a503ed40da6474b7b2c5ddcef9e940f5R101 , so ~~it shouldn't be possible~~ very unlikely to upload file not corresponding to active sesion

So I don't want to make it part of the protocol but will be happy to return to this during refactoring of removing session_id from URLs, and rethink deleting files mechanism for open source implementation.

vdonato · 2023-07-27T00:01:27Z

lib/tests/streamlit/elements/file_uploader_test.py

+    @patch("streamlit.elements.widgets.file_uploader._get_upload_files")
+    def test_deleted_file_omitted(self, get_upload_files_patch):
+        """We should omit DeletedFile objects for final user value ."""
+
+        uploaded_files = [DeletedFile(file_id="A")]
+        get_upload_files_patch.return_value = uploaded_files

-        st.file_uploader("foo", accept_multiple_files=True)
+        result_1: UploadedFile = st.file_uploader("a", accept_multiple_files=False)
+        result_2: UploadedFile = st.file_uploader("b", accept_multiple_files=True)
+
+        self.assertEqual(result_1, None)
+        self.assertEqual(result_2, [])
+
+    @patch("streamlit.elements.widgets.file_uploader._get_upload_files")
+    def test_deleted_files_filtered_out(self, get_upload_files_patch):


Aren't these two tests essentially identical? I think we can probably remove the first one

Good catch, the second test is a superset of the first test, I removed the first one!

vdonato · 2023-07-27T23:34:03Z

lib/streamlit/elements/widgets/camera_input.py

-def _get_file_recs_for_camera_input_widget(
-    widget_id: str, widget_value: Optional[FileUploaderStateProto]
-) -> List[UploadedFileRec]:
+def _get_upload_files(


Can we factor this out into a helper function shared by camera_input and file_uploader? This function seems to be identical for both of the widgets

Yes, it difficult to find a new home for def _get_upload_files (because it is used only in file_uploader and camera_input, so utils.py is probably not the best place, so I just now imported _get_upload_files from file_uploader for camera_input, I think it is good idea, and denote the fact that camra_input essentially a derivative form file_uploader

vdonato · 2023-07-27T23:37:07Z

lib/streamlit/elements/widgets/file_uploader.py

+SomeUploadedFiles = Optional[
+    Union[UploadedFile, DeletedFile, List[Union[UploadedFile, DeletedFile]]]
+]


nit: can add None to the union instead of having an Optional[Union[...]] to be consistent with camera input

vdonato · 2023-07-27T23:41:32Z

lib/streamlit/elements/widgets/file_uploader.py

+
+        filtered_value: Union[UploadedFile, List[UploadedFile], None]
+
+        if isinstance(widget_state.value, DeletedFile):
+            filtered_value = None
+        elif isinstance(widget_state.value, list):
+            filtered_value = [
+                f for f in widget_state.value if not isinstance(f, DeletedFile)
+            ]
+        else:
+            filtered_value = widget_state.value
+
+        return filtered_value


nit: I think it'd be fine to return the value directly from each branch of the if/elif/else statement rather than save it to an intermediate variable that's returned immediately

vdonato · 2023-07-27T23:42:17Z

lib/streamlit/elements/widgets/file_uploader.py

@@ -225,7 +229,7 @@ def file_uploader(
        *,  # keyword-only arguments:
        disabled: bool = False,
        label_visibility: LabelVisibility = "visible",
-    ) -> SomeUploadedFiles:
+    ) -> Optional[Union[UploadedFile, List[UploadedFile]]]:


nit: same comment about adding None to the union instead of having this be Optional (same thing again with L389)

vdonato · 2023-07-27T23:43:49Z

lib/streamlit/runtime/uploaded_file_manager.py

+class DeletedFile(NamedTuple):
+    """Represents a deleted file in deserialized values for st.file_uploader and
+    st.camera_input
+    Return this from st.file_uploader and st.camera_input deserialize (so they can


nit: should have a newline between the docstring summary and the rest of it

# Conflicts: # lib/streamlit/elements/widgets/camera_input.py # lib/streamlit/elements/widgets/file_uploader.py

LukasMasuch

LGTM 👍 I just have some nits and questions, but no major concern.

frontend/lib/src/FileUploadClient.ts

LukasMasuch · 2023-07-28T14:33:22Z

frontend/lib/src/StreamlitEndpoints.ts

@@ -34,6 +34,14 @@ export interface StreamlitEndpoints {
   */
  buildMediaURL(url: string): string

+  /**
+   * Construct a URL for uploading a file.
+   * @param url a relative or absolute URL. If `url` is absolute, it will be


nit: I think in the JSdoc docstring syntax the parameter is separated via a -, e.g.:

Suggested change

* @param url a relative or absolute URL. If `url` is absolute, it will be

* @param url - a relative or absolute URL. If `url` is absolute, it will be

There are also a few other instances in the PR that could get correct, but it seems that we haven't been following this anyways in the current impl.

LukasMasuch · 2023-07-28T14:45:14Z

proto/streamlit/proto/Common.proto

 }

 message FileUploaderState {
+  // DEPRECATED
  sint64 max_file_id = 1;


Same as the comment above, couldn't we just fully remove this via:

Suggested change

sint64 max_file_id = 1;

reserved 1;

reserved "max_file_id";

I am not sure we are okay to remove / change the type of any proto message field, because of dependencies from other teams.

This is a good question to clarify, to have a solid understanding of the contract we keep for proto messages.
For now, I think the safest thing is just to keep the field as is, with a comment that it is now deprecated.

oki, let's keep it as is 👍 But maybe we bring it up in a standup soon to figure out if it would break other dependencies.

LukasMasuch · 2023-07-28T14:46:21Z

proto/streamlit/proto/Common.proto

 // Information on a file uploaded via the file_uploader widget.
 message UploadedFileInfo {
+  // DEPRECATED.
  sint64 id = 1;


Is this field still used in any way? If not, could we just remove this and add:

Suggested change

sint64 id = 1;

reserved 1;

reserved "id";

lib/streamlit/elements/widgets/file_uploader.py

LukasMasuch · 2023-07-28T15:07:08Z

lib/streamlit/runtime/memory_uploaded_file_manager.py

+        all_files: List[UploadedFileRec] = []
+        # Make copy of self.file_storage for thread safety, to be sure
+        # that main storage won't be changed form other thread
+        file_storage_copy = self.file_storage.copy()


nit: does that mean that all files are also duplicated in memory? So calling this (e.g. via the metrics endpoint), might crash the app because of a sudden memory increase. Not a big deal right now since it isn't used a lot, but if we do more with those stats we might want to revisit this.

yes, now we create a duplicate to collect statistics.
The good news is that this duplicate should be short-living, and Python Garbage Collector should collect it very fast because we don't keep any references to file_storage_copy, but yes, this could lead to memory usage increase in case of a lot of stored uploaded files.

Maybe you can add a TODO comment or something there, so that we double check this once we do more with the stats.

lib/streamlit/runtime/uploaded_file_manager.py

frontend/app/src/App.tsx

Co-authored-by: Lukas Masuch <Lukas.Masuch@gmail.com>

…ient (#7092) We want to avoid having the notebooks team have to make any code changes on their end due to the interface / constructor changes we're making to StreamlitEndpoints and FileUploadClient. In order to do this, we make the newly added methods/args optional.

This PR change is a reworking of a process to upload files for st.file_uploader and st.camera_input. It introduces the abstraction for UploadedFileManager, the default implementation continues to store files in memory. Now the process of uploading file will consist from two steps Issuing upload file URLs (happen via web socket communication, file_urls_request handler). Upload a file to the URL issued in step 1.

github-advanced-security bot found potential problems Jul 17, 2023

View reviewed changes

frontend/lib/src/components/widgets/FileUploader/UploadFileInfo.ts Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems Jul 17, 2023

View reviewed changes

lib/streamlit/elements/file_uploader.py Fixed Show fixed Hide fixed

lib/streamlit/runtime/memory_uploaded_file_manager.py Fixed Show fixed Hide fixed

vdonato force-pushed the feature/file_uploader_abstractions branch from 465d08b to 4430df9 Compare July 18, 2023 00:05

github-advanced-security bot found potential problems Jul 18, 2023

View reviewed changes

frontend/lib/src/FileUploadClient.test.ts Fixed Show fixed Hide fixed

frontend/lib/src/FileUploadClient.test.ts Fixed Show fixed Hide fixed

kajarenc force-pushed the feature/file_uploader_abstractions branch from 738aef1 to f683cd0 Compare July 24, 2023 12:01

vdonato and others added 23 commits July 25, 2023 14:50

Add uuid as a dependency (#6789)

d10f668

Tweak TODO

48e38ff

Backend changes (#6819)

3ef5c68

* * add `file_delete_url` to `UploadedFileInfo` proto * add `get_upload_urls` to `UploadedFileManager` * `UploadedFileManager` now works with file_id to get_files. * Add todos * remove abstractmothod from get_upload_urls

Have UploadedFileManager react client fetch and use upload URLs (#6827)

efd3efe

* Have UploadedFileManager react client fetch and use upload URLs * Fix issues from rebase

Add a TODO

88fa4ce

Camera input with new file_uploader abstractions (#6862)

bb0eed5

* reimplement camera_input to work with new uploaded file manager * call delete endpoint when clear camera_input

Fix a bunch of type errors (#6885)

a19d3e9

* Change some types from Promise<number> to Promise<void> * Remove serverFileId and related fields from FileUploader and friends * Remove serverFileId from CameraInput * Remove more newly unused fields from CameraInput and FileUploader

Feature/file uploader file urls (#6902)

f8c7720

* extract FileUrls to separate proto * use copy from * use FileURLS also in camera_input * fixes after review

Start work on fixing file uploader and camera input e2e tests (#6891)

8fd0921

* Run autoformatter on e2e/specs/st_file_uploader.spec.js * Fix some more small type errors * Fix watched cypress route in st_file_uploader.spec.js

Fix issue with file uploader dropping every other file (#6912)

e0033f5

File uploader/docs and tests (#6945)

4d1d296

docs and tests

Update yarn.lock

d6bfaf6

Fix import

a06ae0e

Fix a bunch of FileUploader js unit tests (#7019)

5e7a959

* Fix a bunch of FileUploader js unit tests * Fix a bunch of type errors in tests

Remove some unneeded TODOs

ba40f38

fix some code ql warnings (#7031)

0d40be3

add patch address

3702f13

Remove outdated test

8ba9638

Fix CodeQL errors, docstrings, and other misc things

1ea7806

add uuid back to dependency list

d45a488

kajarenc force-pushed the feature/file_uploader_abstractions branch from a2831c4 to d45a488 Compare July 25, 2023 10:56

Fix cypress failing tests, jstests and regenerate Notices (#7075)

29493cb

* Wait until file uploaded instead of fix amount of time * fix tests for CameraInput fix tests for DefaultStreamlitEndpoints * fix eslint errors * add uuid to NOTICES

Remove updated TODOs

a9a542e

kajarenc added security-assessment-completed change:refactor impact:internal labels Jul 26, 2023

kajarenc marked this pull request as ready for review July 26, 2023 21:50

kajarenc requested review from vdonato, LukasMasuch and sfc-gh-mnowotka as code owners July 26, 2023 21:50

vdonato and others added 2 commits July 26, 2023 17:07

Fix docstring

b7020c4

fix docstring and remove unused import

89dfa4c

kajarenc commented Jul 27, 2023

View reviewed changes

vdonato reviewed Jul 27, 2023

View reviewed changes

kajarenc added 4 commits July 28, 2023 14:51

Merge branch 'remote-develop' into feature/file_uploader_abstractions

c95c53d

# Conflicts: # lib/streamlit/elements/widgets/camera_input.py # lib/streamlit/elements/widgets/file_uploader.py

Changes after review

4ec759f

fix docstrings

448daf6

replace Optional[Union[X]], with Union[X, None]

51a7ade

LukasMasuch approved these changes Jul 28, 2023

View reviewed changes

kajarenc and others added 3 commits July 28, 2023 20:30

add docstring

20622e4

Update frontend/lib/src/FileUploadClient.ts

436b52e

Co-authored-by: Lukas Masuch <Lukas.Masuch@gmail.com>

kajarenc merged commit a2bd07f into develop Jul 31, 2023
86 checks passed

vdonato mentioned this pull request Aug 9, 2023

Stop dropping file uploader state after websocket disconnect #7149

Merged

vdonato mentioned this pull request Oct 12, 2023

Allow a service managing a Streamlit runtime to set its own session IDs #7544

Merged

vdonato deleted the feature/file_uploader_abstractions branch November 1, 2023 23:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/file uploader abstractions #7025

Feature/file uploader abstractions #7025

kajarenc commented Jul 17, 2023 •

edited

kajarenc Jul 27, 2023

vdonato left a comment

vdonato Jul 25, 2023

kajarenc Jul 28, 2023

vdonato Jul 27, 2023

kajarenc Jul 28, 2023

vdonato Jul 27, 2023

kajarenc Jul 28, 2023

vdonato Jul 27, 2023

kajarenc Jul 28, 2023

vdonato Jul 27, 2023

kajarenc Jul 28, 2023

vdonato Jul 27, 2023

kajarenc Jul 28, 2023

vdonato Jul 27, 2023

kajarenc Jul 28, 2023

LukasMasuch left a comment

LukasMasuch Jul 28, 2023 •

edited

LukasMasuch Jul 28, 2023

kajarenc Jul 28, 2023

LukasMasuch Jul 30, 2023

LukasMasuch Jul 28, 2023

LukasMasuch Jul 28, 2023 •

edited

kajarenc Jul 28, 2023

LukasMasuch Jul 29, 2023

	* @param url a relative or absolute URL. If `url` is absolute, it will be

	* @param url - a relative or absolute URL. If `url` is absolute, it will be

Feature/file uploader abstractions #7025

Feature/file uploader abstractions #7025

Conversation

kajarenc commented Jul 17, 2023 • edited

Describe your changes

GitHub Issue Link (if applicable)

Testing Plan

Choose a reason for hiding this comment

vdonato left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukasMasuch left a comment

Choose a reason for hiding this comment

LukasMasuch Jul 28, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukasMasuch Jul 28, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kajarenc commented Jul 17, 2023 •

edited

LukasMasuch Jul 28, 2023 •

edited

LukasMasuch Jul 28, 2023 •

edited