-
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(cli): add CLI command to serve a runner #2920
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2920 +/- ##
==========================================
- Coverage 69.88% 68.92% -0.97%
==========================================
Files 121 122 +1
Lines 9961 10094 +133
==========================================
- Hits 6961 6957 -4
- Misses 3000 3137 +137
|
7b91908
to
9c959b4
Compare
0922de9
to
db74da2
Compare
@@ -11,7 +11,7 @@ _is_sourced() { | |||
|
|||
_main() { | |||
# if first arg looks like a flag | |||
if [ "${1:0:1}" = '-' ]; then | |||
if [ -z "$@" ] || [ "${1:0:1}" = '-' ]; then |
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 -z "$@"
a superset of "${1:0:1}" = '-'
?
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.
Not really
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.
command
:
-z "$@"
-> true
[ "${1:0:1}" = '-' ]
-> false
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.
Minor style nit, but I think [ $# -eq 1 ]
is clearer than [ -z "$@" ]
.
bentoml/serve.py
Outdated
|
||
|
||
@inject | ||
def serve_bare_api_server( |
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.
"Serve" isn't technically accurate here. How about just start_api_server
and start_runner
?
bentoml_cli/serve.py
Outdated
type=click.STRING, | ||
default=None, | ||
envvar="BENTOML_SERVE_COMPONENT", | ||
help="[Experimental] Component (`api-server` or `runner`) to serve, if not specified, will serve all components", |
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.
Should it be api_server
with a underscore? api_server
is how it appears in other references.
sphinx==4.5.0 | ||
setuptools==60.0.0 |
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 is the reason for the downgrade?
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.
ping @aarnphm
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.
this has to do with the newer version of setuptools and it setup different kind of editables install. It is better to just lock setuptools for now.
a9d77f8
to
9d0bdd8
Compare
I think HTTP is more accurate than REST, since our APIs can pretty easily be used to make a non-RESTful server. |
bentoml_cli/serve.py
Outdated
type=click.STRING, | ||
default=None, | ||
envvar="BENTOML_SERVE_COMPONENT", | ||
help="[Experimental] Component (`api-server` or `runner`) to serve, if not specified, will serve all components", |
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.
help="[Experimental] Component (`api-server` or `runner`) to serve, if not specified, will serve all components", | |
help="[EXPERIMENTAL] Component (`api-server` or `runner`) to serve, if not specified, will serve all components", |
bentoml_cli/serve.py
Outdated
type=click.STRING, | ||
default=None, | ||
envvar="BENTOML_SERVE_RUNNER_NAME", | ||
help="[Experimental] required if `component` is `runner`, specify the runner name to serve", |
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.
help="[Experimental] required if `component` is `runner`, specify the runner name to serve", | |
help="[EXPERIMENTAL] required if `component` is `runner`, specify the runner name to serve", |
bentoml_cli/serve.py
Outdated
type=click.STRING, | ||
multiple=True, | ||
envvar="BENTOML_SERVE_RUNNER_MAP", | ||
help="[Experimental] required if `component` is `api-server' JSON string of runners map", |
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.
help="[Experimental] required if `component` is `api-server' JSON string of runners map", | |
help="[EXPERIMENTAL] required if `component` is `api-server' JSON string of runners map", |
sphinx==4.5.0 | ||
setuptools==60.0.0 |
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.
this has to do with the newer version of setuptools and it setup different kind of editables install. It is better to just lock setuptools for now.
|
||
|
||
@inject | ||
def ensure_prometheus_dir( |
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.
Are we duplicating this function?
to arrange our serving CLI/Python APIs like this:
The APIs start with start are low-level APIs used in Yatai, would be hidden from the help message for now
run bare servers from container
docker run -it --rm -p 3000:3000 -e BENTOML_SERVE_COMPONENT=rest_server -e BENTOML_SERVE_RUNNER_MAP="asdfasdf=1 py_model.case-1.e2e=tcp://192.168.67.1:4000" general_workflow_service.case-1.e2e:tefgtebeok7iiusu
docker run -it --rm -p 4000:3000 -e BENTOML_SERVE_COMPONENT=runner -e BENTOML_SERVE_RUNNER_NAME=py_model.case-1.e2e general_workflow_service.case-1.e2e:awvtyqreokd4cusu