-
Notifications
You must be signed in to change notification settings - Fork 757
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
feat: add timeout option for bentoml runner config #2890
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2890 +/- ##
==========================================
- Coverage 70.77% 69.88% -0.89%
==========================================
Files 104 121 +17
Lines 9470 9961 +491
==========================================
+ Hits 6702 6961 +259
- Misses 2768 3000 +232
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick q: have you run this test on lower version of bentoml for compat layer?
I have not actually but can you tell me more about this compat layer? |
Oh just that running this on an older version of bentoml. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
conflicts
0bde657
to
94a8665
Compare
@property | ||
def runner_timeout(self) -> int: | ||
"return the configured timeout for this runner." | ||
runner_cfg = BentoMLContainer.runners_config.get() | ||
if self._runner.name in runner_cfg: | ||
return runner_cfg[self._runner.name]["timeout"] | ||
else: | ||
return runner_cfg["timeout"] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bojiang This is how we get the timeout for each runner client
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a case that the runner_name
exists in the config but didn't provide a timeout
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope, during initialization BentoMLConfiguration will copy over the default config (including timeout, resources, batching etc) to the runner_name
so that situation will not happen.
What does this PR address?
Fixes #2888
eg
by default all runner have a timeout of 300 seconds
Before submitting:
guide on how to create a pull request.
make format
andmake lint
script have passed (instructions)?those accordingly? Here are documentation guidelines and tips on writting docs.
Who can help review?
Feel free to tag members/contributors who can help review your PR.