breaking: deprecate constants from defaults (#1590)

metrizable · web-flow · commit a83bed565614 · 2020-06-15T11:53:21.000-07:00
diff --git a/src/sagemaker/chainer/defaults.py b/src/sagemaker/chainer/defaults.py
@@ -13,12 +13,4 @@
 """Placeholder docstring"""
 from __future__ import absolute_import
 
-CHAINER_VERSION = "4.1.0"
-"""Default Chainer version for when the framework version is not specified.
-This is no longer updated so as to not break existing workflows.
-"""
-
-LATEST_VERSION = "5.0.0"
-"""The latest version of Chainer included in the SageMaker pre-built Docker images."""
-
 LATEST_PY2_VERSION = "5.0.0"
diff --git a/src/sagemaker/chainer/estimator.py b/src/sagemaker/chainer/estimator.py
@@ -40,8 +40,6 @@ class Chainer(Framework):
     _process_slots_per_host = "sagemaker_process_slots_per_host"
     _additional_mpi_options = "sagemaker_additional_mpi_options"
 
-    LATEST_VERSION = defaults.LATEST_VERSION
-
     def __init__(
         self,
         entry_point,
diff --git a/src/sagemaker/cli/mxnet.py b/src/sagemaker/cli/mxnet.py
@@ -14,7 +14,8 @@
 from __future__ import absolute_import
 
 from sagemaker.cli.common import HostCommand, TrainCommand
-from sagemaker.mxnet import defaults
+
+MXNET_VERSION = "1.2"
 
 
 def train(args):
@@ -42,7 +43,7 @@ def create_estimator(self):
 
         return MXNet(
             entry_point=self.script,
-            framework_version=defaults.MXNET_VERSION,
+            framework_version=MXNET_VERSION,
             py_version=self.python,
             role=self.role_name,
             base_job_name=self.job_name,
@@ -66,7 +67,7 @@ def create_model(self, model_url):
             model_data=model_url,
             role=self.role_name,
             entry_point=self.script,
-            framework_version=defaults.MXNET_VERSION,
+            framework_version=MXNET_VERSION,
             py_version=self.python,
             name=self.endpoint_name,
             env=self.environment,
diff --git a/src/sagemaker/mxnet/defaults.py b/src/sagemaker/mxnet/defaults.py
@@ -13,12 +13,4 @@
 """Placeholder docstring"""
 from __future__ import absolute_import
 
-MXNET_VERSION = "1.2"
-"""Default MXNet version for when the framework version is not specified.
-This is no longer updated so as to not break existing workflows.
-"""
-
-LATEST_VERSION = "1.6.0"
-"""The latest version of MXNet included in the SageMaker pre-built Docker images."""
-
 LATEST_PY2_VERSION = "1.6.0"
diff --git a/src/sagemaker/mxnet/estimator.py b/src/sagemaker/mxnet/estimator.py
@@ -38,8 +38,6 @@ class MXNet(Framework):
     __framework_name__ = "mxnet"
     _LOWEST_SCRIPT_MODE_VERSION = ["1", "3"]
 
-    LATEST_VERSION = defaults.LATEST_VERSION
-
     def __init__(
         self,
         entry_point,
@@ -115,7 +113,7 @@ def __init__(
             :class:`~sagemaker.estimator.EstimatorBase`.
         """
         validate_version_or_image_args(framework_version, py_version, image_name)
-        if py_version and py_version == "py2":
+        if py_version == "py2":
             logger.warning(
                 python_deprecation_warning(self.__framework_name__, defaults.LATEST_PY2_VERSION)
             )
diff --git a/src/sagemaker/mxnet/model.py b/src/sagemaker/mxnet/model.py
@@ -115,7 +115,7 @@ def __init__(
             :class:`~sagemaker.model.Model`.
         """
         validate_version_or_image_args(framework_version, py_version, image)
-        if py_version and py_version == "py2":
+        if py_version == "py2":
             logger.warning(
                 python_deprecation_warning(self.__framework_name__, defaults.LATEST_PY2_VERSION)
             )
diff --git a/src/sagemaker/pytorch/defaults.py b/src/sagemaker/pytorch/defaults.py
@@ -13,14 +13,4 @@
 """Placeholder docstring"""
 from __future__ import absolute_import
 
-PYTORCH_VERSION = "0.4"
-"""Default PyTorch version for when the framework version is not specified.
-The default version is no longer updated so as to not break existing workflows.
-"""
-
-LATEST_VERSION = "1.5.0"
-"""The latest version of PyTorch included in the SageMaker pre-built Docker images."""
-
-PYTHON_VERSION = "py3"
-
 LATEST_PY2_VERSION = "1.3.1"
diff --git a/src/sagemaker/pytorch/estimator.py b/src/sagemaker/pytorch/estimator.py
@@ -35,8 +35,6 @@ class PyTorch(Framework):
 
     __framework_name__ = "pytorch"
 
-    LATEST_VERSION = defaults.LATEST_VERSION
-
     def __init__(
         self,
         entry_point,
diff --git a/src/sagemaker/pytorch/model.py b/src/sagemaker/pytorch/model.py
@@ -114,7 +114,7 @@ def __init__(
             :class:`~sagemaker.model.Model`.
         """
         validate_version_or_image_args(framework_version, py_version, image)
-        if py_version and py_version == "py2":
+        if py_version == "py2":
             logger.warning(
                 python_deprecation_warning(self.__framework_name__, defaults.LATEST_PY2_VERSION)
             )
diff --git a/src/sagemaker/sklearn/defaults.py b/src/sagemaker/sklearn/defaults.py
@@ -14,7 +14,3 @@
 from __future__ import absolute_import
 
 SKLEARN_NAME = "scikit-learn"
-
-SKLEARN_VERSION = "0.20.0"
-
-LATEST_PY2_VERSION = "0.20.0"
diff --git a/src/sagemaker/tensorflow/defaults.py b/src/sagemaker/tensorflow/defaults.py
@@ -13,15 +13,4 @@
 """Placeholder docstring"""
 from __future__ import absolute_import
 
-TF_VERSION = "1.11"
-"""Default TF version for when the framework version is not specified.
-This is no longer updated so as to not break existing workflows.
-"""
-
-LATEST_VERSION = "2.2.0"
-"""The latest version of TensorFlow included in the SageMaker pre-built Docker images."""
-
-LATEST_SERVING_VERSION = "2.1.0"
-"""The latest version of TensorFlow Serving included in the SageMaker pre-built Docker images."""
-
 LATEST_PY2_VERSION = "2.1.0"
diff --git a/src/sagemaker/tensorflow/estimator.py b/src/sagemaker/tensorflow/estimator.py
@@ -36,10 +36,6 @@ class TensorFlow(Framework):
     __framework_name__ = "tensorflow"
     _ECR_REPO_NAME = "tensorflow-scriptmode"
 
-    LATEST_VERSION = defaults.LATEST_VERSION
-
-    _LATEST_1X_VERSION = "1.15.2"
-
     _HIGHEST_LEGACY_MODE_ONLY_VERSION = version.Version("1.10.0")
     _HIGHEST_PYTHON_2_VERSION = version.Version("2.1.0")
 
diff --git a/src/sagemaker/xgboost/defaults.py b/src/sagemaker/xgboost/defaults.py
@@ -15,14 +15,19 @@
 
 XGBOOST_NAME = "xgboost"
 XGBOOST_1P_VERSIONS = ["1", "latest"]
-XGBOOST_VERSION_0_90 = "0.90"
+
 XGBOOST_VERSION_0_90_1 = "0.90-1"
 XGBOOST_VERSION_0_90_2 = "0.90-2"
+
 XGBOOST_LATEST_VERSION = "1.0-1"
+
 # XGBOOST_SUPPORTED_VERSIONS has XGBoost Framework versions sorted from oldest to latest
 XGBOOST_SUPPORTED_VERSIONS = [
     XGBOOST_VERSION_0_90_1,
     XGBOOST_VERSION_0_90_2,
     XGBOOST_LATEST_VERSION,
 ]
+
+# TODO: evaluate use of this constant. it's used in precisely one place in different a module
+# may possibly be unnecessary indirection
 XGBOOST_VERSION_EQUIVALENTS = ["-cpu-py3"]
diff --git a/tests/conftest.py b/tests/conftest.py
@@ -21,14 +21,8 @@
 from botocore.config import Config
 
 from sagemaker import Session, utils
-from sagemaker.chainer import Chainer
 from sagemaker.local import LocalSession
-from sagemaker.mxnet import MXNet
-from sagemaker.pytorch import PyTorch
 from sagemaker.rl import RLEstimator
-from sagemaker.sklearn.defaults import SKLEARN_VERSION
-from sagemaker.tensorflow import TensorFlow
-from sagemaker.tensorflow.defaults import LATEST_VERSION, LATEST_SERVING_VERSION
 
 DEFAULT_REGION = "us-west-2"
 CUSTOM_BUCKET_NAME_PREFIX = "sagemaker-custom-bucket"
@@ -49,10 +43,10 @@ def pytest_addoption(parser):
     parser.addoption("--sagemaker-client-config", action="store", default=None)
     parser.addoption("--sagemaker-runtime-config", action="store", default=None)
     parser.addoption("--boto-config", action="store", default=None)
-    parser.addoption("--chainer-full-version", action="store", default=Chainer.LATEST_VERSION)
-    parser.addoption("--mxnet-full-version", action="store", default=MXNet.LATEST_VERSION)
+    parser.addoption("--chainer-full-version", action="store", default="5.0.0")
+    parser.addoption("--mxnet-full-version", action="store", default="1.6.0")
     parser.addoption("--ei-mxnet-full-version", action="store", default="1.5.1")
-    parser.addoption("--pytorch-full-version", action="store", default=PyTorch.LATEST_VERSION)
+    parser.addoption("--pytorch-full-version", action="store", default="1.5.0")
     parser.addoption(
         "--rl-coach-mxnet-full-version",
         action="store",
@@ -64,10 +58,10 @@ def pytest_addoption(parser):
     parser.addoption(
         "--rl-ray-full-version", action="store", default=RLEstimator.RAY_LATEST_VERSION
     )
-    parser.addoption("--sklearn-full-version", action="store", default=SKLEARN_VERSION)
-    parser.addoption("--tf-full-version", action="store")
+    parser.addoption("--sklearn-full-version", action="store", default="0.20.0")
+    parser.addoption("--tf-full-version", action="store", default="2.2.0")
     parser.addoption("--ei-tf-full-version", action="store")
-    parser.addoption("--xgboost-full-version", action="store", default=SKLEARN_VERSION)
+    parser.addoption("--xgboost-full-version", action="store", default="1.0-1")
 
 
 def pytest_configure(config):
@@ -291,17 +285,13 @@ def sklearn_full_version(request):
     return request.config.getoption("--sklearn-full-version")
 
 
-@pytest.fixture(scope="module", params=[TensorFlow._LATEST_1X_VERSION, LATEST_VERSION])
+@pytest.fixture(scope="module")
 def tf_full_version(request):
-    tf_version = request.config.getoption("--tf-full-version")
-    if tf_version is None:
-        return request.param
-    else:
-        return tf_version
+    return request.config.getoption("--tf-full-version")
 
 
 @pytest.fixture(scope="module")
-def tf_full_py_version(tf_full_version, request):
+def tf_full_py_version(tf_full_version):
     """fixture to match tf_full_version
 
     Fixture exists as such, since tf_full_version may be overridden --tf-full-version.
@@ -312,9 +302,17 @@ def tf_full_py_version(tf_full_version, request):
     version = [int(val) for val in tf_full_version.split(".")]
     if version < [1, 11]:
         return "py2"
-    if tf_full_version in [TensorFlow._LATEST_1X_VERSION, LATEST_VERSION]:
-        return "py37"
-    return "py3"
+    if version < [2, 2]:
+        return "py3"
+    return "py37"
+
+
+@pytest.fixture(scope="module")
+def tf_serving_version(tf_full_version):
+    full_version = [int(val) for val in tf_full_version.split(".")]
+    if full_version < [2, 2]:
+        return tf_full_version
+    return "2.1.0"
 
 
 @pytest.fixture(scope="module", params=["1.15.0", "2.0.0"])
@@ -384,10 +382,3 @@ def pytest_generate_tests(metafunc):
 @pytest.fixture(scope="module")
 def xgboost_full_version(request):
     return request.config.getoption("--xgboost-full-version")
-
-
-@pytest.fixture(scope="module")
-def tf_serving_version(tf_full_version):
-    if tf_full_version == LATEST_VERSION:
-        return LATEST_SERVING_VERSION
-    return tf_full_version
diff --git a/tests/integ/test_airflow_config.py b/tests/integ/test_airflow_config.py
@@ -40,7 +40,6 @@
 from sagemaker.pytorch.estimator import PyTorch
 from sagemaker.sklearn import SKLearn
 from sagemaker.tensorflow import TensorFlow
-from sagemaker.xgboost.defaults import XGBOOST_LATEST_VERSION
 from sagemaker.workflow import airflow as sm_airflow
 from sagemaker.utils import sagemaker_timestamp
 
@@ -550,7 +549,9 @@ def test_sklearn_airflow_config_uploads_data_source_to_s3(
 
 
 @pytest.mark.canary_quick
-def test_tf_airflow_config_uploads_data_source_to_s3(sagemaker_session, cpu_instance_type):
+def test_tf_airflow_config_uploads_data_source_to_s3(
+    sagemaker_session, cpu_instance_type, tf_full_version, tf_full_py_version
+):
     with timeout(seconds=AIRFLOW_CONFIG_TIMEOUT_IN_SECONDS):
         tf = TensorFlow(
             image_name=get_image_uri(
@@ -561,8 +562,8 @@ def test_tf_airflow_config_uploads_data_source_to_s3(sagemaker_session, cpu_inst
             train_instance_count=SINGLE_INSTANCE_COUNT,
             train_instance_type=cpu_instance_type,
             sagemaker_session=sagemaker_session,
-            framework_version=TensorFlow.LATEST_VERSION,
-            py_version="py37",  # only version available with 2.2.0
+            framework_version=tf_full_version,
+            py_version=tf_full_py_version,
             metric_definitions=[
                 {"Name": "train:global_steps", "Regex": r"global_step\/sec:\s(.*)"}
             ],
@@ -583,11 +584,13 @@ def test_tf_airflow_config_uploads_data_source_to_s3(sagemaker_session, cpu_inst
 
 @pytest.mark.canary_quick
 @pytest.mark.skipif(PYTHON_VERSION == "py2", reason="XGBoost container does not support Python 2.")
-def test_xgboost_airflow_config_uploads_data_source_to_s3(sagemaker_session, cpu_instance_type):
+def test_xgboost_airflow_config_uploads_data_source_to_s3(
+    sagemaker_session, cpu_instance_type, xgboost_full_version
+):
     with timeout(seconds=AIRFLOW_CONFIG_TIMEOUT_IN_SECONDS):
         xgboost = XGBoost(
             entry_point=os.path.join(DATA_DIR, "dummy_script.py"),
-            framework_version=XGBOOST_LATEST_VERSION,
+            framework_version=xgboost_full_version,
             role=ROLE,
             sagemaker_session=sagemaker_session,
             train_instance_type=cpu_instance_type,
diff --git a/tests/integ/test_git.py b/tests/integ/test_git.py
@@ -84,7 +84,7 @@ def test_github(sagemaker_local_session):
 
 @pytest.mark.local_mode
 @pytest.mark.skip("needs a secure authentication approach")
-def test_private_github(sagemaker_local_session):
+def test_private_github(sagemaker_local_session, mxnet_full_version):
     script_path = "mnist.py"
     data_path = os.path.join(DATA_DIR, "mxnet_mnist")
     git_config = {
@@ -102,7 +102,7 @@ def test_private_github(sagemaker_local_session):
         role="SageMakerRole",
         source_dir=source_dir,
         dependencies=dependencies,
-        framework_version=MXNet.LATEST_VERSION,
+        framework_version=mxnet_full_version,
         py_version=PYTHON_VERSION,
         train_instance_count=1,
         train_instance_type="local",
@@ -222,7 +222,7 @@ def test_github_with_ssh_passphrase_not_configured(sagemaker_local_session, skle
 
 @pytest.mark.local_mode
 @pytest.mark.skip("needs a secure authentication approach")
-def test_codecommit(sagemaker_local_session):
+def test_codecommit(sagemaker_local_session, mxnet_full_version):
     script_path = "mnist.py"
     data_path = os.path.join(DATA_DIR, "mxnet_mnist")
     git_config = {
@@ -238,7 +238,7 @@ def test_codecommit(sagemaker_local_session):
         role="SageMakerRole",
         source_dir=source_dir,
         dependencies=dependencies,
-        framework_version=MXNet.LATEST_VERSION,
+        framework_version=mxnet_full_version,
         py_version=PYTHON_VERSION,
         train_instance_count=1,
         train_instance_type="local",
diff --git a/tests/integ/test_sklearn_train.py b/tests/integ/test_sklearn_train.py
@@ -18,7 +18,6 @@
 import pytest
 import numpy
 
-from sagemaker.sklearn.defaults import SKLEARN_VERSION
 from sagemaker.sklearn import SKLearn
 from sagemaker.sklearn import SKLearnModel
 from sagemaker.utils import sagemaker_timestamp, unique_name_from_base
@@ -147,12 +146,15 @@ def test_deploy_model(
     reason="This test has always failed, but the failure was masked by a bug. "
     "This test should be fixed. Details in https://github.com/aws/sagemaker-python-sdk/pull/968"
 )
-def test_async_fit(sagemaker_session, cpu_instance_type):
+def test_async_fit(sagemaker_session, cpu_instance_type, sklearn_full_version):
     endpoint_name = "test-sklearn-attach-deploy-{}".format(sagemaker_timestamp())
 
     with timeout(minutes=5):
         training_job_name = _run_mnist_training_job(
-            sagemaker_session, cpu_instance_type, sklearn_full_version=SKLEARN_VERSION, wait=False
+            sagemaker_session,
+            cpu_instance_type,
+            sklearn_full_version=sklearn_full_version,
+            wait=False,
         )
 
         print("Waiting to re-attach to the training job: %s" % training_job_name)
diff --git a/tests/integ/test_tf.py b/tests/integ/test_tf.py
diff --git a/tests/integ/test_tf_efs_fsx.py b/tests/integ/test_tf_efs_fsx.py
diff --git a/tests/integ/test_transformer.py b/tests/integ/test_transformer.py
diff --git a/tests/unit/test_sklearn.py b/tests/unit/test_sklearn.py
diff --git a/tests/unit/test_xgboost.py b/tests/unit/test_xgboost.py

Original file line number	Diff line number	Diff line change
`@@ -115,7 +115,7 @@ def __init__(`
`115`	`115`	:class:`~sagemaker.model.Model`.
`116`	`116`	`"""`
`117`	`117`	`validate_version_or_image_args(framework_version, py_version, image)`
`118`		`- if py_version and py_version == "py2":`
	`118`	`+ if py_version == "py2":`
`119`	`119`	`logger.warning(`
`120`	`120`	`python_deprecation_warning(self.__framework_name__, defaults.LATEST_PY2_VERSION)`
`121`	`121`	`)`
Original file line number	Diff line number	Diff line change
`@@ -114,7 +114,7 @@ def __init__(`
`114`	`114`	:class:`~sagemaker.model.Model`.
`115`	`115`	`"""`
`116`	`116`	`validate_version_or_image_args(framework_version, py_version, image)`
`117`		`- if py_version and py_version == "py2":`
	`117`	`+ if py_version == "py2":`
`118`	`118`	`logger.warning(`
`119`	`119`	`python_deprecation_warning(self.__framework_name__, defaults.LATEST_PY2_VERSION)`
`120`	`120`	`)`