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.

Sign up for GitHub

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

Jump to bottom

Lite: Disable the analytics function forcefully in the Wasm mode #4967

Closed
wants to merge 6 commits into from
+18 −1
Closed

Lite: Disable the analytics function forcefully in the Wasm mode #4967

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
18f737d
Disable the analytics function forcefully in the Wasm mode as it does…
whitphx Jul 19, 2023
02e08f4
Add changeset
whitphx Jul 19, 2023
d050098
Merge branch 'main' into wasm-disable-analytics
whitphx Jul 20, 2023
8d84676
Merge branch 'main' into wasm-disable-analytics
whitphx Jul 25, 2023
a8409d3
Add changeset
whitphx Jul 25, 2023
d4e0e4a
Revert "Add changeset"
whitphx Jul 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/chilly-windows-sleep.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, this is a mistake. Will delete one.

"@gradio/lite": patch
---

Disable the analytics function that doesn't work in the Wasm mode
7 changes: 6 additions & 1 deletion gradio/blocks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -720,6 +720,11 @@ def __init__(
if analytics_enabled is not None
else analytics.analytics_enabled()
)
if wasm_utils.IS_WASM and self.analytics_enabled:
self.analytics_enabled = False
warnings.warn(
abidlabs marked this conversation as resolved.
Show resolved Hide resolved
"Analytics are not supported in the Wasm mode."
)
Comment on lines +723 to +727
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That being said, it would be nice to know how much usage gradio-wasm is getting. We can get downloads from npm right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

@whitphx whitphx Jul 28, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@whitphx whitphx Jul 31, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question @whitphx I think its important to have analytics to get an idea to have usage of the wasm mode. We have an existing issue to make sure analytics is compliant with GDPR (#4226) but for now, any user can disable analytics pretty easily so I think it should be fine.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, thanks!

if self.analytics_enabled:
t = threading.Thread(target=analytics.version_check)
t.start()
Expand Down Expand Up @@ -761,7 +766,7 @@ def __init__(
self.root_path = ""
self.root_urls = set()

if not wasm_utils.IS_WASM and self.analytics_enabled:
if self.analytics_enabled:
is_custom_theme = not any(
self.theme.to_dict() == built_in_theme.to_dict()
for built_in_theme in BUILT_IN_THEMES.values()
Expand Down
7 changes: 7 additions & 0 deletions js/wasm/src/webworker/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,13 @@ matplotlib.use("agg")
await pyodide.runPythonAsync(unloadModulesPySource);
unload_local_modules = pyodide.globals.get("unload_local_modules");
console.debug("Python utility functions are set up.");

console.debug("Set environment variables.");
await pyodide.runPythonAsync(`
import os
os.environ["GRADIO_ANALYTICS_ENABLED"] = "False"
`);
console.debug("Environment variables are set.");
abidlabs marked this conversation as resolved.
Show resolved Hide resolved
}

self.onmessage = async (event: MessageEvent<InMessage>): Promise<void> => {
Expand Down