fix: Eliminate the exception during shutdown

[frostming]

Signed-off-by: Frost Ming [email protected]

What does this PR address?

Fixes #3813

A bug that causes the runner to keep running after it is terminated. This is because the CancelledError is swallowed silently, making the tasks never be cancelled by the runner.

BTW, since CancelledError is subclassing BaseException it won't be caught by the except Exception block. We can remove it.

Before submitting:

• Does the Pull Request follow Conventional Commits specification naming? Here are GitHub's
  guide on how to create a pull request.
• Does the code follow BentoML's code style, both make format and make lint script have passed (instructions)?
• Did you read through contribution guidelines and follow development guidelines?
• Did your changes require updates to the documentation? Have you updated
  those accordingly? Here are documentation guidelines and tips on writting docs.
• Did you write tests to cover your changes?

[sauyon]

Just tried this and if you kill the server in training you get

  File "/home/sauyon/devel/bentoml/src/bentoml/_internal/marshal/dispatcher.py", line 206, in train_optimizer
    await self._wake_event.wait_for(self._queue.__len__)
  File "/usr/lib/python3.7/asyncio/locks.py", line 381, in wait_for
    await self.wait()
  File "/usr/lib/python3.7/asyncio/locks.py", line 354, in wait
    await fut
concurrent.futures._base.CancelledError
 (trace=ae8eb131d85f182410e65752fac63ced,span=6b02be23ca37ea62,sampled=0)
Traceback (most recent call last):
  File "/home/sauyon/devel/bentoml/src/bentoml/_internal/marshal/dispatcher.py", line 206, in train_optimizer
    await self._wake_event.wait_for(self._queue.__len__)
  File "/usr/lib/python3.7/asyncio/locks.py", line 381, in wait_for
    await self.wait()
  File "/usr/lib/python3.7/asyncio/locks.py", line 354, in wait
    await fut
concurrent.futures._base.CancelledError
2023-05-03T23:05:35-0700 [ERROR] [runner:iris_clf:1] Traceback (most recent call last):
  File "/home/sauyon/devel/bentoml/src/bentoml/_internal/marshal/dispatcher.py", line 206, in train_optimizer
    await self._wake_event.wait_for(self._queue.__len__)
  File "/usr/lib/python3.7/asyncio/locks.py", line 381, in wait_for
    await self.wait()
  File "/usr/lib/python3.7/asyncio/locks.py", line 354, in wait
    await fut
concurrent.futures._base.CancelledError
 (trace=ae8eb131d85f182410e65752fac63ced,span=6b02be23ca37ea62,sampled=0)
Traceback (most recent call last):
  File "/home/sauyon/devel/bentoml/src/bentoml/_internal/marshal/dispatcher.py", line 206, in train_optimizer
    await self._wake_event.wait_for(self._queue.__len__)
  File "/usr/lib/python3.7/asyncio/locks.py", line 381, in wait_for
    await self.wait()
  File "/usr/lib/python3.7/asyncio/locks.py", line 354, in wait
    await fut
concurrent.futures._base.CancelledError

instead, which doesn't seem much better?

[frostming]

Just tried this and if you kill the server in training you get

@sauyon Thanks for that info. I tested on Python 3.7(which I didn't do previously), it looks good after I reraise the exception.

[sauyon]

Looks better to me! Should we be worried about the return on L316 and on L180?

[frostming]

Looks better to me! Should we be worried about the return on L316 and on L180?

L180 is intentional to return a fallback value. L316 doesn't make a difference since it is a top async task on its own, but IMO it has a slight mismatch in semantic: a cancellation should not be a normal return. I will change it.

[sauyon]

Ok, this LGTM! Not sure why I didn't reraise in the training code and added the raise elsewhere...

[frostming]

Ok, this LGTM! Not sure why I didn't reraise in the training code and added the raise elsewhere...

NVM, it may come from a refactor when the logic is extracted into a function

[sauyon]

@aarnphm are unit tests broken?

[aarnphm]

Oh we need to install Pillow in our tests. Let me make a quick PR for that