Send ProcessCompleted message when job is cancelled #8260

freddyaboulton · 2024-05-10T19:42:15Z

Description

If a job is cancelled, the server and the client will keep the /queue/data stream open forever.

Notice that in the first image, the multiple heartbeats gets sent after the job is cancelled (indicating the channel was kept open for at least 45 seconds post cancellation) while in the second message the process_completed and close_stream messages are sent after cancellation. This was tested on the fake_diffusion demo.

Main

This PR

🎯 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

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
You may need to run the linters: bash scripts/format_backend.sh and bash scripts/format_frontend.sh

gradio-pr-bot · 2024-05-10T19:42:54Z

🪼 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/bfa0dfd7aee551bec5cc1dcf2f0e6e22a9c57b46/gradio-4.31.0-py3-none-any.whl

Install Gradio Python Client from this PR

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

gradio-pr-bot · 2024-05-10T19:43:05Z

🦄 change detected

This Pull Request includes changes to the following packages.

Package	Version
`gradio`	`patch`

Maintainers can select this checkbox to manually select packages to update.

With the following changelog entry.

Send ProcessCompleted message when job is cancelled

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.

aliabid94 · 2024-05-13T19:36:51Z

gradio/blocks.py

+        # If the function is a cancel function, 'predictions' are the ids of
+        # the event in the queue that has been cancelled. We need these
+        # so that the server can put the ProcessCompleted message in the event stream
+        # Cancel events have no output components, so we need to return early otherise the output


Got confused by what you were saying here

Suggested change

# Cancel events have no output components, so we need to return early otherise the output

# Cancel events have no output components, so we need to return early to return the ids

aliabid94 · 2024-05-13T19:42:46Z

gradio/routes.py

@@ -738,6 +738,21 @@ async def predict(
                    fn_index_inferred=fn_index_inferred,
                    root_path=root_path,
                )
+                if (  # noqa: SIM102


when I ran ruff, it didn't need this noqa statement? what does it do here?

It raised an error because the two if statements can be combined into 1 but I think that would make the code harder to read.

aliabid94

Reviewed and tested and LGTM, thanks freddy! I hadn't really explored the cancel logic in the backend before, and found the design a bit confusing. When we cancel, the frontend makes a call to both

/reset and
as a regular event listener whose function is defined by get_cancel_function.
Unless I'm missing something, I think we should remove 2 entirely and handle all the logic needed via 1. Not only does it save a request, but some of the logic is done in a weird way in 1 because it's not being handled by gradio itself but some pre-defined logic. For example, using task names to store and track data for cancelling is weird, but necessary right now. We also have to hack postprocessing specifically for the is_cancel_event case. But if we used the /reset endpoint to handle that logic, gradio can track events directly and I think it would be much simpler and cleaner.

freddyaboulton · 2024-05-13T21:43:18Z

Thanks for the review @aliabid94 ! Let me merge this in and then we can refactor in subsequent PR.

aliabid94 · 2024-05-13T22:23:12Z

gradio/blocks.py

@@ -1656,10 +1669,18 @@ def validate_outputs(self, fn_index: int, predictions: Any | list[Any]):

    async def postprocess_data(
        self, fn_index: int, predictions: list | dict, state: SessionState | None
-    ):
+    ) -> Any:


I see why you set it to Any to avoid type hinting entirely with the list | dict, but would be great to fix in a refactor soon

close on cancel

1824724

add changeset

c85fd92

freddyaboulton added t: fix A change that implements a fix v: patch A change that requires a patch release labels May 10, 2024

gradio-pr-bot and others added 2 commits May 10, 2024 19:44

add changeset

b54f7e4

Add code

bfa0dfd

freddyaboulton marked this pull request as ready for review May 10, 2024 21:57

freddyaboulton requested review from aliabid94, abidlabs, whitphx, aliabd, dawoodkhan82, pngwn and hannahblair May 10, 2024 21:57

aliabid94 reviewed May 13, 2024

View reviewed changes

aliabid94 approved these changes May 13, 2024

View reviewed changes

freddyaboulton merged commit 7e976fd into main May 13, 2024
7 checks passed

freddyaboulton deleted the close-stream-on-cancel branch May 13, 2024 21:43

pngwn mentioned this pull request May 13, 2024

chore: update versions #8274

Merged

freddyaboulton mentioned this pull request May 13, 2024

Refactor backend cancelling logic #8280

Open

1 task

aliabid94 reviewed May 13, 2024

View reviewed changes

freddyaboulton mentioned this pull request May 23, 2024

Refactor Cancelling Logic To Use /cancel #8370

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Send ProcessCompleted message when job is cancelled #8260

Send ProcessCompleted message when job is cancelled #8260

freddyaboulton commented May 10, 2024 •

edited

Loading

gradio-pr-bot commented May 10, 2024 •

edited

Loading

gradio-pr-bot commented May 10, 2024 •

edited

Loading

Something isn't right?

aliabid94 May 13, 2024 •

edited

Loading

aliabid94 May 13, 2024

freddyaboulton May 13, 2024

aliabid94 left a comment •

edited

Loading

freddyaboulton commented May 13, 2024

aliabid94 May 13, 2024

	# Cancel events have no output components, so we need to return early otherise the output
	# Cancel events have no output components, so we need to return early to return the ids

Send ProcessCompleted message when job is cancelled #8260

Send ProcessCompleted message when job is cancelled #8260

Conversation

freddyaboulton commented May 10, 2024 • edited Loading

Description

Main

This PR

🎯 PRs Should Target Issues

Tests

gradio-pr-bot commented May 10, 2024 • edited Loading

🪼 branch checks and previews

gradio-pr-bot commented May 10, 2024 • edited Loading

🦄 change detected

This Pull Request includes changes to the following packages.

With the following changelog entry.

Something isn't right?

aliabid94 May 13, 2024 • edited Loading

Choose a reason for hiding this comment

aliabid94 May 13, 2024

Choose a reason for hiding this comment

freddyaboulton May 13, 2024

Choose a reason for hiding this comment

aliabid94 left a comment • edited Loading

Choose a reason for hiding this comment

freddyaboulton commented May 13, 2024

aliabid94 May 13, 2024

Choose a reason for hiding this comment

freddyaboulton commented May 10, 2024 •

edited

Loading

gradio-pr-bot commented May 10, 2024 •

edited

Loading

gradio-pr-bot commented May 10, 2024 •

edited

Loading

aliabid94 May 13, 2024 •

edited

Loading

aliabid94 left a comment •

edited

Loading