-
Notifications
You must be signed in to change notification settings - Fork 162
Add release build #191
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add release build #191
Conversation
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
buildspec-release.yml
Outdated
- tox -e flake8 | ||
|
||
# run unit tests | ||
- tox -e py36 test/unit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no Python 2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will add
buildspec.yml
Outdated
@@ -97,15 +97,15 @@ phases: | |||
# run cpu sagemaker tests | |||
- | | |||
if has-matching-changes "test/" "tests/" "src/*.py" "docker/*" "buildspec.yml"; then | |||
pytest test/integration/sagemaker --region $AWS_DEFAULT_REGION --docker-base-name $ECR_REPO --account-id $ACCOUNT --tag $CPU_TAG --py-version $CPU_PY_VERSION --instance-type $CPU_INSTANCE_TYPE | |||
pytest test/integration/sagemaker -n 8 --region $AWS_DEFAULT_REGION --docker-base-name $ECR_REPO --account-id $ACCOUNT --tag $CPU_TAG --py-version $CPU_PY_VERSION --instance-type $CPU_INSTANCE_TYPE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why 8? (would auto
make more sense?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
k
scripts/build_all.py
Outdated
'py2-gpu': args.py2_gpu_binary, | ||
'py3-gpu': args.py3_gpu_binary | ||
} | ||
build_dir = 'docker/{}'.format(args.version) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
os.path
? same for l. 66
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
k
scripts/build_all.py
Outdated
# Run docker-login so we can pull the cached image | ||
login_cmd = subprocess.check_output( | ||
'aws ecr get-login --no-include-email --registry-id {}'.format(args.account).split()) | ||
print(login_cmd) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd preface each command with a description of what is being printed out. same goes for ll. 62 and 69 and similar lines in the other file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
k
scripts/build_all.py
Outdated
'-t {}:{} {}'.format(build_dir, arch, prev_image_uri, py_version, binary_file, args.repo, tag, build_dir) | ||
print(build_cmd) | ||
subprocess.check_call(build_cmd.split()) | ||
print('Deleting binary file') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd add the binary file name to this print statement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
k
scripts/publish_all.py
Outdated
print(tag_cmd) | ||
subprocess.check_call(tag_cmd.split()) | ||
login_cmd = subprocess.check_output( | ||
'aws ecr get-login --no-include-email --registry-id {}'.format(args.account).split()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add the region to this command
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will add.
tox.ini
Outdated
@@ -25,6 +25,7 @@ exclude = | |||
venv/ | |||
sagemaker-tensorflow-extensions | |||
benchmarks/ | |||
scripts/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why exclude these? there are Python files written by us in that directory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will add
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
* Run SM tests in parallel in PR build * Add release build spec and build scripts
* Run SM tests in parallel in PR build * Add release build spec and build scripts
* Run SM tests in parallel in PR build * Add release build spec and build scripts
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.