[feature][tensorflow] Add support for DIY and SageMaker containers #1278
Conversation
tensorflow/training/docker/2.5/py3/cu112/Dockerfile.sagemaker.gpu
Outdated
Show resolved
Hide resolved
|
|
||
| RUN HOME_DIR=/root \ | ||
| && curl -o ${HOME_DIR}/oss_compliance.zip https://aws-dlinfra-utilities.s3.amazonaws.com/oss_compliance.zip \ | ||
| && unzip ${HOME_DIR}/oss_compliance.zip -d ${HOME_DIR}/ \ |
There was a problem hiding this comment.
We should delete the existing oss licenses coming from base image. Either delete or replace them entirely.
There was a problem hiding this comment.
Can you provide more context on how this would be done?
...dlc_tests/benchmark/sagemaker/tensorflow/training/test_performance_tensorflow_sm_training.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Lets add sagemaker and sagemaker_only markers on Pytest Marker Checklist.
src/image.py
Outdated
| return bool(self.info.get('base_image_uri')) | ||
|
|
||
| @property | ||
| def is_test_enabled(self): |
There was a problem hiding this comment.
nit: Can we add a docstring for this function as well?
| @@ -424,7 +435,7 @@ def test_cuda_paths(gpu): | |||
| python_version = re.search(r"(py\d+)", image).group(1) | |||
| short_python_version = None | |||
| image_tag = re.search( | |||
| r":(\d+(\.\d+){2}(-transformers\d+(\.\d+){2})?-(cpu|gpu|neuron)-(py\d+)(-cu\d+)-(ubuntu\d+\.\d+)(-example)?)", | |||
| r":(\d+(\.\d+){2}(-transformers\d+(\.\d+){2})?-(cpu|gpu|neuron)-(py\d+)(-cu\d+)-(ubuntu\d+\.\d+)(-example|-diy|-sagemaker)?)", | |||
There was a problem hiding this comment.
nit: regex string can be reformatted
| @@ -35,6 +31,7 @@ def test_awscli(mxnet_inference): | |||
| test_utils.run_cmd_on_container(container_name, ctx, "aws --version") | |||
|
|
|||
|
|
|||
| @pytest.mark.usefixtures("sagemaker") | |||
There was a problem hiding this comment.
This test should only be ran on SM images. The packages are SM utility packages added soley for SM.
Let revamp the test to reflect the same.
There was a problem hiding this comment.
Sure - if you look below, I made sure that non sm* utility packages are installed in DIY - but if this only needs to be a requirement for sagemaker, then we can remove DIY and make this sm only
There was a problem hiding this comment.
All the below packages are SM utility packages and should exist only on SM DLC image.
"bokeh", "imageio", "plotly", "seaborn", "shap", "pandas", "cv2", "sagemaker"
There was a problem hiding this comment.
I think since these are open source packages not explicitly marked as sagemaker, we can also have these in the DIY image (minus the "sagemaker") dependency. Do you see any reason for moving them out of the DIY?
GitHub Issue #, if available:
Note: If merging this PR should also close the associated Issue, please also add that Issue # to the Linked Issues section on the right.
Description
Tests run
sagemaker_onlysagemakerorsagemaker_onlyDLC image/dockerfile
Additional context
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.