Send ProcessCompleted message when job is cancelled #8260
Conversation
🪼 branch checks and previews
Install Gradio from this PR pip install https://gradio-builds.s3.amazonaws.com/bfa0dfd7aee551bec5cc1dcf2f0e6e22a9c57b46/gradio-4.31.0-py3-none-any.whlInstall Gradio Python Client from this PR pip install "gradio-client @ git+https://github.com/gradio-app/gradio@bfa0dfd7aee551bec5cc1dcf2f0e6e22a9c57b46#subdirectory=client/python" |
🦄 change detectedThis Pull Request includes changes to the following packages.
With the following changelog entry.
Maintainers or the PR author can modify the PR title to modify this entry.
|
freddyaboulton
added
t: fix
freddyaboulton
requested review from
aliabid94,
abidlabs,
whitphx,
aliabd,
dawoodkhan82,
pngwn and
hannahblair
| # 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 |
There was a problem hiding this comment.
Got confused by what you were saying here
| # 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 |
| @@ -738,6 +738,21 @@ async def predict( | |||
| fn_index_inferred=fn_index_inferred, | |||
| root_path=root_path, | |||
| ) | |||
| if ( # noqa: SIM102 | |||
There was a problem hiding this comment.
when I ran ruff, it didn't need this noqa statement? what does it do here?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
/resetand- 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 theis_cancel_eventcase. But if we used the/resetendpoint to handle that logic, gradio can track events directly and I think it would be much simpler and cleaner.
|
Thanks for the review @aliabid94 ! Let me merge this in and then we can refactor in subsequent PR. |
| @@ -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: | |||
There was a problem hiding this comment.
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
Description
If a job is cancelled, the server and the client will keep the
/queue/datastream 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_diffusiondemo.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.shYou may need to run the linters:
bash scripts/format_backend.shandbash scripts/format_frontend.sh