Skip to content

Adding multi arch buildspec for Codebuild setup #132

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

Merged
merged 1 commit into from
Oct 5, 2022
Merged

Conversation

NikhilRaverkar
Copy link
Contributor

@NikhilRaverkar NikhilRaverkar commented Sep 7, 2022

Issue #, if available:
To support graviton codebuild, we need graviton specific build specs.
Description of changes:

  • Added buildspecs for Graviton images.

@NikhilRaverkar NikhilRaverkar marked this pull request as ready for review September 8, 2022 18:05
@NikhilRaverkar NikhilRaverkar force-pushed the aarch64_test branch 3 times, most recently from baa7052 to 18bf14e Compare September 9, 2022 16:33
Copy link
Contributor

@mabunday mabunday left a comment

Choose a reason for hiding this comment

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

What's the test/resources/module/user_code.tar.gz for?

Comment on lines 36 to 37
- docker run --rm -t test-sklearn sh -c 'pytest --cov=sagemaker_sklearn_container --cov-fail-under=60 test/unit'
- docker run --rm -t test-sklearn sh -c 'flake8 setup.py src test'
Copy link
Contributor

Choose a reason for hiding this comment

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

This is minor and I know this is carryover from the other buildspecs, but we aren't actually running tox here.

Suggested change
- docker run --rm -t test-sklearn sh -c 'pytest --cov=sagemaker_sklearn_container --cov-fail-under=60 test/unit'
- docker run --rm -t test-sklearn sh -c 'flake8 setup.py src test'
- docker run --rm -t test-sklearn sh -c 'tox -e ALL'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

test/resources/module/user_code.tar.gz is used by the integ-tests

commands:
- echo Build completed on `date`
- echo Pushing the Docker image...
- docker push 515193369038.dkr.ecr.us-west-2.amazonaws.com/sagemaker-scikit-learn:$FRAMEWORK_VERSION-arm-cpu-py3
Copy link
Contributor

@amzn-choeric amzn-choeric Oct 3, 2022

Choose a reason for hiding this comment

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

Please change this to "arm64" to be consistent with XGBoost or the other way around. What it actually is (arm, arm64, etc.) does not really matter to me, but I think consistency across images would be good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

made it consistent with XGBoost (arm64)

@amzn-choeric amzn-choeric merged commit 8600e2a into master Oct 5, 2022
@NikhilRaverkar NikhilRaverkar deleted the aarch64_test branch October 5, 2022 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants