-
Notifications
You must be signed in to change notification settings - Fork 867
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
Adding model-control-mode #3165
Conversation
frontend/server/src/main/java/org/pytorch/serve/http/api/rest/ManagementRequestHandler.java
Outdated
Show resolved
Hide resolved
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.
Can you add test for the priority b/w config file vs cmd, especially for the negative test cases (eg. conflict setting)?
frontend/server/src/main/java/org/pytorch/serve/util/ConfigManager.java
Outdated
Show resolved
Hide resolved
frontend/server/src/main/java/org/pytorch/serve/http/api/rest/ManagementRequestHandler.java
Outdated
Show resolved
Hide resolved
frontend/server/src/main/java/org/pytorch/serve/http/api/rest/ManagementRequestHandler.java
Outdated
Show resolved
Hide resolved
frontend/server/src/main/java/org/pytorch/serve/http/api/rest/ManagementRequestHandler.java
Outdated
Show resolved
Hide resolved
frontend/server/src/main/java/org/pytorch/serve/http/api/rest/ManagementRequestHandler.java
Outdated
Show resolved
Hide resolved
frontend/server/src/main/java/org/pytorch/serve/http/api/rest/ManagementRequestHandler.java
Outdated
Show resolved
Hide resolved
frontend/server/src/main/java/org/pytorch/serve/util/ConfigManager.java
Outdated
Show resolved
Hide resolved
frontend/server/src/main/java/org/pytorch/serve/util/ConfigManager.java
Outdated
Show resolved
Hide resolved
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.
Few minor comments to address. Looks good otherwise!
frontend/server/src/main/java/org/pytorch/serve/http/api/rest/ManagementRequestHandler.java
Show resolved
Hide resolved
frontend/server/src/main/java/org/pytorch/serve/http/api/rest/ManagementRequestHandler.java
Outdated
Show resolved
Hide resolved
frontend/server/src/main/java/org/pytorch/serve/http/api/rest/ManagementRequestHandler.java
Outdated
Show resolved
Hide resolved
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 chosen parameter name is not reflecting the actual functionality in TorchServe. Please see comments.
frontend/server/src/main/java/org/pytorch/serve/util/ConfigManager.java
Outdated
Show resolved
Hide resolved
test/pytest/test_utils.py
Outdated
@@ -59,6 +59,8 @@ def start_torchserve( | |||
no_config_snapshots=False, | |||
gen_mar=True, | |||
plugin_folder=None, | |||
models=None, | |||
mode=None, |
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 default mode is called "none" in the docs. But here None selects explicit. Better to revert the logic to no confuse people.
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 logic for test is to keep enabled as default so that it does not break existing test structure for now. Will require complete test overhaul at a later date.
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.
You can just set the default value to model the old behavior to not break any test.
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 want TorchServe default to be model api enabled as false meaning that users will not be able to register or delete models once it starts. That change will break the tests
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.
Udai, this is not about the way people will setup TorchServe, this is only about the interface of start_torchserve which is not user facing.
|
||
cmd line: `torchserve --start --ncs --model-store model_store --model-api-enabled` | ||
|
||
Result: Model api mode enabled |
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.
Describe here the values that were set in the config file and command line and which took precedence to make it clear to the reader.
docs/model_control_mode.md
Outdated
|
||
cmd line: `torchserve --start --ncs --model-store model_store` | ||
|
||
Result: Mode is enabled (no way to enable "none" through cmd) |
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: Describe here the values that were set in the config file and command line and which took precedence to make it clear to the reader.
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.
Addressed my own comments to move on with this. LGTM now
test/pytest/test_utils.py
Outdated
@@ -59,6 +59,8 @@ def start_torchserve( | |||
no_config_snapshots=False, | |||
gen_mar=True, | |||
plugin_folder=None, | |||
models=None, | |||
mode=None, |
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.
Udai, this is not about the way people will setup TorchServe, this is only about the interface of start_torchserve which is not user facing.
Path(test_utils.MODEL_STORE).mkdir(parents=True, exist_ok=True) | ||
|
||
test_utils.start_torchserve( | ||
no_config_snapshots=True, models="mnist=mnist.mar", mode="none" |
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 interface should be
def start_torchserve(..., model_control_enabled=True):
so when you setup a test that want the new behavior you use:
test_utils.start_torchserve(..., model_control_enabled=False)
Description
Adding a model-control-mode to TorchServe #3158
Default is
none
which prevents users from registering and unregistering models after startup.Can be set to
enabled
which allows users to use register and unregister APIsCheck documentation for how to set the model-control-mode
Tests
Setting model mode to "enabled" for all current tests.
Added a new pytest test_model_control_mode.py that covers default and model api enabled