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

microbatch: set semaphore based on workers #569

Merged
merged 7 commits into from Mar 23, 2020
Merged

Conversation

@hrmthw
Copy link
Collaborator

hrmthw commented Mar 19, 2020

(Thanks for sending a pull request! Please make sure to read the contribution guidelines, then fill out the blanks below.)

What changes were proposed in this pull request?

Does this close any currently open issues?

How was this patch tested?

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Mar 19, 2020

Codecov Report

Merging #569 into master will increase coverage by 0.04%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #569      +/-   ##
==========================================
+ Coverage   56.51%   56.56%   +0.04%     
==========================================
  Files          98       98              
  Lines        7387     7402      +15     
==========================================
+ Hits         4175     4187      +12     
- Misses       3212     3215       +3
Impacted Files Coverage Δ
bentoml/server/gunicorn_config.py 0% <ø> (ø) ⬆️
bentoml/server/gunicorn_server.py 46.34% <0%> (ø) ⬆️
bentoml/server/marshal_server.py 39.47% <0%> (-1.07%) ⬇️
bentoml/handlers/dataframe_handler.py 82.63% <0%> (ø) ⬆️
bentoml/cli/__init__.py 71.76% <100%> (+0.5%) ⬆️
bentoml/marshal/marshal.py 29.68% <20%> (-0.65%) ⬇️
bentoml/marshal/dispatcher.py 23.63% <50%> (-0.51%) ⬇️
bentoml/utils/__init__.py 63.52% <50%> (-1.81%) ⬇️
bentoml/server/utils.py 51.72% <0%> (+13.79%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 124fae4...a3b1a0c. Read the comment docs.

@@ -346,6 +346,7 @@ def serve_gunicorn(
port=port,
num_of_workers=microbatch_workers,
prometheus_lock=prometheus_lock,
model_server_workers=workers or 3,

This comment has been minimized.

Copy link
@parano

parano Mar 19, 2020

Member

3 is actually not the default value, I think it should use this function instead to get the default workers value:
https://github.com/bentoml/BentoML/blob/master/bentoml/server/utils.py#L29
https://github.com/bentoml/BentoML/blob/master/bentoml/server/gunicorn_config.py#L7

This comment has been minimized.

Copy link
@hrmthw

hrmthw Mar 20, 2020

Author Collaborator

@parano updated

self.target_host = target_host
self.target_port = target_port
self.target_count = target_count

This comment has been minimized.

Copy link
@parano

parano Mar 20, 2020

Member

add some inline comments about self.target_count and self._target_sema?

'''
return asyncio.Semaphore(self.task_concurrency)
if self._sema is None:
self._sema = self.shared_sema and self.shared_sema() or asyncio.Semaphore(1)

This comment has been minimized.

Copy link
@parano

parano Mar 20, 2020

Member

consider this for readability?

self._sema = self.shared_sema() if self.shared_sema else asyncio.Semaphore(1)

This comment has been minimized.

Copy link
@hrmthw

hrmthw Mar 20, 2020

Author Collaborator

Yeah, the later is always better than the former.

@@ -346,6 +346,7 @@ def serve_gunicorn(
port=port,
num_of_workers=microbatch_workers,
prometheus_lock=prometheus_lock,
model_server_workers=workers,

This comment has been minimized.

Copy link
@parano

parano Mar 20, 2020

Member

is it possible to start GunicornBentoServer before GunicornMarshalServer? that way you should be able to get the actual worker count via gunicorn_app.cfg.workers

This comment has been minimized.

Copy link
@hrmthw

hrmthw Mar 20, 2020

Author Collaborator

gunicorn_app.run will block itself.

This comment has been minimized.

Copy link
@parano

parano Mar 20, 2020

Member

Ohh got it

This comment has been minimized.

Copy link
@parano

parano Mar 20, 2020

Member

I think a slightly better way to do this is:

model_server_workder_count = workers or get_gunicorn_num_of_workers()

and then pass it to both GunicornBentoServer and GunicornMarshalServer

That also fixes a current issue, for example when user set the CLI arg --workers=5, it still prints something like "get_gunicorn_num_of_workers: 3, calculated by cpu count", which is wrong and very confusing

@hrmthw hrmthw force-pushed the hrmthw:release branch from e6191a8 to c906c7f Mar 23, 2020
prometheus_lock=prometheus_lock,
outbound_host="localhost",

This comment has been minimized.

Copy link
@parano
from bentoml.utils.usage_stats import track_server_stop

workers = get_gunicorn_num_of_workers()

This comment has been minimized.

Copy link
@parano
@parano

This comment has been minimized.

Copy link
Member

parano commented Mar 23, 2020

LGTM, thanks for the update @hrmthw, merging now!

@parano parano merged commit ec9d8c6 into bentoml:master Mar 23, 2020
2 of 3 checks passed
2 of 3 checks passed
codecov/patch 33.33% of diff hit (target 56.51%)
Details
codecov/project 56.56% (+0.04%) compared to 124fae4
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.