Skip to content
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

Refactoring: Remove an unnecessary form field sessionId from the file upload client #7980

Merged

Conversation

whitphx
Copy link
Contributor

@whitphx whitphx commented Jan 19, 2024

Describe your changes

This sessionId field is no longer used in the server-side UploadFileRequestHandler.put(), which actually looks at the path params instead:

def put(self, **kwargs):
"""Receive an uploaded file and add it to our UploadedFileManager."""
args: Dict[str, List[bytes]] = {}
files: Dict[str, List[Any]] = {}
session_id = self.path_kwargs["session_id"]
file_id = self.path_kwargs["file_id"]

Context: whitphx#7 (comment)

GitHub Issue Link (if applicable)

Testing Plan

  • Explanation of why no additional tests are needed
  • Unit Tests (JS and/or Python)
  • E2E Tests
  • Any manual testing needed?

Contribution License Agreement

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

@whitphx
Copy link
Contributor Author

whitphx commented Jan 19, 2024

Also, the sessionId argument of uploadFileUploaderFile is no longer used if this PR is merged.


Can it also be removed, or may not if it's used in another proprietary implementation of the StreamlitEndpoints interface?

(The sessionId info is already embedded in the fileUploadUrl beforehand here:

sessionId: this.sessionInfo.current.sessionId,
)

@LukasMasuch
Copy link
Collaborator

@kajarenc I believe we can remove this, or?

@kajarenc
Copy link
Collaborator

Hey, @whitphx thanks for opening this PR!

let me think about this a little bit, I will be back next week!

@LukasMasuch
Copy link
Collaborator

@whitphx the test here needs to be adapted for the CI to run through:

expectedData.append("sessionId", "mockSessionId")

@whitphx
Copy link
Contributor Author

whitphx commented Jan 20, 2024

@LukasMasuch Oops, missed it.
I pushed the fix now.

but the E2E test failed... 🤔

@LukasMasuch
Copy link
Collaborator

LukasMasuch commented Jan 20, 2024

@whitphx This is not on you :) The newest pandas release (2.2.0) is causing some issues in our CI pipeline.

@LukasMasuch
Copy link
Collaborator

@whitphx Just needs another merge with develop to get the CI to succeed.

@whitphx
Copy link
Contributor Author

whitphx commented Jan 23, 2024

@LukasMasuch Merged it 👍

@kajarenc
Copy link
Collaborator

LGTM 👍 Than you @whitphx!

FOR uploadFileUploaderFile function signature, let's keep session_id there, we could come back to this, but for now, I wouldn’t want to change the interface of StreamlitEndpoints.

@kajarenc kajarenc merged commit df28f11 into streamlit:develop Jan 23, 2024
39 checks passed
@whitphx
Copy link
Contributor Author

whitphx commented Jan 23, 2024

@kajarenc Thanks!

zyxue pushed a commit to zyxue/streamlit that referenced this pull request Apr 16, 2024
…le upload client (streamlit#7980)

This sessionId field is no longer used in the server-side UploadFileRequestHandler.put(), which actually looks at the path params instead

* Remove an unnecessary form field `sessionId` from the file upload client

* Fix DefaultStreamlitEndpoints.test.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants