-
Notifications
You must be signed in to change notification settings - Fork 504
[TensorFlow][Training][Sagemaker] TensorFlow 2.19.0 Currency Release #4789
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
base: master
Are you sure you want to change the base?
Conversation
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.
Overall great job on your first PRs. A few nit organizations here and there. I would retest the images especially EC2 image since the tag would've been wrong in your testing so we should figure out why the EC2 image passed your test even with the wrong image tag.
@@ -41,6 +41,7 @@ def _skip_if_image_is_not_compatible_with_smppy(image_uri): | |||
@pytest.mark.model("mnist") | |||
@pytest.mark.skip_cpu | |||
@pytest.mark.skip_py2_containers | |||
@pytest.mark.skip(reason="SageMaker Profiler binary is not installed") |
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.
This will skip ALL TF images. Are we intending to skip both TF 2.18 and this new TF 2.19?
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.
Oh i see, sagemaker profiler binary is not available for the new TF version. so probably we should skip it for only TF2.19. I will modify the code to only skip TF2.19 as
@pytest.mark.processor("gpu")
@pytest.mark.integration("smppy")
@pytest.mark.model("mnist")
@pytest.mark.multinode(2)
@pytest.mark.skip_cpu
@pytest.mark.skip_py2_containers
def test_training_smppy_multinode(ecr_image, sagemaker_regions, py_version, tmpdir):
if "tensorflow-2.19" in ecr_image:
pytest.skip("SageMaker Profiler binary is not installed in TF 2.19")
_skip_if_image_is_not_compatible_with_smppy(ecr_image)
invoke_sm_helper_function(ecr_image, sagemaker_regions, _test_smppy_mnist_multinode_function)
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.
Please take a look at how we do version checks using SpecifierSet
to check for version in ecr image.
if "tensorflow-2.19" in ecr_image
is bad practice and many things can go wrong during the version checks if we update the image for various reasons.
test/sagemaker_tests/tensorflow/tensorflow2_training/integration/local/test_training.py
Outdated
Show resolved
Hide resolved
@@ -52,6 +52,7 @@ def _skip_if_image_is_not_compatible_with_smppy(image_uri): | |||
@pytest.mark.model("mnist") | |||
@pytest.mark.skip_cpu | |||
@pytest.mark.skip_py2_containers | |||
@pytest.mark.skip(reason="SageMaker Profiler binary is not installed") |
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.
ditto
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 modify code to skip only TF2.19
@@ -109,6 +110,7 @@ def _test_smppy_mnist_function(ecr_image, sagemaker_session): | |||
@pytest.mark.multinode(2) | |||
@pytest.mark.skip_cpu | |||
@pytest.mark.skip_py2_containers | |||
@pytest.mark.skip(reason="SageMaker Profiler binary is not installed") |
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.
ditto
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 modify code to skip only TF2.19
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.
All PR's are checked weekly for staleness. This PR will be closed if not updated in 30 days.
Description
Tests run
NOTE: By default, docker builds are disabled. In order to build your container, please update dlc_developer_config.toml and specify the framework to build in "build_frameworks"
Confused on how to run tests? Try using the helper utility...
Assuming your remote is called
origin
(you can find out more withgit remote -v
)...python src/prepare_dlc_dev_environment.py -b </path/to/buildspec.yml> -cp origin
python src/prepare_dlc_dev_environment.py -b </path/to/buildspec.yml> -t sanity_tests -cp origin
python src/prepare_dlc_dev_environment.py -rcp origin
NOTE: If you are creating a PR for a new framework version, please ensure success of the standard, rc, and efa sagemaker remote tests by updating the dlc_developer_config.toml file:
Expand
sagemaker_remote_tests = true
sagemaker_efa_tests = true
sagemaker_rc_tests = true
Additionally, please run the sagemaker local tests in at least one revision:
sagemaker_local_tests = true
Formatting
black -l 100
on my code (formatting tool: https://black.readthedocs.io/en/stable/getting_started.html)DLC image/dockerfile
Builds to Execute
Expand
Fill out the template and click the checkbox of the builds you'd like to execute
Note: Replace with <X.Y> with the major.minor framework version (i.e. 2.2) you would like to start.
build_pytorch_training_<X.Y>_sm
build_pytorch_training_<X.Y>_ec2
build_pytorch_inference_<X.Y>_sm
build_pytorch_inference_<X.Y>_ec2
build_pytorch_inference_<X.Y>_graviton
build_tensorflow_training_<X.Y>_sm
build_tensorflow_training_<X.Y>_ec2
build_tensorflow_inference_<X.Y>_sm
build_tensorflow_inference_<X.Y>_ec2
build_tensorflow_inference_<X.Y>_graviton
Additional context
PR Checklist
Expand
NEURON/GRAVITON Testing Checklist
dlc_developer_config.toml
in my PR branch by settingneuron_mode = true
orgraviton_mode = true
Benchmark Testing Checklist
dlc_developer_config.toml
in my PR branch by settingec2_benchmark_tests = true
orsagemaker_benchmark_tests = true
Pytest Marker Checklist
Expand
@pytest.mark.model("<model-type>")
to the new tests which I have added, to specify the Deep Learning model that is used in the test (use"N/A"
if the test doesn't use a model)@pytest.mark.integration("<feature-being-tested>")
to the new tests which I have added, to specify the feature that will be tested@pytest.mark.multinode(<integer-num-nodes>)
to the new tests which I have added, to specify the number of nodes used on a multi-node test@pytest.mark.processor(<"cpu"/"gpu"/"eia"/"neuron">)
to the new tests which I have added, if a test is specifically applicable to only one processor typeBy 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.