Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Refactor] fork server processes earlier #1476
Changes from 6 commits
78433d8
d6d5d2f
fa40527
b8a0981
393a90f
02dac88
7aeaa9a
0935aba
2feace9
e214050
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
What do you envision will be added under the /entrypoint package? What do we gain by moving to a new package?
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.
The place before is
/bentoml/server/__init__.py
. I don't think it's a proper place to wire packages includingbentoml.server
itself.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.
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?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 @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: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.
Discussed offline, we will keep these in the original location under
server/__init__.py
.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.
Why do we need to explicitly have port here instead of from config?
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.
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 keepconfig.api_server
rather than overriding it by the randomly picked port.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.
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).
In the container, we can introduce a provider that first checks if there is an intermediate port defined, if not randomly reserve a port.
Basically, a lot of the logic here can be refactored as a provider in
containers.py
.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.
It's great to have a provider for the
intermediate_port
. But we have to wire after creating the new process. In your solution, thereserve_free_port
would be called twice and gaveintermediate_port
different values in each process.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.
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.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.
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.
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.
like this?
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.
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,
Or, if TCP is chosen,
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.
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.
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.
After all, the config structure reflects our system design. We can continue the discussion in the new PR/issue.
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.
Same question here. Why do we need to explicitly have api_server_port here instead of from config?
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.
Same as above.
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.
We probably don't need to lazy initialize client here. Client is pretty much always needed, correct?
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.
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
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.
+1 for UDS.