-
-
Notifications
You must be signed in to change notification settings - Fork 63
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
Add stlite modifications #672
Add stlite modifications #672
Conversation
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.
Thank you for the PR!
Let me comment as below at this moment, while I'm gonna check the code more deeply soon.
@@ -281,7 +280,6 @@ async function loadPyodideAndPackages() { | |||
// The following Python code is based on streamlit.web.cli.main_run(). | |||
self.__streamlitFlagOptions__ = { | |||
...streamlitConfig, | |||
"browser.gatherUsageStats": false, |
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.
Do you think this feature should be enabled?
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.
Good question. From our data side, we are able to handle and distinguish telemetry coming from stlite apps. And in the long-term if stlite gets more integrated as an official Streamlit aspect, we probably want to enable it for all stlite apps. But for this update, I think its fine to leave it disabled as default but in a way that it can be activated if it is explicitly set to true
.
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.
See the update code
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.
Thanks!
widget_id = self._require_arg(args, "widgetId") | ||
path_args = re.match(UPLOAD_FILE_ROUTE, request.path) | ||
session_id = path_args.group('session_id') | ||
file_id = path_args.group('file_id') |
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.
TODO: Check if session_id
and file_id
can be obtained from kwargs
of this handler?
(Now I'm outside and can't confirm it, but think maybe it's possible)
packages/kernel/py/stlite-server/stlite_server/upload_file_request_handler.py
Show resolved
Hide resolved
@@ -1,5 +1,4 @@ | |||
export * from "./kernel"; | |||
export * from "./streamlit-replacements/lib/ConnectionManager"; | |||
export * from "./streamlit-replacements/lib/FileUploadClient"; |
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.
Memo: The patched FileUploadClient
has been moved to whitphx/streamlit
in whitphx/streamlit#7
Co-authored-by: Yuichiro Tachibana (Tsuchiya) <[email protected]>
Co-authored-by: Yuichiro Tachibana (Tsuchiya) <[email protected]>
…h/stlite into stlite-1.27-additions
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.
Thanks, and it LGTM!
I'm gonna merge this PR, and let me add some modifications in #671 after merging.
@@ -51,6 +51,7 @@ export interface WorkerInitialData { | |||
}; | |||
mountedSitePackagesSnapshotFilePath?: string; | |||
streamlitConfig?: StreamlitConfig; | |||
disableProgressToasts?: boolean; |
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.
This is not needed here, as the worker doesn't refer to this option?
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.
Oh, yes that makes sense 👍
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.
Thanks, I updated it as a part of changes in #671 after merging this PR. Plz track it and leave any comment 👍
* Copy .nvmrc from [email protected] * Update the streamlit submodule, fix some package resolutions for it, and fix @stlite/kernel for streamlit 1.27.0 * Update @stlite/common-react and mountable * Set webpack version in @stlite/mountable as same as [email protected] * Set the resolved webpack version to avoid an error "The 'compilation' argument must be an instance of Compilation", which is caused by multiple versions of webpack. Ref: https://qiita.com/Hanpen3019/items/e6ae5d29b334bc3d7af3 * Update @stlire/sharing * Update @stlite/desktop * Update Makefile * Remove blob-polyfill * Update Makefile * Update the streamlit submodule * Fix .github/workflows/main.yml * Add streamlit/frontend as a workspace * Add a make rule for the streamlit frontend lib * Update wheel versions * Update the streamlit submodule * Set the immutable's version as same as the locked version of the upstream Streamlit frontend to avoid type errors * Add stlite modifications (#672) * Add stlite modifications * Add back connection manager * Move methods * Some updates * Remove header * More cleanup * More changes * More cleanup * More modifications * Don't update pyiodide * More cleanup * Don't Update pyodide * Don't update pyiodide * More cleanup * More cleanup * Rename of option * Remove new line * Minor cleanup * Fix wrong option name * Minor renaming * Fix typo Co-authored-by: Yuichiro Tachibana (Tsuchiya) <[email protected]> * Update comment Co-authored-by: Yuichiro Tachibana (Tsuchiya) <[email protected]> * Remove paths * Change to put * Update URL * Add gatherUsageStats flag --------- Co-authored-by: Yuichiro Tachibana (Tsuchiya) <[email protected]> * Update the streamlit submodule * Add the PUT type to the HttpRequest interface * Fix the imports in ConnectionManager.ts * Fix upload_file_request_handler.py to refer to `kwargs` of the handler method * Fix stlite_server/upload_file_request_handler.py to catch up with the upstream implementation * Remove `form-data-encoder` from `@stlite/kernel` as it's moved to `@streamlit/lib` in whitphx/streamlit#7 * Fix the mountable option parser to distinguish the toast callback options from the kernel options * Remove ctx.disableToast() and ctx.enableToast() which were introduced in #636 because the `disableProgressToasts` and `disableErrorToasts` options play the same role * Update CHANGELOG.md * Apply Prettier to kernel * Fix stlite-server tests * Fix the CI workflows * Fix format and types in stlite-server * Update main.yml fixing the build jobs to initialize with venv * Add the streamlit lib package to the deps of the kernel make rule * Remove the TS dependency from the kernel module to the build artifact of `@streamlit/lib` * Fix main.yml * Fix .github/workflows/gh-pages.yml * Update the streamlit submodule * Fix the streamlit-proto make rule to call the python-init-dev-only rule * Fix the venv make rule to execute it every time * Update DEVELOPMENT.md * Fix desktop/bin/dump_artifacts.ts not to pin the altair version * Fix CI files * Fix tsconfig * Refresh package.json and yarn.lock * Fix stlite_server/server.py making it look more similar to the original server.py * Fix dump_artifacts.ts * Add a test for the file delete endpoint --------- Co-authored-by: Lukas Masuch <[email protected]>
Related pull request with Streamlit modifications: whitphx/streamlit#7