Add Pylint (#465)

Add Pylint checking. Fixed all the current errors and warnings,
any future PRs will fail if they introduce any pylint error/warnings.

Author: iquintero

.pylintrc

@@ -0,0 +1,86 @@
+[MASTER]
+
+ignore=
+    tensorflow_serving
+
+[MESSAGES CONTROL]
+
+disable=
+    C, # convention
+    R, # refactor
+    too-many-arguments, # We should fix the offending ones soon.
+    too-many-lines, # Some files are too big, we should fix this too
+    too-few-public-methods,
+    too-many-instance-attributes,
+    too-many-locals,
+    len-as-condition, # Nice to have in the future
+    bad-indentation,
+    line-too-long, # We let Flake8 take care of this
+    logging-format-interpolation,
+    useless-object-inheritance, # We still support python2 so inheriting from object is ok
+    invalid-name,
+    import-error,
+    logging-not-lazy,
+    fixme,
+    no-self-use,
+    attribute-defined-outside-init,
+    protected-access,
+    invalid-all-object,
+    arguments-differ,
+    abstract-method,
+    signature-differs
+
+[REPORTS]
+# Set the output format. Available formats are text, parseable, colorized, msvs
+# (visual studio) and html
+output-format=colorized
+
+# Tells whether to display a full report or only the messages
+# CHANGE: No report.
+reports=no
+
+[FORMAT]
+# Maximum number of characters on a single line.
+max-line-length=100
+# Maximum number of lines in a module
+#max-module-lines=1000
+# String used as indentation unit. This is usually " " (4 spaces) or "\t" (1 tab).
+indent-string='  '
+
+[BASIC]
+
+# Required attributes for module, separated by a comma
+#required-attributes=
+# List of builtins function names that should not be used, separated by a comma.
+# XXX: Should we ban map() & filter() for list comprehensions?
+# exit & quit are for the interactive interpreter shell only.
+# https://docs.python.org/3/library/constants.html#constants-added-by-the-site-module
+bad-functions=
+    apply,
+    exit,
+    input,
+    quit,
+
+[SIMILARITIES]
+# Minimum lines number of a similarity.
+min-similarity-lines=5
+# Ignore comments when computing similarities.
+ignore-comments=yes
+# Ignore docstrings when computing similarities.
+ignore-docstrings=yes
+
+[VARIABLES]
+# Tells whether we should check for unused import in __init__ files.
+init-import=no
+# A regular expression matching the beginning of the name of dummy variables
+# (i.e. not used).
+dummy-variables-rgx=_|unused_
+
+# List of additional names supposed to be defined in builtins. Remember that
+# you should avoid to define new builtins when possible.
+#additional-builtins=
+
+[LOGGING]
+# Apply logging string format checks to calls on these modules.
+logging-modules=
+    logging

CHANGELOG.rst

@@ -6,6 +6,7 @@ CHANGELOG
 ==========
 
 * doc-fix: fix rendering error in README.rst
+* build: added pylint
 
 1.14.1
 ======

src/sagemaker/amazon/amazon_estimator.py

@@ -15,7 +15,9 @@
 import json
 import logging
 import tempfile
+
 from six.moves.urllib.parse import urlparse
+
 from sagemaker.amazon import validation
 from sagemaker.amazon.hyperparameter import Hyperparameter as hp  # noqa
 from sagemaker.amazon.common import write_numpy_to_dense_tensor
@@ -32,6 +34,8 @@ class AmazonAlgorithmEstimatorBase(EstimatorBase):
 
     feature_dim = hp('feature_dim', validation.gt(0), data_type=int)
     mini_batch_size = hp('mini_batch_size', validation.gt(0), data_type=int)
+    repo_name = None
+    repo_version = None
 
     def __init__(self, role, train_instance_count, train_instance_type, data_location=None, **kwargs):
         """Initialize an AmazonAlgorithmEstimatorBase.
@@ -263,7 +267,7 @@ def upload_numpy_to_s3_shards(num_shards, s3, bucket, key_prefix, array, labels=
             [{'prefix': 's3://{}/{}'.format(bucket, key_prefix)}] + uploaded_files)
         s3.Object(bucket, manifest_key).put(Body=manifest_str.encode('utf-8'))
         return "s3://{}/{}".format(bucket, manifest_key)
-    except Exception as ex:
+    except Exception as ex:  # pylint: disable=broad-except
         try:
             for file in uploaded_files:
                 s3.Object(bucket, key_prefix + file).delete()

src/sagemaker/amazon/kmeans.py

@@ -121,9 +121,9 @@ def _prepare_for_training(self, records, mini_batch_size=5000, job_name=None):
 
     def hyperparameters(self):
         """Return the SageMaker hyperparameters for training this KMeans Estimator"""
-        hp = dict(force_dense='True')  # KMeans requires this hp to fit on Record objects
-        hp.update(super(KMeans, self).hyperparameters())
-        return hp
+        hp_dict = dict(force_dense='True')  # KMeans requires this hp to fit on Record objects
+        hp_dict.update(super(KMeans, self).hyperparameters())
+        return hp_dict
 
 
 class KMeansPredictor(RealTimePredictor):

src/sagemaker/cli/mxnet.py

@@ -24,9 +24,6 @@ def host(args):
 
 
 class MXNetTrainCommand(TrainCommand):
-    def __init__(self, args):
-        super(MXNetTrainCommand, self).__init__(args)
-
     def create_estimator(self):
         from sagemaker.mxnet.estimator import MXNet
         return MXNet(self.script,
@@ -39,9 +36,6 @@ def create_estimator(self):
 
 
 class MXNetHostCommand(HostCommand):
-    def __init__(self, args):
-        super(MXNetHostCommand, self).__init__(args)
-
     def create_model(self, model_url):
         from sagemaker.mxnet.model import MXNetModel
         return MXNetModel(model_data=model_url, role=self.role_name, entry_point=self.script,

src/sagemaker/cli/tensorflow.py

@@ -43,9 +43,6 @@ def create_estimator(self):
 
 
 class TensorFlowHostCommand(HostCommand):
-    def __init__(self, args):
-        super(TensorFlowHostCommand, self).__init__(args)
-
     def create_model(self, model_url):
         from sagemaker.tensorflow.model import TensorFlowModel
         return TensorFlowModel(model_data=model_url, role=self.role_name, entry_point=self.script,

src/sagemaker/estimator.py

@@ -210,21 +210,6 @@ def fit(self, inputs=None, wait=True, logs=True, job_name=None):
         if wait:
             self.latest_training_job.wait(logs=logs)
 
-    @classmethod
-    def _from_training_job(cls, init_params, hyperparameters, image, sagemaker_session):
-        """Create an Estimator from existing training job data.
-
-        Args:
-            init_params (dict): The init_params the training job was created with.
-            hyperparameters (dict):  The hyperparameters the training job was created with.
-            image (str): Container image (if any) the training job was created with
-            sagemaker_session (sagemaker.session.Session): A sagemaker Session to pass to the estimator.
-
-        Returns: An instance of the calling Estimator Class.
-
-        """
-        raise NotImplementedError()
-
     @classmethod
     def attach(cls, training_job_name, sagemaker_session=None, model_channel_name='model'):
         """Attach to an existing training job.
@@ -262,7 +247,7 @@ def attach(cls, training_job_name, sagemaker_session=None, model_channel_name='m
 
         estimator = cls(sagemaker_session=sagemaker_session, **init_params)
         estimator.latest_training_job = _TrainingJob(sagemaker_session=sagemaker_session,
-                                                     training_job_name=init_params['base_job_name'])
+                                                     job_name=init_params['base_job_name'])
         estimator.latest_training_job.wait()
         return estimator
 
@@ -425,9 +410,6 @@ def _ensure_latest_training_job(self, error_message='Estimator is not associated
 
 
 class _TrainingJob(_Job):
-    def __init__(self, sagemaker_session, training_job_name):
-        super(_TrainingJob, self).__init__(sagemaker_session, training_job_name)
-
     @classmethod
     def start_new(cls, estimator, inputs):
         """Create a new Amazon SageMaker training job from the estimator.
@@ -627,12 +609,10 @@ class Framework(EstimatorBase):
     such as training/deployment images and predictor instances.
     """
 
-    _DISTRIBUTION_SUPPORTED_FRAMEWORKS = ('mxnet',)
-    LAUNCH_PS_ENV_NAME = 'sagemaker_parameter_server_enabled'
+    __framework_name__ = None
 
     def __init__(self, entry_point, source_dir=None, hyperparameters=None, enable_cloudwatch_metrics=False,
-                 container_log_level=logging.INFO, code_location=None, image_name=None,
-                 distributions=None, **kwargs):
+                 container_log_level=logging.INFO, code_location=None, image_name=None, **kwargs):
         """Base class initializer. Subclasses which override ``__init__`` should invoke ``super()``
 
         Args:
@@ -654,8 +634,6 @@ def __init__(self, entry_point, source_dir=None, hyperparameters=None, enable_cl
             image_name (str): An alternate image name to use instead of the official Sagemaker image
                 for the framework. This is useful to run one of the Sagemaker supported frameworks
                 with an image containing custom dependencies.
-            distributions (dict): A dictionary with information on how to run distributed training
-                (default: None).
             **kwargs: Additional kwargs passed to the ``EstimatorBase`` constructor.
         """
         super(Framework, self).__init__(**kwargs)
@@ -670,22 +648,6 @@ def __init__(self, entry_point, source_dir=None, hyperparameters=None, enable_cl
         self.image_name = image_name
 
         self._hyperparameters = hyperparameters or {}
-        self._configure_distributions(distributions)
-
-    def _configure_distributions(self, distributions):
-        if distributions is None:
-            return
-
-        if self.__framework_name__ not in self._DISTRIBUTION_SUPPORTED_FRAMEWORKS:
-            raise ValueError('This framework does not support the distributions option.')
-
-        if self.framework_version.split('.') < self._LOWEST_SCRIPT_MODE_VERSION:
-            raise ValueError('The distributions option is valid for only versions {} and higher'
-                             .format('.'.join(self._LOWEST_SCRIPT_MODE_VERSION)))
-
-        if 'parameter_server' in distributions:
-            enabled = distributions['parameter_server'].get('enabled', False)
-            self._hyperparameters[self.LAUNCH_PS_ENV_NAME] = enabled
 
     def _prepare_for_training(self, job_name=None):
         """Set hyperparameters needed for training. This method will also validate ``source_dir``.
@@ -810,8 +772,11 @@ def train_image(self):
         if self.image_name:
             return self.image_name
         else:
-            return create_image_uri(self.sagemaker_session.boto_region_name, self.__framework_name__,
-                                    self.train_instance_type, self.framework_version, py_version=self.py_version)
+            return create_image_uri(self.sagemaker_session.boto_region_name,
+                                    self.__framework_name__,
+                                    self.train_instance_type,
+                                    self.framework_version,  # pylint: disable=no-member
+                                    py_version=self.py_version)  # pylint: disable=no-member
 
     @classmethod
     def attach(cls, training_job_name, sagemaker_session=None, model_channel_name='model'):
@@ -844,7 +809,11 @@ def attach(cls, training_job_name, sagemaker_session=None, model_channel_name='m
             Instance of the calling ``Estimator`` Class with the attached training job.
         """
         estimator = super(Framework, cls).attach(training_job_name, sagemaker_session, model_channel_name)
-        estimator.uploaded_code = UploadedCode(estimator.source_dir, estimator.entry_point)
+
+        # pylint gets confused thinking that estimator is an EstimatorBase instance, but it actually
+        # is a Framework or any of its derived classes. We can safely ignore the no-member errors.
+        estimator.uploaded_code = UploadedCode(
+            estimator.source_dir, estimator.entry_point)  # pylint: disable=no-member
         return estimator
 
     @staticmethod

src/sagemaker/fw_utils.py

@@ -19,8 +19,6 @@
 
 import sagemaker.utils
 
-"""This module contains utility functions shared across ``Framework`` components."""
-
 UploadedCode = namedtuple('UserCode', ['s3_prefix', 'script_name'])
 """sagemaker.fw_utils.UserCode: An object containing the S3 prefix and script name.
 
@@ -36,8 +34,9 @@
 VALID_PY_VERSIONS = ['py2', 'py3']
 
 
-def create_image_uri(region, framework, instance_type, framework_version, py_version=None, account='520713654638',
-                     optimized_families=[]):
+def create_image_uri(region, framework, instance_type, framework_version, py_version=None,
+                     account='520713654638', optimized_families=None):
+
     """Return the ECR URI of an image.
 
     Args:
@@ -53,6 +52,7 @@ def create_image_uri(region, framework, instance_type, framework_version, py_ver
     Returns:
         str: The appropriate image URI based on the given parameters.
     """
+    optimized_families = optimized_families or []
 
     if py_version and py_version not in VALID_PY_VERSIONS:
         raise ValueError('invalid py_version argument: {}'.format(py_version))

src/sagemaker/job.py

@@ -32,7 +32,7 @@ def __init__(self, sagemaker_session, job_name):
         self.job_name = job_name
 
     @abstractmethod
-    def start_new(cls, estimator, inputs):
+    def start_new(self, estimator, inputs):
         """Create a new Amazon SageMaker job from the estimator.
 
         Args:
@@ -111,21 +111,21 @@ def _convert_input_to_channel(channel_name, channel_s3_input):
         return channel_config
 
     @staticmethod
-    def _format_string_uri_input(input):
-        if isinstance(input, str):
-            if input.startswith('s3://'):
-                return s3_input(input)
-            elif input.startswith('file://'):
-                return file_input(input)
+    def _format_string_uri_input(uri_input):
+        if isinstance(uri_input, str):
+            if uri_input.startswith('s3://'):
+                return s3_input(uri_input)
+            elif uri_input.startswith('file://'):
+                return file_input(uri_input)
             else:
                 raise ValueError('Training input data must be a valid S3 or FILE URI: must start with "s3://" or '
                                  '"file://"')
-        elif isinstance(input, s3_input):
-            return input
-        elif isinstance(input, file_input):
-            return input
+        elif isinstance(uri_input, s3_input):
+            return uri_input
+        elif isinstance(uri_input, file_input):
+            return uri_input
         else:
-            raise ValueError('Cannot format input {}. Expecting one of str, s3_input, or file_input'.format(input))
+            raise ValueError('Cannot format input {}. Expecting one of str, s3_input, or file_input'.format(uri_input))
 
     @staticmethod
     def _prepare_model_channel(input_config, model_uri=None, model_channel_name=None):