Lite: Disable the analytics function forcefully in the Wasm mode #4967
Conversation
…n't make sense and it also technically doesn't work because it relies on the threading module
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
All the demos for this PR have been deployed at https://huggingface.co/spaces/gradio-pr-deploys/pr-4967-all-demos You can install the changes in this PR by running: pip install https://gradio-builds.s3.amazonaws.com/cca6239d52f8dc159cbe03276b4985884ca79203/gradio-3.39.0-py3-none-any.whl |
whitphx
changed the title
| @@ -0,0 +1,5 @@ | |||
| --- | |||
There was a problem hiding this comment.
Not sure why two changeset files were created. Maybe because the PR was renamed? But you can probably just delete both of them to reset the action
There was a problem hiding this comment.
Oh, this is a mistake. Will delete one.
| if wasm_utils.IS_WASM and self.analytics_enabled: | ||
| self.analytics_enabled = False | ||
| warnings.warn( | ||
| "Analytics are not supported in the Wasm mode." | ||
| ) |
There was a problem hiding this comment.
| if wasm_utils.IS_WASM and self.analytics_enabled: | |
| self.analytics_enabled = False | |
| warnings.warn( | |
| "Analytics are not supported in the Wasm mode." | |
| ) | |
| if wasm_utils.IS_WASM: | |
| self.analytics_enabled = False |
There was a problem hiding this comment.
That being said, it would be nice to know how much usage gradio-wasm is getting. We can get downloads from npm right?
There was a problem hiding this comment.
That may be right, but the reason I disabled the analytics now is that the analytics feature does not work on Pyodide because it uses the threading module.
We can get downloads from npm right?
Yes. Also, the CDN jsDelivr should also provides such stats, while its page is not found somehow (I don't know why... maybe because the downloads are still few? For exmaple, my @stlite/mountable has its jsDelivr page and we can see the stats).
- NPM: https://www.npmjs.com/package/@gradio/lite
- jsDeliver: (currently available now)
There was a problem hiding this comment.
So, I thought disabling the analytics is the simplest solution to make gradio-lite work,
but fixing the analytics module to be Wasm-compatible is another way if we think the analytics is important on gradio-lite too.
It would be fixing the code depending on thethreading module to depend on asyncio instead, and replacing the requests module with pyodide.http.pyfetch.
What do you think?
There was a problem hiding this comment.
but fixing the analytics module to be Wasm-compatible is another way if we think the analytics is important on gradio-lite too.
It would be fixing the code depending on thethreading module to depend on asyncio instead, and replacing the requests module with pyodide.http.pyfetch.
This would be great! I do think analytics capturing the usage would be very valuable. We don't have to add them in this PR, but would be good to have before launch
There was a problem hiding this comment.
I created another PR doing it: #5045
However let me ask one question:
While it is a higher-level discussion, should we enable the analytics feature on the Wasm ver too?
My concern is, though not 100% sure, users may expect the Wasm ver. is similar to being "standalone" and such external network access does not occur.
There was a problem hiding this comment.
My another concern is whether it is legally clear (sorry I'm not a legal expert though).
In the Wasm mode, the analytics telemetry including IP address is sent from users' devices, and it may conflict with something like GDPR (and Hugging Face has a legal entity in EU)?
There was a problem hiding this comment.
|
Sorry for the delay in responding @whitphx! I made some suggestions, but overall change lgtm |
This reverts commit a8409d3.
Description
Technically, the analytics doesn't work in the Wasm mode because it relies on the
threadingmodule that doesn't work on the Pyodide runtime.🎯 PRs Should Target Issues
Before your create a PR, please check to see if there is an existing issue for this change. If not, please create an issue before you create this PR, unless the fix is very small.
Not adhering to this guideline will result in the PR being closed.
Tests & Changelog
PRs will only be merged if tests pass on CI. To run the tests locally, please set up your Gradio environment locally and run the tests:
bash scripts/run_all_tests.shYou may need to run the linters:
bash scripts/format_backend.shandbash scripts/format_frontend.shUnless the pull request is labeled with the "no-changelog-update" label by a maintainer of the repo, all pull requests must update the changelog located in
CHANGELOG.md:Please add a brief summary of the change to the Upcoming Release section of the
CHANGELOG.mdfile and includea link to the PR (formatted in markdown) and a link to your github profile (if you like). For example, "* Added a cool new feature by
[@myusername](link-to-your-github-profile)in[PR 11111](https://github.com/gradio-app/gradio/pull/11111)".