feature: Add Granular Target Description support for compilation (Neo) - #1752

Merged
laurenyu merged 1 commit into
aws:masterfrom
apivovarov:GTDD
Jul 28, 2020
Merged

feature: Add Granular Target Description support for compilation (Neo)#1752
laurenyu merged 1 commit into
aws:masterfrom
apivovarov:GTDD

Conversation

@apivovarov

@apivovarovapivovarov commented Jul 24, 2020•
edited
Loading

Copy link
Copy Markdown
Contributor

Description

Add Granular Target Description support for DL model compilation.
Example:

 model.compile(
target_instance_family=None,
input_shape={"data": [1, 3, 1024, 1024]},
output_path="s3://output",
role="role",
framework="tensorflow",
job_name="compile-model",
target_platform_os="LINUX",
target_platform_arch="ARM64",
target_platform_accelerator="NVIDIA",
compiler_options={"gpu-code": "sm_72", "trt-ver": "6.0.1", "cuda-ver": "10.1"}
)

Tests

  • I have added tests that prove my fix is effective or that my feature works (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

sagemaker-bot commented Jul 24, 2020

Copy link
Copy Markdown
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: ffec986
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@sagemaker-bot

sagemaker-bot commented Jul 24, 2020

Copy link
Copy Markdown
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: ffec986
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@sagemaker-bot

sagemaker-bot commented Jul 24, 2020

Copy link
Copy Markdown
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: ffec986
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@sagemaker-bot

sagemaker-bot commented Jul 24, 2020

Copy link
Copy Markdown
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-pr
  • Commit ID: ffec986
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@sagemaker-bot

sagemaker-bot commented Jul 24, 2020

Copy link
Copy Markdown
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: b42989f
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@sagemaker-bot

sagemaker-bot commented Jul 24, 2020

Copy link
Copy Markdown
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: b42989f
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@sagemaker-bot

sagemaker-bot commented Jul 24, 2020

Copy link
Copy Markdown
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: b42989f
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@sagemaker-bot

sagemaker-bot commented Jul 25, 2020

Copy link
Copy Markdown
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: 6f5f6af
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@sagemaker-bot

sagemaker-bot commented Jul 25, 2020

Copy link
Copy Markdown
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: 6f5f6af
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@sagemaker-bot

sagemaker-bot commented Jul 25, 2020

Copy link
Copy Markdown
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-pr
  • Commit ID: 6f5f6af
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@sagemaker-bot

sagemaker-bot commented Jul 25, 2020

Copy link
Copy Markdown
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: 6f5f6af
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@laurenyulaurenyu changed the title neo: Add Granular Target Description support for compilationfeature: Add Granular Target Description support for compilation (Neo)Jul 27, 2020
laurenyu
laurenyu reviewed Jul 27, 2020
View reviewed changes
Comment threadsrc/sagemaker/estimator.py
Comment threadsrc/sagemaker/model.py Outdated
Comment on lines +253 to +256
assert target_platform_os is not None and target_platform_arch is not None, (
"target_instance_type or (target_platform_os and target_platform_arch) "
"should be provided"
)

@laurenyulaurenyuJul 27, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it's better practice to avoid using asserts in source code, and instead raise a ValueError in this case

https://stackoverflow.com/a/945135
https://stackoverflow.com/a/1838411

@apivovarovapivovarovJul 28, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Replaced with ValueError

Comment threadsrc/sagemaker/model.py Outdated
run your model after compilation, for example: ml_c5. For allowed
strings see
https://docs.aws.amazon.com/sagemaker/latest/dg/API_OutputConfig.html.
Alternatively, you can select OS, Arch and Accelerator using Target Platform

@laurenyulaurenyuJul 27, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
Alternatively, youcanselectOS, ArchandAcceleratorusingTargetPlatform
Alternatively, youcanselectanOS, Architecture,andAcceleratorusing
``target_platform_os``, ``target_platform_arch``, and``target_platform_accelerator``.

@apivovarovapivovarovJul 28, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

fixed as suggested

Comment threadsrc/sagemaker/model.py Outdated
Comment on lines +378 to +384
target_platform_os (str): Target Platform OS. Allowed values: 'LINUX', 'ANDROID'.
It can be used instead of target_instance_family.
target_platform_arch (str): Target Platform Architecture. Allowed values: 'X86_64',
'X86', 'ARM64', 'ARM_EABIHF', 'ARM_EABI'.
It can be used instead of target_instance_family.
target_platform_accelerator (str): Target Platform Accelerator. Allowed values:
'INTEL_GRAPHICS', 'MALI', 'NVIDIA'. Optional.

@laurenyulaurenyuJul 27, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can you instead link to docs that list the allowed versions? I assume they might change in the future, and if that happens, this docstring will inevitably be forgotten.

@mchoi8739mchoi8739Jul 27, 2020•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The API reference is updated:
OutputConfig API
TargetPlatform API

It would be good to provide these links as well.

@apivovarovapivovarovJul 28, 2020•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I left one example value and added link to allowed values.

Comment threadsrc/sagemaker/model.py Outdated
It can be used instead of target_instance_family.
target_platform_accelerator (str): Target Platform Accelerator. Allowed values:
'INTEL_GRAPHICS', 'MALI', 'NVIDIA'. Optional.
compiler_options (str): Additional parameters for compiler (Optional, in JSON format).

@laurenyulaurenyuJul 27, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if it needs to be in JSON, would it make more sense to accept a dict, and then let the Python SDK call json.dumps?

@apivovarovapivovarovJul 28, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Fixed, now it support both dict and string. Similar to DataInputConfig

Comment threadsrc/sagemaker/model.py Outdated
Comment on lines +451 to +464
"Devices described by Target Platform Os, Arch and Accelerator are not supported"
"to deploy via SageMaker, please deploy the model manually."

@laurenyulaurenyuJul 27, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Devices described by Target Platform Os, Arch and Accelerator are not supported"
"to deploy via SageMaker, please deploy the model manually."
"Deploying devices described by Target Platform OS, Architecture, and Accelerator "
"is not supported via SageMaker. Please deploy the model manually."

@apivovarovapivovarovJul 28, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Can we keep old comment? I like it better.

@laurenyulaurenyuJul 28, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

"not supported to deploy" is the part that was bothering me. maybe "not supported for deployment"?

@apivovarovapivovarovJul 28, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

ok, changed to not supported for deployment.

Comment threadsrc/sagemaker/model.py Outdated
Comment on lines +445 to +459
"The instance type %s is not supported to deploy via SageMaker,"
"please deploy the model manually.",
target_instance_family,

@laurenyulaurenyuJul 27, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(I know this isn't your change, but while you're here...)

Suggested change
"The instance type %s is not supported to deploy via SageMaker,"
"please deploy the model manually.",
target_instance_family,
"Deploying instance type %s is not supported via SageMaker."
"Please deploy the model manually.",
target_instance_family,

@apivovarovapivovarovJul 28, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Can we keep old comment? I like it better.

@apivovarovapivovarovJul 28, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

changed to not supported for deployment

Comment threadtests/unit/sagemaker/model/test_neo.py
mchoi8739
mchoi8739 reviewed Jul 27, 2020
View reviewed changes
Comment threadtests/unit/sagemaker/model/test_neo.py
model = _create_model(sagemaker_session)
model.compile(
target_instance_family=None,
input_shape={"data": [1, 3, 1024, 1024]},

@mchoi8739mchoi8739Jul 27, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

input_shape={'data': [1, 3, 1024, 1024]}

^ suggesting to use single-quote (based on what we discussed earlier)

@apivovarovapivovarovJul 28, 2020•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

because of black-check python code style check we have to use double-quotes here

mchoi8739
mchoi8739 reviewed Jul 27, 2020
View reviewed changes
Comment threadtests/unit/sagemaker/model/test_neo.py
model = _create_model(sagemaker_session)
model.compile(
target_instance_family=None,
input_shape={"data": [1, 3, 1024, 1024]},

@mchoi8739mchoi8739Jul 27, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

input_shape={'data': [1, 3, 1024, 1024]}

^ suggesting to use single-quote (based on what we discussed earlier)

@apivovarovapivovarovJul 28, 2020•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This is not JSON-like String which we use in Console UI or AWS CLI.
This is Python code. In Python SDK we use Dictionary {...} instead of JSON-string "{...}".
Technically we can use single quotes here. However, This project automatic build runs Python code style check (black-check) and it requires all strings to use double-quoutes for Strings in Python code.

mchoi8739
mchoi8739 reviewed Jul 27, 2020
View reviewed changes
Comment threadsrc/sagemaker/model.py Outdated
It can be used instead of target_instance_family.
target_platform_accelerator (str): Target Platform Accelerator. Allowed values:
'INTEL_GRAPHICS', 'MALI', 'NVIDIA'. Optional.
compiler_options (str): Additional parameters for compiler (Optional, in JSON format).

@mchoi8739mchoi8739Jul 27, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We may need to point out here as well that target_instance_family and target_platform_* are not usable together?

@apivovarovapivovarovJul 28, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

For each target_platform_* field documentation I added - It can be used instead of target_instance_family.

@sagemaker-bot

sagemaker-bot commented Jul 28, 2020

Copy link
Copy Markdown
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: 338dad1
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@sagemaker-bot

sagemaker-bot commented Jul 28, 2020

Copy link
Copy Markdown
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: c7fb5ed
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@sagemaker-bot

sagemaker-bot commented Jul 28, 2020

Copy link
Copy Markdown
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: 338dad1
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@sagemaker-bot

sagemaker-bot commented Jul 28, 2020

Copy link
Copy Markdown
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: c7fb5ed
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@sagemaker-bot

sagemaker-bot commented Jul 28, 2020

Copy link
Copy Markdown
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-pr
  • Commit ID: c7fb5ed
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@sagemaker-bot

sagemaker-bot commented Jul 28, 2020

Copy link
Copy Markdown
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: 338dad1
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@sagemaker-bot

sagemaker-bot commented Jul 28, 2020

Copy link
Copy Markdown
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: c7fb5ed
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

laurenyu
laurenyu reviewed Jul 28, 2020
View reviewed changes
Comment threadsrc/sagemaker/model.py Outdated
Comment on lines +268 to +270
compiler_options
if not isinstance(compiler_options, dict)
else json.dumps(compiler_options)

@laurenyulaurenyuJul 28, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

let's flip this around to put the positive condition first, i.e. json.dumps(compiler_options) if isinstance(compiler_options, dict) else compiler_options

@apivovarovapivovarovJul 28, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

fixed here and for DataInputConfig as well.

@sagemaker-bot

sagemaker-bot commented Jul 28, 2020

Copy link
Copy Markdown
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: fe8ba9d
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@sagemaker-bot

sagemaker-bot commented Jul 28, 2020

Copy link
Copy Markdown
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: fe8ba9d
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@sagemaker-bot

sagemaker-bot commented Jul 28, 2020

Copy link
Copy Markdown
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: fe8ba9d
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@sagemaker-bot

sagemaker-bot commented Jul 28, 2020

Copy link
Copy Markdown
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: sagemaker-python-sdk-pr
  • Commit ID: fe8ba9d
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@laurenyu
laurenyu merged commit 20751c9 into aws:masterJul 28, 2020
Evan-W-ang added a commit to Evan-W-ang/sagemaker-python-sdk that referenced this pull request Jun 8, 2026
@Evan-W-ang
feature: Neo: Add Granular Target Description support for compilation (…
77b6fc9
…aws#1752)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers
@laurenyulaurenyulaurenyu approved these changes
@mchoi8739mchoi8739mchoi8739 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@apivovarov@sagemaker-bot@laurenyu@mchoi8739
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
feature: Add Granular Target Description support for compilation (Neo) - #1752
Merged
laurenyu merged 1 commit into
aws:masterfrom 
apivovarov:GTDD
Jul 28, 2020
Merged
feature: Add Granular Target Description support for compilation (Neo)#1752
laurenyu merged 1 commit into
aws:masterfrom 
apivovarov:GTDD
Conversation
@apivovarov
@apivovarovapivovarov
commented
Jul 24, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
Description
Add Granular Target Description support for DL model compilation.

Example:
 model.compile(
target_instance_family=None,
input_shape={"data": [1, 3, 1024, 1024]},
output_path="s3://output",
role="role",
framework="tensorflow",
job_name="compile-model",
target_platform_os="LINUX",
target_platform_arch="ARM64",
target_platform_accelerator="NVIDIA",
compiler_options={"gpu-code": "sm_72", "trt-ver": "6.0.1", "cuda-ver": "10.1"}
)

Tests
  •  I have added tests that prove my fix is effective or that my feature works (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
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: ffec986
  • Result: FAILED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: ffec986
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: ffec986
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-pr
  • Commit ID: ffec986
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: b42989f
  • Result: FAILED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: b42989f
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: b42989f
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 25, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: 6f5f6af
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 25, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: 6f5f6af
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 25, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-pr
  • Commit ID: 6f5f6af
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 25, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: 6f5f6af
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@laurenyulaurenyu
changed the title
neo: Add Granular Target Description support for compilationfeature: Add Granular Target Description support for compilation (Neo)Jul 27, 2020
laurenyu
laurenyu
reviewed
Jul 27, 2020
View reviewed changes
Comment threadsrc/sagemaker/estimator.py
Comment threadsrc/sagemaker/model.py
Outdated


Comment on lines

+253
 to 
+256

 assert target_platform_os is not None and target_platform_arch is not None, (
 "target_instance_type or (target_platform_os and target_platform_arch) "
 "should be provided"
 )
@laurenyulaurenyuJul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

it's better practice to avoid using asserts in source code, and instead raise a ValueError in this case
https://stackoverflow.com/a/945135
https://stackoverflow.com/a/1838411
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Replaced with ValueError
Comment threadsrc/sagemaker/model.py
Outdated

 run your model after compilation, for example: ml_c5. For allowed
 strings see
 https://docs.aws.amazon.com/sagemaker/latest/dg/API_OutputConfig.html.
 Alternatively, you can select OS, Arch and Accelerator using Target Platform
@laurenyulaurenyuJul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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


Suggested change

Alternatively, youcanselectOS, ArchandAcceleratorusingTargetPlatform
Alternatively, youcanselectanOS, Architecture,andAcceleratorusing
``target_platform_os``, ``target_platform_arch``, and``target_platform_accelerator``.
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

fixed as suggested
Comment threadsrc/sagemaker/model.py
Outdated


Comment on lines

+378
 to 
+384

 target_platform_os (str): Target Platform OS. Allowed values: 'LINUX', 'ANDROID'.
 It can be used instead of target_instance_family.
 target_platform_arch (str): Target Platform Architecture. Allowed values: 'X86_64',
 'X86', 'ARM64', 'ARM_EABIHF', 'ARM_EABI'.
 It can be used instead of target_instance_family.
 target_platform_accelerator (str): Target Platform Accelerator. Allowed values:
 'INTEL_GRAPHICS', 'MALI', 'NVIDIA'. Optional.
@laurenyulaurenyuJul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

can you instead link to docs that list the allowed versions? I assume they might change in the future, and if that happens, this docstring will inevitably be forgotten.
@mchoi8739mchoi8739Jul 27, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

The API reference is updated:
OutputConfig API
TargetPlatform API
It would be good to provide these links as well.
@apivovarovapivovarovJul 28, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I left one example value and added link to allowed values.
Comment threadsrc/sagemaker/model.py
Outdated

 It can be used instead of target_instance_family.
 target_platform_accelerator (str): Target Platform Accelerator. Allowed values:
 'INTEL_GRAPHICS', 'MALI', 'NVIDIA'. Optional.
 compiler_options (str): Additional parameters for compiler (Optional, in JSON format).
@laurenyulaurenyuJul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

if it needs to be in JSON, would it make more sense to accept a dict, and then let the Python SDK call json.dumps?
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Fixed, now it support both dict and string. Similar to DataInputConfig
Comment threadsrc/sagemaker/model.py
Outdated


Comment on lines

+451
 to 
+464

 "Devices described by Target Platform Os, Arch and Accelerator are not supported"
 "to deploy via SageMaker, please deploy the model manually."
@laurenyulaurenyuJul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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


Suggested change

"Devices described by Target Platform Os, Arch and Accelerator are not supported"
"to deploy via SageMaker, please deploy the model manually."
"Deploying devices described by Target Platform OS, Architecture, and Accelerator "
"is not supported via SageMaker. Please deploy the model manually."
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Can we keep old comment? I like it better.
@laurenyulaurenyuJul 28, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

"not supported to deploy" is the part that was bothering me. maybe "not supported for deployment"?
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

ok, changed to not supported for deployment.
Comment threadsrc/sagemaker/model.py
Outdated


Comment on lines

+445
 to 
+459

 "The instance type %s is not supported to deploy via SageMaker,"
 "please deploy the model manually.",
 target_instance_family,
@laurenyulaurenyuJul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

(I know this isn't your change, but while you're here...)

Suggested change

"The instance type %s is not supported to deploy via SageMaker,"
"please deploy the model manually.",
target_instance_family,
"Deploying instance type %s is not supported via SageMaker."
"Please deploy the model manually.",
target_instance_family,
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Can we keep old comment? I like it better.
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

changed to not supported for deployment
Comment threadtests/unit/sagemaker/model/test_neo.py
mchoi8739
mchoi8739
reviewed
Jul 27, 2020
View reviewed changes
Comment threadtests/unit/sagemaker/model/test_neo.py
 model = _create_model(sagemaker_session)
 model.compile(
 target_instance_family=None,
 input_shape={"data": [1, 3, 1024, 1024]},
@mchoi8739mchoi8739Jul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

input_shape={'data': [1, 3, 1024, 1024]}
^ suggesting to use single-quote (based on what we discussed earlier)
@apivovarovapivovarovJul 28, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

because of black-check python code style check we have to use double-quotes here
mchoi8739
mchoi8739
reviewed
Jul 27, 2020
View reviewed changes
Comment threadtests/unit/sagemaker/model/test_neo.py
 model = _create_model(sagemaker_session)
 model.compile(
 target_instance_family=None,
 input_shape={"data": [1, 3, 1024, 1024]},
@mchoi8739mchoi8739Jul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

input_shape={'data': [1, 3, 1024, 1024]}
^ suggesting to use single-quote (based on what we discussed earlier)
@apivovarovapivovarovJul 28, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

This is not JSON-like String which we use in Console UI or AWS CLI.

This is Python code. In Python SDK we use Dictionary {...} instead of JSON-string "{...}".

Technically we can use single quotes here. However, This project automatic build runs Python code style check (black-check) and it requires all strings to use double-quoutes for Strings in Python code.
mchoi8739
mchoi8739
reviewed
Jul 27, 2020
View reviewed changes
Comment threadsrc/sagemaker/model.py
Outdated

 It can be used instead of target_instance_family.
 target_platform_accelerator (str): Target Platform Accelerator. Allowed values:
 'INTEL_GRAPHICS', 'MALI', 'NVIDIA'. Optional.
 compiler_options (str): Additional parameters for compiler (Optional, in JSON format).
@mchoi8739mchoi8739Jul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

We may need to point out here as well that target_instance_family and target_platform_* are not usable together?
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

For each target_platform_* field documentation I added - It can be used instead of target_instance_family.
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: 338dad1
  • Result: FAILED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: c7fb5ed
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: 338dad1
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: c7fb5ed
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-pr
  • Commit ID: c7fb5ed
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: 338dad1
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: c7fb5ed
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
laurenyu
laurenyu
reviewed
Jul 28, 2020
View reviewed changes
Comment threadsrc/sagemaker/model.py
Outdated


Comment on lines

+268
 to 
+270

 compiler_options
 if not isinstance(compiler_options, dict)
 else json.dumps(compiler_options)
@laurenyulaurenyuJul 28, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

let's flip this around to put the positive condition first, i.e. json.dumps(compiler_options) if isinstance(compiler_options, dict) else compiler_options
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

fixed here and for DataInputConfig as well.
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: fe8ba9d
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: fe8ba9d
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: fe8ba9d
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-pr
  • Commit ID: fe8ba9d
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@laurenyu
laurenyu
merged commit 20751c9
into
aws:masterJul 28, 2020
Evan-W-ang added a commit
to Evan-W-ang/sagemaker-python-sdk
that referenced
this pull request
Jun 8, 2026
@Evan-W-ang
feature: Neo: Add Granular Target Description support for compilation (…
77b6fc9
…aws#1752)
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers


2 more reviewers

@laurenyulaurenyulaurenyu approved these changes
@mchoi8739mchoi8739mchoi8739 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@apivovarov@sagemaker-bot@laurenyu@mchoi8739

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
feature: Add Granular Target Description support for compilation (Neo) - #1752
Merged
laurenyu merged 1 commit into
aws:masterfrom 
apivovarov:GTDD
Jul 28, 2020
Merged
feature: Add Granular Target Description support for compilation (Neo)#1752
laurenyu merged 1 commit into
aws:masterfrom 
apivovarov:GTDD
Conversation
@apivovarov
@apivovarovapivovarov
commented
Jul 24, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
Description
Add Granular Target Description support for DL model compilation.

Example:
 model.compile(
target_instance_family=None,
input_shape={"data": [1, 3, 1024, 1024]},
output_path="s3://output",
role="role",
framework="tensorflow",
job_name="compile-model",
target_platform_os="LINUX",
target_platform_arch="ARM64",
target_platform_accelerator="NVIDIA",
compiler_options={"gpu-code": "sm_72", "trt-ver": "6.0.1", "cuda-ver": "10.1"}
)

Tests
  •  I have added tests that prove my fix is effective or that my feature works (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
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: ffec986
  • Result: FAILED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: ffec986
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: ffec986
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-pr
  • Commit ID: ffec986
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: b42989f
  • Result: FAILED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: b42989f
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: b42989f
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 25, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: 6f5f6af
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 25, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: 6f5f6af
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 25, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-pr
  • Commit ID: 6f5f6af
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 25, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: 6f5f6af
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@laurenyulaurenyu
changed the title
neo: Add Granular Target Description support for compilationfeature: Add Granular Target Description support for compilation (Neo)Jul 27, 2020
laurenyu
laurenyu
reviewed
Jul 27, 2020
View reviewed changes
Comment threadsrc/sagemaker/estimator.py
Comment threadsrc/sagemaker/model.py
Outdated


Comment on lines

+253
 to 
+256

 assert target_platform_os is not None and target_platform_arch is not None, (
 "target_instance_type or (target_platform_os and target_platform_arch) "
 "should be provided"
 )
@laurenyulaurenyuJul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

it's better practice to avoid using asserts in source code, and instead raise a ValueError in this case
https://stackoverflow.com/a/945135
https://stackoverflow.com/a/1838411
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Replaced with ValueError
Comment threadsrc/sagemaker/model.py
Outdated

 run your model after compilation, for example: ml_c5. For allowed
 strings see
 https://docs.aws.amazon.com/sagemaker/latest/dg/API_OutputConfig.html.
 Alternatively, you can select OS, Arch and Accelerator using Target Platform
@laurenyulaurenyuJul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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


Suggested change

Alternatively, youcanselectOS, ArchandAcceleratorusingTargetPlatform
Alternatively, youcanselectanOS, Architecture,andAcceleratorusing
``target_platform_os``, ``target_platform_arch``, and``target_platform_accelerator``.
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

fixed as suggested
Comment threadsrc/sagemaker/model.py
Outdated


Comment on lines

+378
 to 
+384

 target_platform_os (str): Target Platform OS. Allowed values: 'LINUX', 'ANDROID'.
 It can be used instead of target_instance_family.
 target_platform_arch (str): Target Platform Architecture. Allowed values: 'X86_64',
 'X86', 'ARM64', 'ARM_EABIHF', 'ARM_EABI'.
 It can be used instead of target_instance_family.
 target_platform_accelerator (str): Target Platform Accelerator. Allowed values:
 'INTEL_GRAPHICS', 'MALI', 'NVIDIA'. Optional.
@laurenyulaurenyuJul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

can you instead link to docs that list the allowed versions? I assume they might change in the future, and if that happens, this docstring will inevitably be forgotten.
@mchoi8739mchoi8739Jul 27, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

The API reference is updated:
OutputConfig API
TargetPlatform API
It would be good to provide these links as well.
@apivovarovapivovarovJul 28, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I left one example value and added link to allowed values.
Comment threadsrc/sagemaker/model.py
Outdated

 It can be used instead of target_instance_family.
 target_platform_accelerator (str): Target Platform Accelerator. Allowed values:
 'INTEL_GRAPHICS', 'MALI', 'NVIDIA'. Optional.
 compiler_options (str): Additional parameters for compiler (Optional, in JSON format).
@laurenyulaurenyuJul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

if it needs to be in JSON, would it make more sense to accept a dict, and then let the Python SDK call json.dumps?
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Fixed, now it support both dict and string. Similar to DataInputConfig
Comment threadsrc/sagemaker/model.py
Outdated


Comment on lines

+451
 to 
+464

 "Devices described by Target Platform Os, Arch and Accelerator are not supported"
 "to deploy via SageMaker, please deploy the model manually."
@laurenyulaurenyuJul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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


Suggested change

"Devices described by Target Platform Os, Arch and Accelerator are not supported"
"to deploy via SageMaker, please deploy the model manually."
"Deploying devices described by Target Platform OS, Architecture, and Accelerator "
"is not supported via SageMaker. Please deploy the model manually."
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Can we keep old comment? I like it better.
@laurenyulaurenyuJul 28, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

"not supported to deploy" is the part that was bothering me. maybe "not supported for deployment"?
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

ok, changed to not supported for deployment.
Comment threadsrc/sagemaker/model.py
Outdated


Comment on lines

+445
 to 
+459

 "The instance type %s is not supported to deploy via SageMaker,"
 "please deploy the model manually.",
 target_instance_family,
@laurenyulaurenyuJul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

(I know this isn't your change, but while you're here...)

Suggested change

"The instance type %s is not supported to deploy via SageMaker,"
"please deploy the model manually.",
target_instance_family,
"Deploying instance type %s is not supported via SageMaker."
"Please deploy the model manually.",
target_instance_family,
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Can we keep old comment? I like it better.
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

changed to not supported for deployment
Comment threadtests/unit/sagemaker/model/test_neo.py
mchoi8739
mchoi8739
reviewed
Jul 27, 2020
View reviewed changes
Comment threadtests/unit/sagemaker/model/test_neo.py
 model = _create_model(sagemaker_session)
 model.compile(
 target_instance_family=None,
 input_shape={"data": [1, 3, 1024, 1024]},
@mchoi8739mchoi8739Jul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

input_shape={'data': [1, 3, 1024, 1024]}
^ suggesting to use single-quote (based on what we discussed earlier)
@apivovarovapivovarovJul 28, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

because of black-check python code style check we have to use double-quotes here
mchoi8739
mchoi8739
reviewed
Jul 27, 2020
View reviewed changes
Comment threadtests/unit/sagemaker/model/test_neo.py
 model = _create_model(sagemaker_session)
 model.compile(
 target_instance_family=None,
 input_shape={"data": [1, 3, 1024, 1024]},
@mchoi8739mchoi8739Jul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

input_shape={'data': [1, 3, 1024, 1024]}
^ suggesting to use single-quote (based on what we discussed earlier)
@apivovarovapivovarovJul 28, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

This is not JSON-like String which we use in Console UI or AWS CLI.

This is Python code. In Python SDK we use Dictionary {...} instead of JSON-string "{...}".

Technically we can use single quotes here. However, This project automatic build runs Python code style check (black-check) and it requires all strings to use double-quoutes for Strings in Python code.
mchoi8739
mchoi8739
reviewed
Jul 27, 2020
View reviewed changes
Comment threadsrc/sagemaker/model.py
Outdated

 It can be used instead of target_instance_family.
 target_platform_accelerator (str): Target Platform Accelerator. Allowed values:
 'INTEL_GRAPHICS', 'MALI', 'NVIDIA'. Optional.
 compiler_options (str): Additional parameters for compiler (Optional, in JSON format).
@mchoi8739mchoi8739Jul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

We may need to point out here as well that target_instance_family and target_platform_* are not usable together?
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

For each target_platform_* field documentation I added - It can be used instead of target_instance_family.
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: 338dad1
  • Result: FAILED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: c7fb5ed
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: 338dad1
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: c7fb5ed
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-pr
  • Commit ID: c7fb5ed
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: 338dad1
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: c7fb5ed
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
laurenyu
laurenyu
reviewed
Jul 28, 2020
View reviewed changes
Comment threadsrc/sagemaker/model.py
Outdated


Comment on lines

+268
 to 
+270

 compiler_options
 if not isinstance(compiler_options, dict)
 else json.dumps(compiler_options)
@laurenyulaurenyuJul 28, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

let's flip this around to put the positive condition first, i.e. json.dumps(compiler_options) if isinstance(compiler_options, dict) else compiler_options
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

fixed here and for DataInputConfig as well.
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: fe8ba9d
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: fe8ba9d
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: fe8ba9d
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-pr
  • Commit ID: fe8ba9d
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@laurenyu
laurenyu
merged commit 20751c9
into
aws:masterJul 28, 2020
Evan-W-ang added a commit
to Evan-W-ang/sagemaker-python-sdk
that referenced
this pull request
Jun 8, 2026
@Evan-W-ang
feature: Neo: Add Granular Target Description support for compilation (…
77b6fc9
…aws#1752)
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers


2 more reviewers

@laurenyulaurenyulaurenyu approved these changes
@mchoi8739mchoi8739mchoi8739 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@apivovarov@sagemaker-bot@laurenyu@mchoi8739

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
feature: Add Granular Target Description support for compilation (Neo) - #1752
Merged
laurenyu merged 1 commit into
aws:masterfrom 
apivovarov:GTDD
Jul 28, 2020
Merged
feature: Add Granular Target Description support for compilation (Neo)#1752
laurenyu merged 1 commit into
aws:masterfrom 
apivovarov:GTDD
Conversation
@apivovarov
@apivovarovapivovarov
commented
Jul 24, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
Description
Add Granular Target Description support for DL model compilation.

Example:
 model.compile(
target_instance_family=None,
input_shape={"data": [1, 3, 1024, 1024]},
output_path="s3://output",
role="role",
framework="tensorflow",
job_name="compile-model",
target_platform_os="LINUX",
target_platform_arch="ARM64",
target_platform_accelerator="NVIDIA",
compiler_options={"gpu-code": "sm_72", "trt-ver": "6.0.1", "cuda-ver": "10.1"}
)

Tests
  •  I have added tests that prove my fix is effective or that my feature works (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
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: ffec986
  • Result: FAILED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: ffec986
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: ffec986
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-pr
  • Commit ID: ffec986
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: b42989f
  • Result: FAILED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: b42989f
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: b42989f
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 25, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: 6f5f6af
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 25, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: 6f5f6af
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 25, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-pr
  • Commit ID: 6f5f6af
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 25, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: 6f5f6af
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@laurenyulaurenyu
changed the title
neo: Add Granular Target Description support for compilationfeature: Add Granular Target Description support for compilation (Neo)Jul 27, 2020
laurenyu
laurenyu
reviewed
Jul 27, 2020
View reviewed changes
Comment threadsrc/sagemaker/estimator.py
Comment threadsrc/sagemaker/model.py
Outdated


Comment on lines

+253
 to 
+256

 assert target_platform_os is not None and target_platform_arch is not None, (
 "target_instance_type or (target_platform_os and target_platform_arch) "
 "should be provided"
 )
@laurenyulaurenyuJul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

it's better practice to avoid using asserts in source code, and instead raise a ValueError in this case
https://stackoverflow.com/a/945135
https://stackoverflow.com/a/1838411
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Replaced with ValueError
Comment threadsrc/sagemaker/model.py
Outdated

 run your model after compilation, for example: ml_c5. For allowed
 strings see
 https://docs.aws.amazon.com/sagemaker/latest/dg/API_OutputConfig.html.
 Alternatively, you can select OS, Arch and Accelerator using Target Platform
@laurenyulaurenyuJul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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


Suggested change

Alternatively, youcanselectOS, ArchandAcceleratorusingTargetPlatform
Alternatively, youcanselectanOS, Architecture,andAcceleratorusing
``target_platform_os``, ``target_platform_arch``, and``target_platform_accelerator``.
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

fixed as suggested
Comment threadsrc/sagemaker/model.py
Outdated


Comment on lines

+378
 to 
+384

 target_platform_os (str): Target Platform OS. Allowed values: 'LINUX', 'ANDROID'.
 It can be used instead of target_instance_family.
 target_platform_arch (str): Target Platform Architecture. Allowed values: 'X86_64',
 'X86', 'ARM64', 'ARM_EABIHF', 'ARM_EABI'.
 It can be used instead of target_instance_family.
 target_platform_accelerator (str): Target Platform Accelerator. Allowed values:
 'INTEL_GRAPHICS', 'MALI', 'NVIDIA'. Optional.
@laurenyulaurenyuJul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

can you instead link to docs that list the allowed versions? I assume they might change in the future, and if that happens, this docstring will inevitably be forgotten.
@mchoi8739mchoi8739Jul 27, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

The API reference is updated:
OutputConfig API
TargetPlatform API
It would be good to provide these links as well.
@apivovarovapivovarovJul 28, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I left one example value and added link to allowed values.
Comment threadsrc/sagemaker/model.py
Outdated

 It can be used instead of target_instance_family.
 target_platform_accelerator (str): Target Platform Accelerator. Allowed values:
 'INTEL_GRAPHICS', 'MALI', 'NVIDIA'. Optional.
 compiler_options (str): Additional parameters for compiler (Optional, in JSON format).
@laurenyulaurenyuJul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

if it needs to be in JSON, would it make more sense to accept a dict, and then let the Python SDK call json.dumps?
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Fixed, now it support both dict and string. Similar to DataInputConfig
Comment threadsrc/sagemaker/model.py
Outdated


Comment on lines

+451
 to 
+464

 "Devices described by Target Platform Os, Arch and Accelerator are not supported"
 "to deploy via SageMaker, please deploy the model manually."
@laurenyulaurenyuJul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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


Suggested change

"Devices described by Target Platform Os, Arch and Accelerator are not supported"
"to deploy via SageMaker, please deploy the model manually."
"Deploying devices described by Target Platform OS, Architecture, and Accelerator "
"is not supported via SageMaker. Please deploy the model manually."
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Can we keep old comment? I like it better.
@laurenyulaurenyuJul 28, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

"not supported to deploy" is the part that was bothering me. maybe "not supported for deployment"?
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

ok, changed to not supported for deployment.
Comment threadsrc/sagemaker/model.py
Outdated


Comment on lines

+445
 to 
+459

 "The instance type %s is not supported to deploy via SageMaker,"
 "please deploy the model manually.",
 target_instance_family,
@laurenyulaurenyuJul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

(I know this isn't your change, but while you're here...)

Suggested change

"The instance type %s is not supported to deploy via SageMaker,"
"please deploy the model manually.",
target_instance_family,
"Deploying instance type %s is not supported via SageMaker."
"Please deploy the model manually.",
target_instance_family,
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Can we keep old comment? I like it better.
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

changed to not supported for deployment
Comment threadtests/unit/sagemaker/model/test_neo.py
mchoi8739
mchoi8739
reviewed
Jul 27, 2020
View reviewed changes
Comment threadtests/unit/sagemaker/model/test_neo.py
 model = _create_model(sagemaker_session)
 model.compile(
 target_instance_family=None,
 input_shape={"data": [1, 3, 1024, 1024]},
@mchoi8739mchoi8739Jul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

input_shape={'data': [1, 3, 1024, 1024]}
^ suggesting to use single-quote (based on what we discussed earlier)
@apivovarovapivovarovJul 28, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

because of black-check python code style check we have to use double-quotes here
mchoi8739
mchoi8739
reviewed
Jul 27, 2020
View reviewed changes
Comment threadtests/unit/sagemaker/model/test_neo.py
 model = _create_model(sagemaker_session)
 model.compile(
 target_instance_family=None,
 input_shape={"data": [1, 3, 1024, 1024]},
@mchoi8739mchoi8739Jul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

input_shape={'data': [1, 3, 1024, 1024]}
^ suggesting to use single-quote (based on what we discussed earlier)
@apivovarovapivovarovJul 28, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

This is not JSON-like String which we use in Console UI or AWS CLI.

This is Python code. In Python SDK we use Dictionary {...} instead of JSON-string "{...}".

Technically we can use single quotes here. However, This project automatic build runs Python code style check (black-check) and it requires all strings to use double-quoutes for Strings in Python code.
mchoi8739
mchoi8739
reviewed
Jul 27, 2020
View reviewed changes
Comment threadsrc/sagemaker/model.py
Outdated

 It can be used instead of target_instance_family.
 target_platform_accelerator (str): Target Platform Accelerator. Allowed values:
 'INTEL_GRAPHICS', 'MALI', 'NVIDIA'. Optional.
 compiler_options (str): Additional parameters for compiler (Optional, in JSON format).
@mchoi8739mchoi8739Jul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

We may need to point out here as well that target_instance_family and target_platform_* are not usable together?
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

For each target_platform_* field documentation I added - It can be used instead of target_instance_family.
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: 338dad1
  • Result: FAILED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: c7fb5ed
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: 338dad1
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: c7fb5ed
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-pr
  • Commit ID: c7fb5ed
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: 338dad1
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: c7fb5ed
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
laurenyu
laurenyu
reviewed
Jul 28, 2020
View reviewed changes
Comment threadsrc/sagemaker/model.py
Outdated


Comment on lines

+268
 to 
+270

 compiler_options
 if not isinstance(compiler_options, dict)
 else json.dumps(compiler_options)
@laurenyulaurenyuJul 28, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

let's flip this around to put the positive condition first, i.e. json.dumps(compiler_options) if isinstance(compiler_options, dict) else compiler_options
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

fixed here and for DataInputConfig as well.
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: fe8ba9d
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: fe8ba9d
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: fe8ba9d
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-pr
  • Commit ID: fe8ba9d
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@laurenyu
laurenyu
merged commit 20751c9
into
aws:masterJul 28, 2020
Evan-W-ang added a commit
to Evan-W-ang/sagemaker-python-sdk
that referenced
this pull request
Jun 8, 2026
@Evan-W-ang
feature: Neo: Add Granular Target Description support for compilation (…
77b6fc9
…aws#1752)
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers


2 more reviewers

@laurenyulaurenyulaurenyu approved these changes
@mchoi8739mchoi8739mchoi8739 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@apivovarov@sagemaker-bot@laurenyu@mchoi8739

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
feature: Add Granular Target Description support for compilation (Neo) - #1752
Merged
laurenyu merged 1 commit into
aws:masterfrom 
apivovarov:GTDD
Jul 28, 2020
Merged
feature: Add Granular Target Description support for compilation (Neo)#1752
laurenyu merged 1 commit into
aws:masterfrom 
apivovarov:GTDD
Conversation
@apivovarov
@apivovarovapivovarov
commented
Jul 24, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
Description
Add Granular Target Description support for DL model compilation.

Example:
 model.compile(
target_instance_family=None,
input_shape={"data": [1, 3, 1024, 1024]},
output_path="s3://output",
role="role",
framework="tensorflow",
job_name="compile-model",
target_platform_os="LINUX",
target_platform_arch="ARM64",
target_platform_accelerator="NVIDIA",
compiler_options={"gpu-code": "sm_72", "trt-ver": "6.0.1", "cuda-ver": "10.1"}
)

Tests
  •  I have added tests that prove my fix is effective or that my feature works (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
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: ffec986
  • Result: FAILED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: ffec986
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: ffec986
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-pr
  • Commit ID: ffec986
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: b42989f
  • Result: FAILED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: b42989f
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: b42989f
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 25, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: 6f5f6af
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 25, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: 6f5f6af
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 25, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-pr
  • Commit ID: 6f5f6af
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 25, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: 6f5f6af
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@laurenyulaurenyu
changed the title
neo: Add Granular Target Description support for compilationfeature: Add Granular Target Description support for compilation (Neo)Jul 27, 2020
laurenyu
laurenyu
reviewed
Jul 27, 2020
View reviewed changes
Comment threadsrc/sagemaker/estimator.py
Comment threadsrc/sagemaker/model.py
Outdated


Comment on lines

+253
 to 
+256

 assert target_platform_os is not None and target_platform_arch is not None, (
 "target_instance_type or (target_platform_os and target_platform_arch) "
 "should be provided"
 )
@laurenyulaurenyuJul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

it's better practice to avoid using asserts in source code, and instead raise a ValueError in this case
https://stackoverflow.com/a/945135
https://stackoverflow.com/a/1838411
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Replaced with ValueError
Comment threadsrc/sagemaker/model.py
Outdated

 run your model after compilation, for example: ml_c5. For allowed
 strings see
 https://docs.aws.amazon.com/sagemaker/latest/dg/API_OutputConfig.html.
 Alternatively, you can select OS, Arch and Accelerator using Target Platform
@laurenyulaurenyuJul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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


Suggested change

Alternatively, youcanselectOS, ArchandAcceleratorusingTargetPlatform
Alternatively, youcanselectanOS, Architecture,andAcceleratorusing
``target_platform_os``, ``target_platform_arch``, and``target_platform_accelerator``.
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

fixed as suggested
Comment threadsrc/sagemaker/model.py
Outdated


Comment on lines

+378
 to 
+384

 target_platform_os (str): Target Platform OS. Allowed values: 'LINUX', 'ANDROID'.
 It can be used instead of target_instance_family.
 target_platform_arch (str): Target Platform Architecture. Allowed values: 'X86_64',
 'X86', 'ARM64', 'ARM_EABIHF', 'ARM_EABI'.
 It can be used instead of target_instance_family.
 target_platform_accelerator (str): Target Platform Accelerator. Allowed values:
 'INTEL_GRAPHICS', 'MALI', 'NVIDIA'. Optional.
@laurenyulaurenyuJul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

can you instead link to docs that list the allowed versions? I assume they might change in the future, and if that happens, this docstring will inevitably be forgotten.
@mchoi8739mchoi8739Jul 27, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

The API reference is updated:
OutputConfig API
TargetPlatform API
It would be good to provide these links as well.
@apivovarovapivovarovJul 28, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I left one example value and added link to allowed values.
Comment threadsrc/sagemaker/model.py
Outdated

 It can be used instead of target_instance_family.
 target_platform_accelerator (str): Target Platform Accelerator. Allowed values:
 'INTEL_GRAPHICS', 'MALI', 'NVIDIA'. Optional.
 compiler_options (str): Additional parameters for compiler (Optional, in JSON format).
@laurenyulaurenyuJul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

if it needs to be in JSON, would it make more sense to accept a dict, and then let the Python SDK call json.dumps?
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Fixed, now it support both dict and string. Similar to DataInputConfig
Comment threadsrc/sagemaker/model.py
Outdated


Comment on lines

+451
 to 
+464

 "Devices described by Target Platform Os, Arch and Accelerator are not supported"
 "to deploy via SageMaker, please deploy the model manually."
@laurenyulaurenyuJul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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


Suggested change

"Devices described by Target Platform Os, Arch and Accelerator are not supported"
"to deploy via SageMaker, please deploy the model manually."
"Deploying devices described by Target Platform OS, Architecture, and Accelerator "
"is not supported via SageMaker. Please deploy the model manually."
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Can we keep old comment? I like it better.
@laurenyulaurenyuJul 28, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

"not supported to deploy" is the part that was bothering me. maybe "not supported for deployment"?
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

ok, changed to not supported for deployment.
Comment threadsrc/sagemaker/model.py
Outdated


Comment on lines

+445
 to 
+459

 "The instance type %s is not supported to deploy via SageMaker,"
 "please deploy the model manually.",
 target_instance_family,
@laurenyulaurenyuJul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

(I know this isn't your change, but while you're here...)

Suggested change

"The instance type %s is not supported to deploy via SageMaker,"
"please deploy the model manually.",
target_instance_family,
"Deploying instance type %s is not supported via SageMaker."
"Please deploy the model manually.",
target_instance_family,
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Can we keep old comment? I like it better.
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

changed to not supported for deployment
Comment threadtests/unit/sagemaker/model/test_neo.py
mchoi8739
mchoi8739
reviewed
Jul 27, 2020
View reviewed changes
Comment threadtests/unit/sagemaker/model/test_neo.py
 model = _create_model(sagemaker_session)
 model.compile(
 target_instance_family=None,
 input_shape={"data": [1, 3, 1024, 1024]},
@mchoi8739mchoi8739Jul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

input_shape={'data': [1, 3, 1024, 1024]}
^ suggesting to use single-quote (based on what we discussed earlier)
@apivovarovapivovarovJul 28, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

because of black-check python code style check we have to use double-quotes here
mchoi8739
mchoi8739
reviewed
Jul 27, 2020
View reviewed changes
Comment threadtests/unit/sagemaker/model/test_neo.py
 model = _create_model(sagemaker_session)
 model.compile(
 target_instance_family=None,
 input_shape={"data": [1, 3, 1024, 1024]},
@mchoi8739mchoi8739Jul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

input_shape={'data': [1, 3, 1024, 1024]}
^ suggesting to use single-quote (based on what we discussed earlier)
@apivovarovapivovarovJul 28, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

This is not JSON-like String which we use in Console UI or AWS CLI.

This is Python code. In Python SDK we use Dictionary {...} instead of JSON-string "{...}".

Technically we can use single quotes here. However, This project automatic build runs Python code style check (black-check) and it requires all strings to use double-quoutes for Strings in Python code.
mchoi8739
mchoi8739
reviewed
Jul 27, 2020
View reviewed changes
Comment threadsrc/sagemaker/model.py
Outdated

 It can be used instead of target_instance_family.
 target_platform_accelerator (str): Target Platform Accelerator. Allowed values:
 'INTEL_GRAPHICS', 'MALI', 'NVIDIA'. Optional.
 compiler_options (str): Additional parameters for compiler (Optional, in JSON format).
@mchoi8739mchoi8739Jul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

We may need to point out here as well that target_instance_family and target_platform_* are not usable together?
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

For each target_platform_* field documentation I added - It can be used instead of target_instance_family.
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: 338dad1
  • Result: FAILED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: c7fb5ed
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: 338dad1
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: c7fb5ed
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-pr
  • Commit ID: c7fb5ed
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: 338dad1
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: c7fb5ed
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
laurenyu
laurenyu
reviewed
Jul 28, 2020
View reviewed changes
Comment threadsrc/sagemaker/model.py
Outdated


Comment on lines

+268
 to 
+270

 compiler_options
 if not isinstance(compiler_options, dict)
 else json.dumps(compiler_options)
@laurenyulaurenyuJul 28, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

let's flip this around to put the positive condition first, i.e. json.dumps(compiler_options) if isinstance(compiler_options, dict) else compiler_options
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

fixed here and for DataInputConfig as well.
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: fe8ba9d
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: fe8ba9d
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: fe8ba9d
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-pr
  • Commit ID: fe8ba9d
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@laurenyu
laurenyu
merged commit 20751c9
into
aws:masterJul 28, 2020
Evan-W-ang added a commit
to Evan-W-ang/sagemaker-python-sdk
that referenced
this pull request
Jun 8, 2026
@Evan-W-ang
feature: Neo: Add Granular Target Description support for compilation (…
77b6fc9
…aws#1752)
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers


2 more reviewers

@laurenyulaurenyulaurenyu approved these changes
@mchoi8739mchoi8739mchoi8739 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@apivovarov@sagemaker-bot@laurenyu@mchoi8739

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
feature: Add Granular Target Description support for compilation (Neo) - #1752
Merged
laurenyu merged 1 commit into
aws:masterfrom 
apivovarov:GTDD
Jul 28, 2020
Merged
feature: Add Granular Target Description support for compilation (Neo)#1752
laurenyu merged 1 commit into
aws:masterfrom 
apivovarov:GTDD
Conversation
@apivovarov
@apivovarovapivovarov
commented
Jul 24, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
Description
Add Granular Target Description support for DL model compilation.

Example:
 model.compile(
target_instance_family=None,
input_shape={"data": [1, 3, 1024, 1024]},
output_path="s3://output",
role="role",
framework="tensorflow",
job_name="compile-model",
target_platform_os="LINUX",
target_platform_arch="ARM64",
target_platform_accelerator="NVIDIA",
compiler_options={"gpu-code": "sm_72", "trt-ver": "6.0.1", "cuda-ver": "10.1"}
)

Tests
  •  I have added tests that prove my fix is effective or that my feature works (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
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: ffec986
  • Result: FAILED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: ffec986
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: ffec986
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-pr
  • Commit ID: ffec986
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: b42989f
  • Result: FAILED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: b42989f
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: b42989f
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 25, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: 6f5f6af
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 25, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: 6f5f6af
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 25, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-pr
  • Commit ID: 6f5f6af
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 25, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: 6f5f6af
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@laurenyulaurenyu
changed the title
neo: Add Granular Target Description support for compilationfeature: Add Granular Target Description support for compilation (Neo)Jul 27, 2020
laurenyu
laurenyu
reviewed
Jul 27, 2020
View reviewed changes
Comment threadsrc/sagemaker/estimator.py
Comment threadsrc/sagemaker/model.py
Outdated


Comment on lines

+253
 to 
+256

 assert target_platform_os is not None and target_platform_arch is not None, (
 "target_instance_type or (target_platform_os and target_platform_arch) "
 "should be provided"
 )
@laurenyulaurenyuJul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

it's better practice to avoid using asserts in source code, and instead raise a ValueError in this case
https://stackoverflow.com/a/945135
https://stackoverflow.com/a/1838411
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Replaced with ValueError
Comment threadsrc/sagemaker/model.py
Outdated

 run your model after compilation, for example: ml_c5. For allowed
 strings see
 https://docs.aws.amazon.com/sagemaker/latest/dg/API_OutputConfig.html.
 Alternatively, you can select OS, Arch and Accelerator using Target Platform
@laurenyulaurenyuJul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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


Suggested change

Alternatively, youcanselectOS, ArchandAcceleratorusingTargetPlatform
Alternatively, youcanselectanOS, Architecture,andAcceleratorusing
``target_platform_os``, ``target_platform_arch``, and``target_platform_accelerator``.
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

fixed as suggested
Comment threadsrc/sagemaker/model.py
Outdated


Comment on lines

+378
 to 
+384

 target_platform_os (str): Target Platform OS. Allowed values: 'LINUX', 'ANDROID'.
 It can be used instead of target_instance_family.
 target_platform_arch (str): Target Platform Architecture. Allowed values: 'X86_64',
 'X86', 'ARM64', 'ARM_EABIHF', 'ARM_EABI'.
 It can be used instead of target_instance_family.
 target_platform_accelerator (str): Target Platform Accelerator. Allowed values:
 'INTEL_GRAPHICS', 'MALI', 'NVIDIA'. Optional.
@laurenyulaurenyuJul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

can you instead link to docs that list the allowed versions? I assume they might change in the future, and if that happens, this docstring will inevitably be forgotten.
@mchoi8739mchoi8739Jul 27, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

The API reference is updated:
OutputConfig API
TargetPlatform API
It would be good to provide these links as well.
@apivovarovapivovarovJul 28, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I left one example value and added link to allowed values.
Comment threadsrc/sagemaker/model.py
Outdated

 It can be used instead of target_instance_family.
 target_platform_accelerator (str): Target Platform Accelerator. Allowed values:
 'INTEL_GRAPHICS', 'MALI', 'NVIDIA'. Optional.
 compiler_options (str): Additional parameters for compiler (Optional, in JSON format).
@laurenyulaurenyuJul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

if it needs to be in JSON, would it make more sense to accept a dict, and then let the Python SDK call json.dumps?
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Fixed, now it support both dict and string. Similar to DataInputConfig
Comment threadsrc/sagemaker/model.py
Outdated


Comment on lines

+451
 to 
+464

 "Devices described by Target Platform Os, Arch and Accelerator are not supported"
 "to deploy via SageMaker, please deploy the model manually."
@laurenyulaurenyuJul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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


Suggested change

"Devices described by Target Platform Os, Arch and Accelerator are not supported"
"to deploy via SageMaker, please deploy the model manually."
"Deploying devices described by Target Platform OS, Architecture, and Accelerator "
"is not supported via SageMaker. Please deploy the model manually."
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Can we keep old comment? I like it better.
@laurenyulaurenyuJul 28, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

"not supported to deploy" is the part that was bothering me. maybe "not supported for deployment"?
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

ok, changed to not supported for deployment.
Comment threadsrc/sagemaker/model.py
Outdated


Comment on lines

+445
 to 
+459

 "The instance type %s is not supported to deploy via SageMaker,"
 "please deploy the model manually.",
 target_instance_family,
@laurenyulaurenyuJul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

(I know this isn't your change, but while you're here...)

Suggested change

"The instance type %s is not supported to deploy via SageMaker,"
"please deploy the model manually.",
target_instance_family,
"Deploying instance type %s is not supported via SageMaker."
"Please deploy the model manually.",
target_instance_family,
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Can we keep old comment? I like it better.
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

changed to not supported for deployment
Comment threadtests/unit/sagemaker/model/test_neo.py
mchoi8739
mchoi8739
reviewed
Jul 27, 2020
View reviewed changes
Comment threadtests/unit/sagemaker/model/test_neo.py
 model = _create_model(sagemaker_session)
 model.compile(
 target_instance_family=None,
 input_shape={"data": [1, 3, 1024, 1024]},
@mchoi8739mchoi8739Jul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

input_shape={'data': [1, 3, 1024, 1024]}
^ suggesting to use single-quote (based on what we discussed earlier)
@apivovarovapivovarovJul 28, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

because of black-check python code style check we have to use double-quotes here
mchoi8739
mchoi8739
reviewed
Jul 27, 2020
View reviewed changes
Comment threadtests/unit/sagemaker/model/test_neo.py
 model = _create_model(sagemaker_session)
 model.compile(
 target_instance_family=None,
 input_shape={"data": [1, 3, 1024, 1024]},
@mchoi8739mchoi8739Jul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

input_shape={'data': [1, 3, 1024, 1024]}
^ suggesting to use single-quote (based on what we discussed earlier)
@apivovarovapivovarovJul 28, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

This is not JSON-like String which we use in Console UI or AWS CLI.

This is Python code. In Python SDK we use Dictionary {...} instead of JSON-string "{...}".

Technically we can use single quotes here. However, This project automatic build runs Python code style check (black-check) and it requires all strings to use double-quoutes for Strings in Python code.
mchoi8739
mchoi8739
reviewed
Jul 27, 2020
View reviewed changes
Comment threadsrc/sagemaker/model.py
Outdated

 It can be used instead of target_instance_family.
 target_platform_accelerator (str): Target Platform Accelerator. Allowed values:
 'INTEL_GRAPHICS', 'MALI', 'NVIDIA'. Optional.
 compiler_options (str): Additional parameters for compiler (Optional, in JSON format).
@mchoi8739mchoi8739Jul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

We may need to point out here as well that target_instance_family and target_platform_* are not usable together?
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

For each target_platform_* field documentation I added - It can be used instead of target_instance_family.
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: 338dad1
  • Result: FAILED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: c7fb5ed
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: 338dad1
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: c7fb5ed
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-pr
  • Commit ID: c7fb5ed
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: 338dad1
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: c7fb5ed
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
laurenyu
laurenyu
reviewed
Jul 28, 2020
View reviewed changes
Comment threadsrc/sagemaker/model.py
Outdated


Comment on lines

+268
 to 
+270

 compiler_options
 if not isinstance(compiler_options, dict)
 else json.dumps(compiler_options)
@laurenyulaurenyuJul 28, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

let's flip this around to put the positive condition first, i.e. json.dumps(compiler_options) if isinstance(compiler_options, dict) else compiler_options
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

fixed here and for DataInputConfig as well.
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: fe8ba9d
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: fe8ba9d
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: fe8ba9d
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-pr
  • Commit ID: fe8ba9d
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@laurenyu
laurenyu
merged commit 20751c9
into
aws:masterJul 28, 2020
Evan-W-ang added a commit
to Evan-W-ang/sagemaker-python-sdk
that referenced
this pull request
Jun 8, 2026
@Evan-W-ang
feature: Neo: Add Granular Target Description support for compilation (…
77b6fc9
…aws#1752)
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers


2 more reviewers

@laurenyulaurenyulaurenyu approved these changes
@mchoi8739mchoi8739mchoi8739 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@apivovarov@sagemaker-bot@laurenyu@mchoi8739

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
feature: Add Granular Target Description support for compilation (Neo) - #1752
Merged
laurenyu merged 1 commit into
aws:masterfrom 
apivovarov:GTDD
Jul 28, 2020
Merged
feature: Add Granular Target Description support for compilation (Neo)#1752
laurenyu merged 1 commit into
aws:masterfrom 
apivovarov:GTDD
Conversation
@apivovarov
@apivovarovapivovarov
commented
Jul 24, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
Description
Add Granular Target Description support for DL model compilation.

Example:
 model.compile(
target_instance_family=None,
input_shape={"data": [1, 3, 1024, 1024]},
output_path="s3://output",
role="role",
framework="tensorflow",
job_name="compile-model",
target_platform_os="LINUX",
target_platform_arch="ARM64",
target_platform_accelerator="NVIDIA",
compiler_options={"gpu-code": "sm_72", "trt-ver": "6.0.1", "cuda-ver": "10.1"}
)

Tests
  •  I have added tests that prove my fix is effective or that my feature works (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
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: ffec986
  • Result: FAILED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: ffec986
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: ffec986
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-pr
  • Commit ID: ffec986
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: b42989f
  • Result: FAILED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: b42989f
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: b42989f
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 25, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: 6f5f6af
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 25, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: 6f5f6af
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 25, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-pr
  • Commit ID: 6f5f6af
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 25, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: 6f5f6af
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@laurenyulaurenyu
changed the title
neo: Add Granular Target Description support for compilationfeature: Add Granular Target Description support for compilation (Neo)Jul 27, 2020
laurenyu
laurenyu
reviewed
Jul 27, 2020
View reviewed changes
Comment threadsrc/sagemaker/estimator.py
Comment threadsrc/sagemaker/model.py
Outdated


Comment on lines

+253
 to 
+256

 assert target_platform_os is not None and target_platform_arch is not None, (
 "target_instance_type or (target_platform_os and target_platform_arch) "
 "should be provided"
 )
@laurenyulaurenyuJul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

it's better practice to avoid using asserts in source code, and instead raise a ValueError in this case
https://stackoverflow.com/a/945135
https://stackoverflow.com/a/1838411
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Replaced with ValueError
Comment threadsrc/sagemaker/model.py
Outdated

 run your model after compilation, for example: ml_c5. For allowed
 strings see
 https://docs.aws.amazon.com/sagemaker/latest/dg/API_OutputConfig.html.
 Alternatively, you can select OS, Arch and Accelerator using Target Platform
@laurenyulaurenyuJul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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


Suggested change

Alternatively, youcanselectOS, ArchandAcceleratorusingTargetPlatform
Alternatively, youcanselectanOS, Architecture,andAcceleratorusing
``target_platform_os``, ``target_platform_arch``, and``target_platform_accelerator``.
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

fixed as suggested
Comment threadsrc/sagemaker/model.py
Outdated


Comment on lines

+378
 to 
+384

 target_platform_os (str): Target Platform OS. Allowed values: 'LINUX', 'ANDROID'.
 It can be used instead of target_instance_family.
 target_platform_arch (str): Target Platform Architecture. Allowed values: 'X86_64',
 'X86', 'ARM64', 'ARM_EABIHF', 'ARM_EABI'.
 It can be used instead of target_instance_family.
 target_platform_accelerator (str): Target Platform Accelerator. Allowed values:
 'INTEL_GRAPHICS', 'MALI', 'NVIDIA'. Optional.
@laurenyulaurenyuJul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

can you instead link to docs that list the allowed versions? I assume they might change in the future, and if that happens, this docstring will inevitably be forgotten.
@mchoi8739mchoi8739Jul 27, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

The API reference is updated:
OutputConfig API
TargetPlatform API
It would be good to provide these links as well.
@apivovarovapivovarovJul 28, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I left one example value and added link to allowed values.
Comment threadsrc/sagemaker/model.py
Outdated

 It can be used instead of target_instance_family.
 target_platform_accelerator (str): Target Platform Accelerator. Allowed values:
 'INTEL_GRAPHICS', 'MALI', 'NVIDIA'. Optional.
 compiler_options (str): Additional parameters for compiler (Optional, in JSON format).
@laurenyulaurenyuJul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

if it needs to be in JSON, would it make more sense to accept a dict, and then let the Python SDK call json.dumps?
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Fixed, now it support both dict and string. Similar to DataInputConfig
Comment threadsrc/sagemaker/model.py
Outdated


Comment on lines

+451
 to 
+464

 "Devices described by Target Platform Os, Arch and Accelerator are not supported"
 "to deploy via SageMaker, please deploy the model manually."
@laurenyulaurenyuJul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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


Suggested change

"Devices described by Target Platform Os, Arch and Accelerator are not supported"
"to deploy via SageMaker, please deploy the model manually."
"Deploying devices described by Target Platform OS, Architecture, and Accelerator "
"is not supported via SageMaker. Please deploy the model manually."
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Can we keep old comment? I like it better.
@laurenyulaurenyuJul 28, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

"not supported to deploy" is the part that was bothering me. maybe "not supported for deployment"?
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

ok, changed to not supported for deployment.
Comment threadsrc/sagemaker/model.py
Outdated


Comment on lines

+445
 to 
+459

 "The instance type %s is not supported to deploy via SageMaker,"
 "please deploy the model manually.",
 target_instance_family,
@laurenyulaurenyuJul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

(I know this isn't your change, but while you're here...)

Suggested change

"The instance type %s is not supported to deploy via SageMaker,"
"please deploy the model manually.",
target_instance_family,
"Deploying instance type %s is not supported via SageMaker."
"Please deploy the model manually.",
target_instance_family,
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Can we keep old comment? I like it better.
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

changed to not supported for deployment
Comment threadtests/unit/sagemaker/model/test_neo.py
mchoi8739
mchoi8739
reviewed
Jul 27, 2020
View reviewed changes
Comment threadtests/unit/sagemaker/model/test_neo.py
 model = _create_model(sagemaker_session)
 model.compile(
 target_instance_family=None,
 input_shape={"data": [1, 3, 1024, 1024]},
@mchoi8739mchoi8739Jul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

input_shape={'data': [1, 3, 1024, 1024]}
^ suggesting to use single-quote (based on what we discussed earlier)
@apivovarovapivovarovJul 28, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

because of black-check python code style check we have to use double-quotes here
mchoi8739
mchoi8739
reviewed
Jul 27, 2020
View reviewed changes
Comment threadtests/unit/sagemaker/model/test_neo.py
 model = _create_model(sagemaker_session)
 model.compile(
 target_instance_family=None,
 input_shape={"data": [1, 3, 1024, 1024]},
@mchoi8739mchoi8739Jul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

input_shape={'data': [1, 3, 1024, 1024]}
^ suggesting to use single-quote (based on what we discussed earlier)
@apivovarovapivovarovJul 28, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

This is not JSON-like String which we use in Console UI or AWS CLI.

This is Python code. In Python SDK we use Dictionary {...} instead of JSON-string "{...}".

Technically we can use single quotes here. However, This project automatic build runs Python code style check (black-check) and it requires all strings to use double-quoutes for Strings in Python code.
mchoi8739
mchoi8739
reviewed
Jul 27, 2020
View reviewed changes
Comment threadsrc/sagemaker/model.py
Outdated

 It can be used instead of target_instance_family.
 target_platform_accelerator (str): Target Platform Accelerator. Allowed values:
 'INTEL_GRAPHICS', 'MALI', 'NVIDIA'. Optional.
 compiler_options (str): Additional parameters for compiler (Optional, in JSON format).
@mchoi8739mchoi8739Jul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

We may need to point out here as well that target_instance_family and target_platform_* are not usable together?
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

For each target_platform_* field documentation I added - It can be used instead of target_instance_family.
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: 338dad1
  • Result: FAILED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: c7fb5ed
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: 338dad1
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: c7fb5ed
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-pr
  • Commit ID: c7fb5ed
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: 338dad1
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: c7fb5ed
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
laurenyu
laurenyu
reviewed
Jul 28, 2020
View reviewed changes
Comment threadsrc/sagemaker/model.py
Outdated


Comment on lines

+268
 to 
+270

 compiler_options
 if not isinstance(compiler_options, dict)
 else json.dumps(compiler_options)
@laurenyulaurenyuJul 28, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

let's flip this around to put the positive condition first, i.e. json.dumps(compiler_options) if isinstance(compiler_options, dict) else compiler_options
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

fixed here and for DataInputConfig as well.
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: fe8ba9d
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: fe8ba9d
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: fe8ba9d
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-pr
  • Commit ID: fe8ba9d
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@laurenyu
laurenyu
merged commit 20751c9
into
aws:masterJul 28, 2020
Evan-W-ang added a commit
to Evan-W-ang/sagemaker-python-sdk
that referenced
this pull request
Jun 8, 2026
@Evan-W-ang
feature: Neo: Add Granular Target Description support for compilation (…
77b6fc9
…aws#1752)
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers


2 more reviewers

@laurenyulaurenyulaurenyu approved these changes
@mchoi8739mchoi8739mchoi8739 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@apivovarov@sagemaker-bot@laurenyu@mchoi8739

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
feature: Add Granular Target Description support for compilation (Neo) - #1752
Merged
laurenyu merged 1 commit into
aws:masterfrom 
apivovarov:GTDD
Jul 28, 2020
Merged
feature: Add Granular Target Description support for compilation (Neo)#1752
laurenyu merged 1 commit into
aws:masterfrom 
apivovarov:GTDD
Conversation
@apivovarov
@apivovarovapivovarov
commented
Jul 24, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
Description
Add Granular Target Description support for DL model compilation.

Example:
 model.compile(
target_instance_family=None,
input_shape={"data": [1, 3, 1024, 1024]},
output_path="s3://output",
role="role",
framework="tensorflow",
job_name="compile-model",
target_platform_os="LINUX",
target_platform_arch="ARM64",
target_platform_accelerator="NVIDIA",
compiler_options={"gpu-code": "sm_72", "trt-ver": "6.0.1", "cuda-ver": "10.1"}
)

Tests
  •  I have added tests that prove my fix is effective or that my feature works (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
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: ffec986
  • Result: FAILED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: ffec986
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: ffec986
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-pr
  • Commit ID: ffec986
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: b42989f
  • Result: FAILED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: b42989f
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 24, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: b42989f
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 25, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: 6f5f6af
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 25, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: 6f5f6af
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 25, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-pr
  • Commit ID: 6f5f6af
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 25, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: 6f5f6af
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@laurenyulaurenyu
changed the title
neo: Add Granular Target Description support for compilationfeature: Add Granular Target Description support for compilation (Neo)Jul 27, 2020
laurenyu
laurenyu
reviewed
Jul 27, 2020
View reviewed changes
Comment threadsrc/sagemaker/estimator.py
Comment threadsrc/sagemaker/model.py
Outdated


Comment on lines

+253
 to 
+256

 assert target_platform_os is not None and target_platform_arch is not None, (
 "target_instance_type or (target_platform_os and target_platform_arch) "
 "should be provided"
 )
@laurenyulaurenyuJul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

it's better practice to avoid using asserts in source code, and instead raise a ValueError in this case
https://stackoverflow.com/a/945135
https://stackoverflow.com/a/1838411
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Replaced with ValueError
Comment threadsrc/sagemaker/model.py
Outdated

 run your model after compilation, for example: ml_c5. For allowed
 strings see
 https://docs.aws.amazon.com/sagemaker/latest/dg/API_OutputConfig.html.
 Alternatively, you can select OS, Arch and Accelerator using Target Platform
@laurenyulaurenyuJul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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


Suggested change

Alternatively, youcanselectOS, ArchandAcceleratorusingTargetPlatform
Alternatively, youcanselectanOS, Architecture,andAcceleratorusing
``target_platform_os``, ``target_platform_arch``, and``target_platform_accelerator``.
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

fixed as suggested
Comment threadsrc/sagemaker/model.py
Outdated


Comment on lines

+378
 to 
+384

 target_platform_os (str): Target Platform OS. Allowed values: 'LINUX', 'ANDROID'.
 It can be used instead of target_instance_family.
 target_platform_arch (str): Target Platform Architecture. Allowed values: 'X86_64',
 'X86', 'ARM64', 'ARM_EABIHF', 'ARM_EABI'.
 It can be used instead of target_instance_family.
 target_platform_accelerator (str): Target Platform Accelerator. Allowed values:
 'INTEL_GRAPHICS', 'MALI', 'NVIDIA'. Optional.
@laurenyulaurenyuJul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

can you instead link to docs that list the allowed versions? I assume they might change in the future, and if that happens, this docstring will inevitably be forgotten.
@mchoi8739mchoi8739Jul 27, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

The API reference is updated:
OutputConfig API
TargetPlatform API
It would be good to provide these links as well.
@apivovarovapivovarovJul 28, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I left one example value and added link to allowed values.
Comment threadsrc/sagemaker/model.py
Outdated

 It can be used instead of target_instance_family.
 target_platform_accelerator (str): Target Platform Accelerator. Allowed values:
 'INTEL_GRAPHICS', 'MALI', 'NVIDIA'. Optional.
 compiler_options (str): Additional parameters for compiler (Optional, in JSON format).
@laurenyulaurenyuJul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

if it needs to be in JSON, would it make more sense to accept a dict, and then let the Python SDK call json.dumps?
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Fixed, now it support both dict and string. Similar to DataInputConfig
Comment threadsrc/sagemaker/model.py
Outdated


Comment on lines

+451
 to 
+464

 "Devices described by Target Platform Os, Arch and Accelerator are not supported"
 "to deploy via SageMaker, please deploy the model manually."
@laurenyulaurenyuJul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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


Suggested change

"Devices described by Target Platform Os, Arch and Accelerator are not supported"
"to deploy via SageMaker, please deploy the model manually."
"Deploying devices described by Target Platform OS, Architecture, and Accelerator "
"is not supported via SageMaker. Please deploy the model manually."
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Can we keep old comment? I like it better.
@laurenyulaurenyuJul 28, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

"not supported to deploy" is the part that was bothering me. maybe "not supported for deployment"?
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

ok, changed to not supported for deployment.
Comment threadsrc/sagemaker/model.py
Outdated


Comment on lines

+445
 to 
+459

 "The instance type %s is not supported to deploy via SageMaker,"
 "please deploy the model manually.",
 target_instance_family,
@laurenyulaurenyuJul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

(I know this isn't your change, but while you're here...)

Suggested change

"The instance type %s is not supported to deploy via SageMaker,"
"please deploy the model manually.",
target_instance_family,
"Deploying instance type %s is not supported via SageMaker."
"Please deploy the model manually.",
target_instance_family,
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Can we keep old comment? I like it better.
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

changed to not supported for deployment
Comment threadtests/unit/sagemaker/model/test_neo.py
mchoi8739
mchoi8739
reviewed
Jul 27, 2020
View reviewed changes
Comment threadtests/unit/sagemaker/model/test_neo.py
 model = _create_model(sagemaker_session)
 model.compile(
 target_instance_family=None,
 input_shape={"data": [1, 3, 1024, 1024]},
@mchoi8739mchoi8739Jul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

input_shape={'data': [1, 3, 1024, 1024]}
^ suggesting to use single-quote (based on what we discussed earlier)
@apivovarovapivovarovJul 28, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

because of black-check python code style check we have to use double-quotes here
mchoi8739
mchoi8739
reviewed
Jul 27, 2020
View reviewed changes
Comment threadtests/unit/sagemaker/model/test_neo.py
 model = _create_model(sagemaker_session)
 model.compile(
 target_instance_family=None,
 input_shape={"data": [1, 3, 1024, 1024]},
@mchoi8739mchoi8739Jul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

input_shape={'data': [1, 3, 1024, 1024]}
^ suggesting to use single-quote (based on what we discussed earlier)
@apivovarovapivovarovJul 28, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

This is not JSON-like String which we use in Console UI or AWS CLI.

This is Python code. In Python SDK we use Dictionary {...} instead of JSON-string "{...}".

Technically we can use single quotes here. However, This project automatic build runs Python code style check (black-check) and it requires all strings to use double-quoutes for Strings in Python code.
mchoi8739
mchoi8739
reviewed
Jul 27, 2020
View reviewed changes
Comment threadsrc/sagemaker/model.py
Outdated

 It can be used instead of target_instance_family.
 target_platform_accelerator (str): Target Platform Accelerator. Allowed values:
 'INTEL_GRAPHICS', 'MALI', 'NVIDIA'. Optional.
 compiler_options (str): Additional parameters for compiler (Optional, in JSON format).
@mchoi8739mchoi8739Jul 27, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

We may need to point out here as well that target_instance_family and target_platform_* are not usable together?
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

For each target_platform_* field documentation I added - It can be used instead of target_instance_family.
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: 338dad1
  • Result: FAILED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: c7fb5ed
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: 338dad1
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: c7fb5ed
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-pr
  • Commit ID: c7fb5ed
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: 338dad1
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: c7fb5ed
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
laurenyu
laurenyu
reviewed
Jul 28, 2020
View reviewed changes
Comment threadsrc/sagemaker/model.py
Outdated


Comment on lines

+268
 to 
+270

 compiler_options
 if not isinstance(compiler_options, dict)
 else json.dumps(compiler_options)
@laurenyulaurenyuJul 28, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

let's flip this around to put the positive condition first, i.e. json.dumps(compiler_options) if isinstance(compiler_options, dict) else compiler_options
@apivovarovapivovarovJul 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

fixed here and for DataInputConfig as well.
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-unit-tests
  • Commit ID: fe8ba9d
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-notebook-tests
  • Commit ID: fe8ba9d
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-local-mode-tests
  • Commit ID: fe8ba9d
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@sagemaker-bot
sagemaker-bot
commented
Jul 28, 2020

Copy link


Copy Markdown

Collaborator
AWS CodeBuild CI Report
  • CodeBuild project: sagemaker-python-sdk-pr
  • Commit ID: fe8ba9d
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
@laurenyu
laurenyu
merged commit 20751c9
into
aws:masterJul 28, 2020
Evan-W-ang added a commit
to Evan-W-ang/sagemaker-python-sdk
that referenced
this pull request
Jun 8, 2026
@Evan-W-ang
feature: Neo: Add Granular Target Description support for compilation (…
77b6fc9
…aws#1752)
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers


2 more reviewers

@laurenyulaurenyulaurenyu approved these changes
@mchoi8739mchoi8739mchoi8739 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@apivovarov@sagemaker-bot@laurenyu@mchoi8739