Clean up file update calls #2597

karriebear · 2021-01-13T17:48:52Z

Issue: #2561

Description:
Unclear why the deploy button caused a regression, potentially because of new message type. Looking at the changes for the deploy button, it looks correct. Instead cleaned up areas of file uploader to become more stable.

Changes include

on_files_updated was being signaled more than required. Minimized requests to update.
FileUploadRequestHandler POST and DELETE will handle updating the file count instead of a separate request. On file upload, pass in the total number of files that should be uploaded for the session/widget
Always remove file on delete in parallel to the request. We remove it anyways on error.

Tests added

Upload and delete for single and multiple file uploader
Replace file on upload in single file uploader

Test cases
[ ] Works with multiple sessions
[ ] Upload single file
[ ] Upload another single file replaces initial file
[ ] Upload multiple files
[ ] Delete file(s)

Contribution License Agreement

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

tconkling

Without tests, it's hard to understand what the broken behavior currently is, and how this fixes it.

If I understand correctly, there are circumstances where UploadedFileManager.on_files_updated is being emitted too frequently, and this PR fixes that issue?

(What's the harm caused the the signal being emitted too frequently?)

Is it possible to add a test that fails on the current, broken behavior and succeeds with this PR, so that we can prevent a regression?

Same with FileUploader.tsx - is it possible to add a test that validates the setState is called the expected number of times (or whatever) for file removals?

karriebear · 2021-01-13T20:08:11Z

That's kind of the difficult part. We do have a test that was failing but it seems like the regression itself was flaky (#2567). And tbh I don't quite get how all this relates to the deploy button change. However as I started looking into the regression I noticed that we were making a lot of calls that wasn't really needed. When I optimized all that it seems like I can't reproduce the issue anymore 🤷‍♀️

I'll add in some unit tests that specifically tests the replace functionality and a unit test to make sure removeFile calls updateFileCount with the right params

…eploy-regression

tconkling

Hey, I'm being a total pain in the ass, and I apologize :( - I'm just finding a lot of little things to be surprising or confusing. I've left a number of requests for comments.

I'm also wondering if there's a cleaner way to track "number of uploaded files":

In Python, why are we tracking UploadedFileManager._file_counts_by_id, and how does that number relate to the lengths of the UploadedFileManager._files_by_id lists?
In the frontend, why are we tracking FileUploader.state.validFiles and how does that number relate to the length of the FileUploader.state.files array?

In general, I'm wary of using multiple independent bits of state to track a single thing, because they can get out of sync, and it makes future changes more tricky to reason about. Is there a more atomic way of tracking these things?

e2e/specs/st_file_uploader.spec.js

frontend/src/components/widgets/FileUploader/FileUploader.tsx

tconkling · 2021-01-19T23:31:30Z

frontend/src/components/widgets/FileUploader/FileUploader.tsx

    this.props.uploadClient
      .uploadFiles(
        this.props.element.id,
-        [file],
+        [updatedFile],
+        this.state.validFiles,


This isn't your doing, but I find the updatedFile vs file thing happening here to be really confusing. It looks like this.handleFile is returning a new object, but it's actually just mutating its passed-in object, which means that file and updatedFile are the same thing. (I wish that "handleFile" just didn't return the passed-in file.)

It was my doing :(

tconkling · 2021-01-19T23:34:08Z

frontend/src/components/widgets/FileUploader/FileUploader.tsx

+  public removeFile = (fileId: string): void => {
+    this.setState(state => {
+      const filteredFiles = state.files.filter(file => file.id !== fileId)
+
+      return {
+        status: filteredFiles.length ? FileStatus.UPLOADED : FileStatus.READY,
+        errorMessage: undefined,
+        files: filteredFiles,
+        validFiles: state.validFiles - 1,
+      }


What happens if this function is called with a non-existent fileId? (Or with a fileId that's not "valid"). Is that possible? It looks like validFiles would be improperly decremented. Is there a sanity check we can do here, to throw an error if someone down the road calls the function improperly?

True, we can do something like state.validFiles - (state.files.length - filteredFiles.length)

frontend/src/lib/FileUploadClient.ts

tconkling · 2021-01-19T23:43:01Z

lib/streamlit/server/upload_file_request_handler.py

+        try:
+            total_files = self._require_arg(args, "totalFiles")
+            self._file_mgr.update_file_count(
+                session_id=session_id,
+                widget_id=widget_id,
+                file_count=int(total_files),
+            )
+
+        except Exception as e:
+            self._file_mgr.update_file_count(
+                session_id=session_id,
+                widget_id=widget_id,
+                file_count=1,
+            )
+


Is this try/except handling errors thrown by _require_arg or update_file_count or both? Why would we want to swallow an error thrown here? (My assumption from glancing at the code is that it's to catch a missing "totalFiles" argument - if that's the case, why would this argument be missing? Is it optional from the client?)

It handles the errors from _require_arg. It's not optional from what we have currently on the client side but I left it open so that it could work with in the future with totalFiles as an optional param. We can make it an optional parameter on the client side if that's preferred.

Cool. How about this slight tweak:

try: total_files = int(self._require_arg(args, "totalFiles")) except: total_files = 1 self._file_mgr.update_file_count( session_id=session_id, widget_id=widget_id, file_count=total_files )

This way we don't erroneously catch exceptions thrown by update_file_count

lib/streamlit/uploaded_file_manager.py

tconkling

Thanks! The additional comments you added make a huge difference.

tconkling · 2021-01-27T00:48:43Z

e2e/specs/st_file_uploader.spec.js

+
+        // Can delete
+        cy.get("[data-testid='fileDeleteBtn'] button")
+          .first()
+          .click();


Is this missing a check that the file's actually been deleted, after the button is clicked?

tconkling · 2021-01-27T01:00:33Z

lib/streamlit/server/upload_file_request_handler.py

+        try:
+            total_files = self._require_arg(args, "totalFiles")
+            self._file_mgr.update_file_count(
+                session_id=session_id,
+                widget_id=widget_id,
+                file_count=int(total_files),
+            )
+
+        except Exception as e:
+            self._file_mgr.update_file_count(
+                session_id=session_id,
+                widget_id=widget_id,
+                file_count=1,
+            )
+


Cool. How about this slight tweak:

try: total_files = int(self._require_arg(args, "totalFiles")) except: total_files = 1 self._file_mgr.update_file_count( session_id=session_id, widget_id=widget_id, file_count=total_files )

This way we don't erroneously catch exceptions thrown by update_file_count

asaini · 2021-02-02T06:45:51Z

LGTM. Verified functionality with multiple CSV files.

Normal file uploader functionality also looks good!

* develop: Fix svg rendering in st.image() (streamlit#2666) Indicate widgets need to have labels (streamlit#2688) Clean up file update calls (streamlit#2597) Revert "🔧 "Run on save" now defaults to true (streamlit#2641)" (streamlit#2694)

* Clean up file update calls * Add more tests * Move updating of file count to upload file request handler * Fix tests * Fix deleting * Address PR comments * PR cleanup * Fix test

* develop: Autofocus "clear cache" button (#2739) Fix checkbox spacing (#2738) Fix progress bar alignment (#2734) Fix select menu not autoselecting first item (#2732) Show user command line in disconnect dialog (#2735) Up version to 0.76.0 Update change log Improve usage warning wording and formatting Finally remove beta versions of set_page_config and color_picker (#2711) Finally remove beta versions of set_page_config and color_picker (#2711) Clean up file update calls (#2597)

* develop: Slider thumb values are always visible (streamlit#2724) Update component-template submodule to latest (streamlit#2767) `allow_multiple_files` -> `accept_multiple_files` (streamlit#2761) Autofocus "clear cache" button (streamlit#2739) Fix checkbox spacing (streamlit#2738) Fix progress bar alignment (streamlit#2734) Fix select menu not autoselecting first item (streamlit#2732) Show user command line in disconnect dialog (streamlit#2735) Up version to 0.76.0 Update change log Improve usage warning wording and formatting Finally remove beta versions of set_page_config and color_picker (streamlit#2711) Finally remove beta versions of set_page_config and color_picker (streamlit#2711) Clean up file update calls (streamlit#2597)

Clean up file update calls

bb5c465

karriebear requested a review from a team January 13, 2021 17:48

tconkling suggested changes Jan 13, 2021

View reviewed changes

Add more tests

7b04db5

karriebear force-pushed the file-uploader-deploy-regression branch 2 times, most recently from 7bee202 to ca46e77 Compare January 15, 2021 22:11

karriebear added 2 commits January 19, 2021 01:15

Move updating of file count to upload file request handler

a7cb8c8

Merge remote-tracking branch 'streamlit/develop' into file-uploader-d…

7fe18bf

…eploy-regression

karriebear force-pushed the file-uploader-deploy-regression branch from ca46e77 to 7fe18bf Compare January 19, 2021 06:16

Fix tests

58dcb34

karriebear force-pushed the file-uploader-deploy-regression branch from 6772958 to 58dcb34 Compare January 19, 2021 07:13

Fix deleting

676608d

akrolsmir assigned tconkling Jan 19, 2021

tconkling suggested changes Jan 20, 2021

View reviewed changes

karriebear force-pushed the file-uploader-deploy-regression branch 2 times, most recently from dc395c2 to 7063301 Compare January 20, 2021 17:54

Address PR comments

dd8ad16

karriebear force-pushed the file-uploader-deploy-regression branch from 7063301 to dd8ad16 Compare January 20, 2021 22:49

tconkling approved these changes Jan 27, 2021

View reviewed changes

karriebear added 3 commits January 26, 2021 21:04

PR cleanup

e5a9afb

Merge branch 'develop' into file-uploader-deploy-regression

c663728

Fix test

d6539db

karriebear merged commit 8b8a1a2 into streamlit:develop Feb 2, 2021

karriebear mentioned this pull request Feb 2, 2021

File uploader has recursive behavior when closing a session #2697

Closed

Clean up file update calls #2597

Clean up file update calls #2597

Uh oh!

Conversation

karriebear commented Jan 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tconkling left a comment

Choose a reason for hiding this comment

Uh oh!

karriebear commented Jan 13, 2021

Uh oh!

tconkling left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tconkling left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asaini commented Feb 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

karriebear commented Jan 13, 2021 •

edited

Loading