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

[Refactor] fork server processes earlier #1476

Merged
merged 10 commits into from
Mar 10, 2021
Merged

Conversation

bojiang
Copy link
Member

@bojiang bojiang commented Mar 1, 2021

Description

requires test utils in #1467 #1347

Fork before importing third-party dependencies to avoid hacking like #925

Should introduce a lib of process management in the future. Now, this function was provided by Gunicorn.

Motivation and Context

How Has This Been Tested?

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature and improvements (non-breaking change which adds/improves functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Code Refactoring (internal change which is not user facing)
  • Documentation
  • Test, CI, or build

Component(s) if applicable

  • BentoService (service definition, dependency management, API input/output adapters)
  • Model Artifact (model serialization, multi-framework support)
  • Model Server (mico-batching, dockerisation, logging, OpenAPI, instruments)
  • YataiService gRPC server (model registry, cloud deployment automation)
  • YataiService web server (nodejs HTTP server and web UI)
  • Internal (BentoML's own configuration, logging, utility, exception handling)
  • BentoML CLI

Checklist:

  • My code follows the bentoml code style, both ./dev/format.sh and
    ./dev/lint.sh script have passed
    (instructions).
  • My change reduces project test coverage and requires unit tests to be added
  • I have added unit tests covering my code change
  • My change requires a change to the documentation
  • I have updated the documentation accordingly

@bojiang bojiang changed the title [Refactor] earlier fork server processes [Refactor] fork server processes earlier Mar 1, 2021
@codecov
Copy link

codecov bot commented Mar 1, 2021

Codecov Report

Merging #1476 (e214050) into master (b040add) will increase coverage by 0.97%.
The diff coverage is 88.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1476      +/-   ##
==========================================
+ Coverage   68.65%   69.62%   +0.97%     
==========================================
  Files         150      150              
  Lines       10045    10072      +27     
==========================================
+ Hits         6896     7013     +117     
+ Misses       3149     3059      -90     
Impacted Files Coverage Δ
bentoml/marshal/marshal.py 74.25% <70.00%> (+48.42%) ⬆️
bentoml/server/gunicorn_server.py 87.50% <90.00%> (-0.88%) ⬇️
bentoml/server/__init__.py 69.33% <91.11%> (+19.33%) ⬆️
bentoml/cli/bento_service.py 80.85% <100.00%> (-1.62%) ⬇️
bentoml/configuration/containers.py 86.48% <100.00%> (ø)
bentoml/server/api_server.py 71.21% <100.00%> (-0.22%) ⬇️
bentoml/server/marshal_server.py 97.61% <100.00%> (+4.28%) ⬆️
bentoml/utils/log.py 95.74% <100.00%> (-0.18%) ⬇️
... and 2 more

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 b040add...e214050. Read the comment docs.

@bojiang bojiang added the pr/merge-hold Requires further discussions before a pull request can be merged label Mar 1, 2021
@bojiang
Copy link
Member Author

bojiang commented Mar 3, 2021

Updated
Fixed: DI error on MacOS.

@bojiang
Copy link
Member Author

bojiang commented Mar 4, 2021

req: #1483

@bojiang bojiang removed the pr/merge-hold Requires further discussions before a pull request can be merged label Mar 4, 2021
@bojiang bojiang requested review from ssheng and removed request for ssheng March 5, 2021 02:23
@bojiang
Copy link
Member Author

bojiang commented Mar 5, 2021

cc @ssheng

bentoml/configuration/containers.py Outdated Show resolved Hide resolved
@@ -0,0 +1,133 @@
# Copyright 2019 Atalaya Tech, Inc.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you envision will be added under the /entrypoint package? What do we gain by moving to a new package?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The place before is /bentoml/server/__init__.py. I don't think it's a proper place to wire packages including bentoml.server itself.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be fine to wire one's own package. Taking a step back, we should think about how we'd like to expose our public APIs. Having a module like BentoMLController is one choice. What are some of the best practice in Python?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bojiang @ssheng I think what @bojiang suggested last time, having an /api/ module for exposing public APIs is probably the convention among python-based DS/ML tools:

/api
├── server.py
├── __init__.py
├── yatai.py
├── bundle.py

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed offline, we will keep these in the original location under server/__init__.py.

bentoml/entrypoint/__init__.py Outdated Show resolved Hide resolved
def _start_prod_server(
saved_bundle_path: str,
config: BentoMLConfiguration,
port: Optional[int] = None,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to explicitly have port here instead of from config?

Copy link
Member Author

@bojiang bojiang Mar 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not want to change the value of config.api_server.port. Seeing the batching app and the model app as the whole API server, it makes sense to keep config.api_server rather than overriding it by the randomly picked port.

Copy link
Collaborator

@ssheng ssheng Mar 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the concern. Here is why we should restructure the config keys and terminology. Before we do that, I think there is an opportunity here to make the intermediate port injectable as well. First we can introduce an intermediate port (for lack of a better name).

api_server:
    marshal:
        intermediate_port: Null # or an actual port e.g. 6000

In the container, we can introduce a provider that first checks if there is an intermediate port defined, if not randomly reserve a port.

intermediate_port = providers.Callable(
    lambda port: port if port else reserve_free_port(),
    config.marshal.intermediate_port,
)

Basically, a lot of the logic here can be refactored as a provider in containers.py.

Copy link
Member Author

@bojiang bojiang Mar 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's great to have a provider for the intermediate_port. But we have to wire after creating the new process. In your solution, the reserve_free_port would be called twice and gave intermediate_port different values in each process.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call that reserve_free_port might be called twice. But it should be a solvable problem. We can use a singleton provider for creating the intermediate port. All users of the provider will get the same port. We will need to move the container creation out, however. Happy to discuss with you over a Zoom call.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UDS is superior but we might have to leave the TCP option open to make remote marshaling a possibility. We can structure the config meaningfully to reflect this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to leave the TCP option open to make remote marshaling a possibility. We can structure the config meaningfully to reflect this.

api_server:
  port: 5000
  enable_microbatch: False
  run_with_ngrok: False
  enable_swagger: True
  enable_metrics: True
  enable_feedback: True
  max_request_size: 20971520
  workers: 1
  timeout: 60

model_server:
  port: Null  # default: api_server.port when enable_marshal=False

marshal_server:
  port: Null  # default: api_server.port when enable_marshal=True


like this?

Copy link
Collaborator

@ssheng ssheng Mar 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking something like the following where the user can choose the connector type of provide related configs. Basically the schema for connector is a union of UDS and TCP connector schemas.

If UDS is chosen,

api_server:
  marshal:
    connector:
      type: UDS
      uds_related_key1: value1
      uds_related_key2: value2

Or, if TCP is chosen,

api_server:
  marshal:
    connector:
      type: TCP
      port: None # or some configured port
      tcp_related_key1: value1

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For UDS support, I'd like to have a simple host field in URI format, just like gunicorn and Nginx does.

  • 127.0.0.1:5000
  • unix:/tmp/gunicorn.sock

Your schema is fancy but looks too powerful for me.
I think you can draft a new PR to demonstrate your idea.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After all, the config structure reflects our system design. We can continue the discussion in the new PR/issue.


def _start_prod_batching_server(
saved_bundle_path: str,
api_server_port: int,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question here. Why do we need to explicitly have api_server_port here instead of from config?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

@@ -178,6 +180,7 @@ def __init__(
"or launch more microbatch instances to accept more concurrent connection.",
self.CONNECTION_LIMIT,
)
self._client = None
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably don't need to lazy initialize client here. Client is pretty much always needed, correct?

Copy link
Member Author

@bojiang bojiang Mar 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Client is pretty much always needed, correct?

Yeah. But IMO we best only do value assignment operations in the __init__.

In addition, in this case, an aiohttp client session should be initialized with a running asyncio event loop.
aio-libs/aiohttp#3331

if self._client is None:
jar = aiohttp.DummyCookieJar()
if self.outbound_unix_socket:
conn = aiohttp.UnixConnector(path=self.outbound_unix_socket,)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for UDS.

bento_service, port=api_server_port, enable_swagger=enable_swagger
)
api_server.start()
api_server = BentoAPIServer(bento_service, enable_swagger=enable_swagger)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think start_dev_server could follow the same pattern as start_prod_server?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. But that should be done by another PR. It involves more than the prod server.

@@ -183,14 +181,18 @@ def __init__(

self.setup_routes()

def start(self):
def start(self, port: int, host: str = "127.0.0.1"):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can host be in the config?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. But it should be done in another PR.


import psutil
from dependency_injector.wiring import Provide as P
Copy link
Collaborator

@ssheng ssheng Mar 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a big issue. I think it's more readable to fully spell it out. Same for container.

@parano parano merged commit 97051d2 into bentoml:master Mar 10, 2021
aarnphm pushed a commit to aarnphm/BentoML that referenced this pull request Jul 29, 2022
* [Refactor] earlier fork

* [Refactor] Add server entrypoint module

* format code

* clean

* move configuration to entrypoint

* clean

* revert changes about container

* Move start_prod_server

* make pylint happy

* fix wiring
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants