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

Only connect to heartbeat if needed #8169

Merged
merged 3 commits into from
May 3, 2024

Conversation

freddyaboulton
Copy link
Collaborator

@freddyaboulton freddyaboulton commented Apr 29, 2024

Description

Closes: #8022

Rather than always connecting to the heartbeat route, we should do so only when gr.State variables are used (to clean up state) or when the unload event is set. Users may still run into the browser limit, however.

EDIT: I originally wanted to raise an error or message when the page locked up due to the connection limit. But the browser will block all requests after the limit is reached so we won't be able to fetch the config and progressively load the app.

🎯 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

  1. 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.sh

  2. You may need to run the linters: bash scripts/format_backend.sh and bash scripts/format_frontend.sh

@gradio-pr-bot
Copy link
Contributor

gradio-pr-bot commented Apr 29, 2024

🪼 branch checks and previews

Name Status URL
Spaces ready! Spaces preview
Website ready! Website preview
🦄 Changes detected! Details

Install Gradio from this PR

pip install https://gradio-builds.s3.amazonaws.com/97cfc159447ad8eb3b29f03aae8fa4484606e395/gradio-4.28.3-py3-none-any.whl

Install Gradio Python Client from this PR

pip install "gradio-client @ git+https://github.com/gradio-app/gradio@97cfc159447ad8eb3b29f03aae8fa4484606e395#subdirectory=client/python"

@gradio-pr-bot
Copy link
Contributor

gradio-pr-bot commented Apr 29, 2024

🦄 change detected

This Pull Request includes changes to the following packages.

Package Version
@gradio/client patch
gradio patch
  • Maintainers can select this checkbox to manually select packages to update.

With the following changelog entry.

Only connect to heartbeat if needed

Maintainers or the PR author can modify the PR title to modify this entry.

Something isn't right?

  • Maintainers can change the version label to modify the version bump.
  • If the bot has failed to detect any changes, or if this pull request needs to update multiple packages to different versions or requires a more comprehensive changelog entry, maintainers can update the changelog file directly.

@freddyaboulton freddyaboulton force-pushed the 8022-connect-heartbeat-when-needed branch from a5b7238 to 4793b18 Compare May 1, 2024 17:46
@freddyaboulton freddyaboulton marked this pull request as ready for review May 1, 2024 17:46
@abidlabs
Copy link
Member

abidlabs commented May 2, 2024

Will this be compatible with gr.render() -- now that we can dynamically create components, a developer might include a workflow that dynamically creates gr.State() components?

The only other idea I can think of is to use long polling instead of a persistent SSE connection for the heartbeat

@freddyaboulton
Copy link
Collaborator Author

One of the reasons we did not use long-polling was that browsers can slow down timers so it could not be reliable. We're sending an updated config when the components are added in gr.render, I think we can recalculate the connect_heartbeat flag on every re-render.

@abidlabs abidlabs requested a review from aliabid94 May 3, 2024 07:42
Copy link
Member

@pngwn pngwn left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks @freddyaboulton!

Note: this should working fine with the render decorator because we refresh the config, as freddy mentioned.

Copy link
Collaborator

@aliabid94 aliabid94 left a comment

Choose a reason for hiding this comment

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

LGTM!

@freddyaboulton
Copy link
Collaborator Author

Thanks for the reviews all!

@freddyaboulton freddyaboulton merged commit 3a6f1a5 into main May 3, 2024
7 checks passed
@freddyaboulton freddyaboulton deleted the 8022-connect-heartbeat-when-needed branch May 3, 2024 17:36
@pngwn pngwn mentioned this pull request May 3, 2024
dawoodkhan82 pushed a commit that referenced this pull request May 6, 2024
* Add connect_heartbeat field

* fix types

* add changeset

---------

Co-authored-by: gradio-pr-bot <gradio-pr-bot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gradio Unable to Load More than 5 iFrames After Package Update to 4.25 Version
5 participants