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

Jump to bottom
Merged
merged 8 commits into from
Jan 31, 2023
Merged

Add environment variable VMARGS #118

merged 8 commits into from
Jan 31, 2023

Conversation

nikhil-sk
Copy link
Contributor

Issue #, if available:

Description of changes:

  1. Currently, there is no direct way to override VMARGS when starting model server. The default-mms.properties is used, which does not specify vmargs.
  2. We can consider adding vmargs to default-mms.properties but that still does not allow customers to easily override it.
  3. Hence, adding an env variable to override it. The defatult value of the env variable is set to: -XX:-UseContainerSupport in accordance with

Merge Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.

General

  • I have read the CONTRIBUTING doc
  • I used the commit message format described in CONTRIBUTING
  • I have used the regional endpoint when creating S3 and/or STS clients (if appropriate)
  • I have updated any necessary documentation, including READMEs

Tests

  • I have added tests that prove my fix is effective or that my feature works (if appropriate)
  • I have checked that my tests are not configured for a specific region or account (if appropriate)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-inference-toolkit-pr
  • Commit ID: c01dde7
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-inference-toolkit-pr
  • Commit ID: e7f4473
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-inference-toolkit-pr
  • Commit ID: 729df77
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

rohithkrn
rohithkrn previously approved these changes Jan 21, 2023
View reviewed changes
@davidthomas426 davidthomas426 mentioned this pull request Jan 22, 2023
Merged
maaquib
maaquib previously approved these changes Jan 23, 2023
View reviewed changes
Copy link
Contributor

@maaquib maaquib left a comment

Choose a reason for hiding this comment

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

LGTM. We should also get this added to the official documentation

@nikhil-sk nikhil-sk dismissed stale reviews from maaquib and rohithkrn via 5b3579c January 25, 2023 00:28
@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-inference-toolkit-pr
  • Commit ID: 5b3579c
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-inference-toolkit-pr
  • Commit ID: 2c9e7d3
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-inference-toolkit-pr
  • Commit ID: 98f4119
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-inference-toolkit-pr
  • Commit ID: 030a532
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

davidthomas426
davidthomas426 previously approved these changes Jan 30, 2023
View reviewed changes
src/sagemaker_inference/model_server.py
@@ -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.

maaquib
maaquib reviewed Jan 30, 2023
View reviewed changes
src/sagemaker_inference/parameters.py Outdated
@@ -24,3 +24,4 @@
BIND_TO_PORT_ENV = "SAGEMAKER_BIND_TO_PORT" # type: str
SAFE_PORT_RANGE_ENV = "SAGEMAKER_SAFE_PORT_RANGE" # type: str
MULTI_MODEL_ENV = "SAGEMAKER_MULTI_MODEL" # type: str
VMARGS = "VMARGS" # type: str
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: Given all other env variables start with SAGEMAKER_ should we keep it consistent for VMARGS as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I think SAGEMAKER_MODEL_SERVER_VMARGS makes more sense, to keep it similar to workers/timeout env variables, I've updated the PR accordingly. Let me know what you think @maaquib @davidthomas426

@sagemaker-bot
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-inference-toolkit-pr
  • Commit ID: d44ea08
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@nikhil-sk nikhil-sk merged commit 3821f30 into aws:master Jan 31, 2023
@nikhil-sk nikhil-sk deleted the env_vmargs branch January 31, 2023 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maaquib maaquib maaquib approved these changes

@davidthomas426 davidthomas426 davidthomas426 approved these changes

@rohithkrn rohithkrn rohithkrn left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nikhil-sk @sagemaker-bot @maaquib @davidthomas426 @rohithkrn