From 85e642c327015cee06fd1c1e6c74dd854b90e91c Mon Sep 17 00:00:00 2001 From: Karim Nakad Date: Fri, 12 Jul 2019 17:40:42 -0700 Subject: [PATCH] change: enable inconsistent-return-statements Pylint check Note that this commit also raises ValueErrors in situations that would previously have returned None. Per PEP8: Be consistent in return statements. Either all return statements in a function should return an expression, or none of them should. If any return statement returns an expression, any return statements where no value is returned should explicitly state this as return None, and an explicit return statement should be present at the end of the function (if reachable). --- .pylintrc | 1 - src/sagemaker/job.py | 2 +- src/sagemaker/local/data.py | 8 +++++++- src/sagemaker/model.py | 1 + src/sagemaker/pipeline.py | 1 + src/sagemaker/predictor.py | 5 +++++ src/sagemaker/rl/estimator.py | 10 +++++++++- 7 files changed, 24 insertions(+), 4 deletions(-) diff --git a/.pylintrc b/.pylintrc index 0a980d43b3..2a5c0c295b 100644 --- a/.pylintrc +++ b/.pylintrc @@ -91,7 +91,6 @@ disable= useless-object-inheritance, # TODO: Remove unnecessary imports cyclic-import, # TODO: Resolve cyclic imports no-self-use, # TODO: Convert methods to functions where appropriate - inconsistent-return-statements, # TODO: Make returns consistent consider-merging-isinstance, # TODO: Merge isinstance where appropriate consider-using-in, # TODO: Consider merging comparisons with "in" simplifiable-if-expression, # TODO: Simplify expressions diff --git a/src/sagemaker/job.py b/src/sagemaker/job.py index ffe4ba31b0..e590e474a7 100644 --- a/src/sagemaker/job.py +++ b/src/sagemaker/job.py @@ -169,7 +169,7 @@ def _prepare_channel( input_mode=None, ): if not channel_uri: - return + return None if not channel_name: raise ValueError( "Expected a channel name if a channel URI {} is specified".format(channel_uri) diff --git a/src/sagemaker/local/data.py b/src/sagemaker/local/data.py index 62f41bf4e0..cf3cd88af5 100644 --- a/src/sagemaker/local/data.py +++ b/src/sagemaker/local/data.py @@ -38,15 +38,21 @@ def get_data_source_instance(data_source, sagemaker_session): sagemaker_session (:class:`sagemaker.session.Session`): a SageMaker Session to interact with S3 if required. - Returns + Returns: :class:`sagemaker.local.data.DataSource`: an Instance of a Data Source + Raises: + ValueError: If parsed_uri scheme is neither `file` nor `s3`, raise an error. + """ parsed_uri = urlparse(data_source) if parsed_uri.scheme == "file": return LocalFileDataSource(parsed_uri.netloc + parsed_uri.path) if parsed_uri.scheme == "s3": return S3DataSource(parsed_uri.netloc, parsed_uri.path, sagemaker_session) + raise ValueError( + "data_source must be either file or s3. parsed_uri.scheme: {}".format(parsed_uri.scheme) + ) def get_splitter_instance(split_type): diff --git a/src/sagemaker/model.py b/src/sagemaker/model.py index e2352c5f41..571d51923f 100644 --- a/src/sagemaker/model.py +++ b/src/sagemaker/model.py @@ -389,6 +389,7 @@ def deploy( if self.predictor_cls: return self.predictor_cls(self.endpoint_name, self.sagemaker_session) + return None def transformer( self, diff --git a/src/sagemaker/pipeline.py b/src/sagemaker/pipeline.py index 027f7a9c16..a91631dde7 100644 --- a/src/sagemaker/pipeline.py +++ b/src/sagemaker/pipeline.py @@ -115,6 +115,7 @@ def deploy( ) if self.predictor_cls: return self.predictor_cls(self.endpoint_name, self.sagemaker_session) + return None def _create_sagemaker_pipeline_model(self, instance_type): """Create a SageMaker Model Entity diff --git a/src/sagemaker/predictor.py b/src/sagemaker/predictor.py index 4bdf0ed665..68efe14d20 100644 --- a/src/sagemaker/predictor.py +++ b/src/sagemaker/predictor.py @@ -392,6 +392,11 @@ def __call__(self, stream, content_type=CONTENT_TYPE_NPY): return np.load(BytesIO(stream.read())) finally: stream.close() + raise ValueError( + "content_type must be one of the following: CSV, JSON, NPY. content_type: {}".format( + content_type + ) + ) numpy_deserializer = _NumpyDeserializer() diff --git a/src/sagemaker/rl/estimator.py b/src/sagemaker/rl/estimator.py index 5836948c10..aa2565d02a 100644 --- a/src/sagemaker/rl/estimator.py +++ b/src/sagemaker/rl/estimator.py @@ -184,7 +184,8 @@ def create_model( MXNet was used as RL backend; * sagemaker.tensorflow.serving.Model - if image_name wasn't specified and TensorFlow was used as RL backend. - + Raises: + ValueError: If image_name was not specified and framework enum is not valid. """ base_args = dict( model_data=self.model_data, @@ -230,6 +231,9 @@ def create_model( return MXNetModel( framework_version=self.framework_version, py_version=PYTHON_VERSION, **extended_args ) + raise ValueError( + "An unknown RLFramework enum was passed in. framework: {}".format(self.framework) + ) def train_image(self): """Return the Docker image to use for training. @@ -399,6 +403,9 @@ def default_metric_definitions(cls, toolkit): Returns: list: metric definitions + + Raises: + ValueError: If toolkit enum is not valid. """ if toolkit is RLToolkit.COACH: return [ @@ -412,3 +419,4 @@ def default_metric_definitions(cls, toolkit): {"Name": "episode_reward_mean", "Regex": "episode_reward_mean: (%s)" % float_regex}, {"Name": "episode_reward_max", "Regex": "episode_reward_max: (%s)" % float_regex}, ] + raise ValueError("An unknown RLToolkit enum was passed in. toolkit: {}".format(toolkit))