Skip to content

Add environment variable VMARGS #118

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Jan 31, 2023
Merged

Add environment variable VMARGS #118

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c01dde7
Add environment variable with VMARGS
nikhil-sk Jan 20, 2023
e7f4473
Fix linting error
nikhil-sk Jan 21, 2023
729df77
Fix flake8
nikhil-sk Jan 21, 2023
5b3579c
Fix mock calls in default handler test
nikhil-sk Jan 25, 2023
2c9e7d3
Remove duplicate assert
nikhil-sk Jan 25, 2023
98f4119
Fix function call
nikhil-sk Jan 25, 2023
030a532
Use pytest instead of py.test
nikhil-sk Jan 28, 2023
d44ea08
Change env variable name
nikhil-sk Jan 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/sagemaker_inference/environment.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
DEFAULT_MODEL_SERVER_TIMEOUT = "60"
DEFAULT_STARTUP_TIMEOUT = "600" # 10 minutes
DEFAULT_HTTP_PORT = "8080"
DEFAULT_VMARGS = "-XX:-UseContainerSupport"

SAGEMAKER_BASE_PATH = os.path.join("/opt", "ml") # type: str

Expand Down Expand Up @@ -79,6 +80,7 @@ def __init__(self):
self._inference_http_port = os.environ.get(parameters.BIND_TO_PORT_ENV, DEFAULT_HTTP_PORT)
self._management_http_port = os.environ.get(parameters.BIND_TO_PORT_ENV, DEFAULT_HTTP_PORT)
self._safe_port_range = os.environ.get(parameters.SAFE_PORT_RANGE_ENV)
self._vmargs = os.environ.get(parameters.MODEL_SERVER_VMARGS, DEFAULT_VMARGS)

@staticmethod
def _parse_module_name(program_param):
Expand Down Expand Up @@ -140,3 +142,8 @@ def safe_port_range(self): # type: () -> str
specified by SageMaker for handling pings and invocations.
"""
return self._safe_port_range

@property
def vmargs(self): # type: () -> str
"""str: vmargs can be provided for the JVM, to be overriden"""
return self._vmargs
2 changes: 1 addition & 1 deletion src/sagemaker_inference/model_server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ def _generate_mms_config_properties(env, handler_service=None):
"default_workers_per_model": env.model_server_workers,
"inference_address": "http://0.0.0.0:{}".format(env.inference_http_port),
"management_address": "http://0.0.0.0:{}".format(env.management_http_port),
"vmargs": "-XX:-UseContainerSupport",
"vmargs": env.vmargs,
Copy link
Contributor

Choose a reason for hiding this comment

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

If user currently creates their own derived container with their own version of the config properties file with vmargs, will this break their use case?

If I'm reading the old and new code correctly, I think any vmargs specified in the properties file would have gotten ignored (in old code), and will still be ignored (in new code), whether or not they set the environment variable. Am I right?

Here's my analysis: in the case where properties file has vmargs, there will be a duplicate key, we're currently requiring on undefined behavior in Java properties spec (see https://stackoverflow.com/a/15570024), but in practice the behavior is that the last value wins. That means that in this case, the value in env.vmargs will override any value in the config file, which is the same thing that would have happened before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@davidthomas426 That's correct, vmargs specified in config file was ignored in old code, and will be ignored in the new code as well. Since env.vmargs is set after reading config file, it should override any previous value, if specified at all, else will use default value.

}
# If provided, add handler service to user config
if handler_service:
Expand Down
1 change: 1 addition & 0 deletions src/sagemaker_inference/parameters.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
DEFAULT_INVOCATIONS_ACCEPT_ENV = "SAGEMAKER_DEFAULT_INVOCATIONS_ACCEPT" # type: str
MODEL_SERVER_WORKERS_ENV = "SAGEMAKER_MODEL_SERVER_WORKERS" # type: str
MODEL_SERVER_TIMEOUT_ENV = "SAGEMAKER_MODEL_SERVER_TIMEOUT" # type: str
MODEL_SERVER_VMARGS = "SAGEMAKER_MODEL_SERVER_VMARGS" # type: str
STARTUP_TIMEOUT_ENV = "SAGEMAKER_STARTUP_TIMEOUT" # type: str
BIND_TO_PORT_ENV = "SAGEMAKER_BIND_TO_PORT" # type: str
SAFE_PORT_RANGE_ENV = "SAGEMAKER_SAFE_PORT_RANGE" # type: str
Expand Down
4 changes: 2 additions & 2 deletions test/unit/test_default_handler_service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ def test_handle():
result = handler_service.handle(DATA, CONTEXT)

assert result == TRANSFORMED_RESULT
assert transformer.transform.called_once_with(DATA, CONTEXT)
transformer.transform.assert_called_once_with(DATA, CONTEXT)


def test_initialize():
Expand All @@ -57,4 +57,4 @@ def getitem(key):
context.system_properties.__getitem__.side_effect = getitem
DefaultHandlerService(transformer).initialize(context)

assert transformer.validate_and_initialize().called_once()
transformer.validate_and_initialize.assert_called_once()
2 changes: 2 additions & 0 deletions test/unit/test_environment.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
parameters.DEFAULT_INVOCATIONS_ACCEPT_ENV: "text/html",
parameters.BIND_TO_PORT_ENV: "1738",
parameters.SAFE_PORT_RANGE_ENV: "1111-2222",
parameters.MODEL_SERVER_VMARGS: "-XX:-UseContainerSupport",
},
clear=True,
)
Expand All @@ -45,6 +46,7 @@ def test_env():
assert env.inference_http_port == "1738"
assert env.management_http_port == "1738"
assert env.safe_port_range == "1111-2222"
assert "-XX:-UseContainerSupport" in env.vmargs


@pytest.mark.parametrize("sagemaker_program", ["program.py", "program"])
Expand Down
2 changes: 1 addition & 1 deletion tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ passenv =
# {posargs} can be passed in by additional arguments specified when invoking tox.
# Can be used to specify which tests to run, e.g.: tox -- -s
commands =
coverage run --rcfile .coveragerc_{envname} --source sagemaker_inference -m py.test {posargs}
coverage run --rcfile .coveragerc_{envname} --source sagemaker_inference -m pytest {posargs}
{env:IGNORE_COVERAGE:} coverage report --rcfile .coveragerc_{envname}
{env:IGNORE_COVERAGE:} coverage html --rcfile .coveragerc_{envname}

Expand Down