From 7779b87540ea57ec56d1b7ac42e420a6a6f9df23 Mon Sep 17 00:00:00 2001 From: Harutaka Kawamura Date: Tue, 21 Jul 2020 15:05:31 +0900 Subject: [PATCH] Remove f-strings because we still support Python 3.5 (#3121) --- .../store/model_registry/sqlalchemy_store.py | 10 ++-- mlflow/utils/search_utils.py | 20 +++++--- tests/deployments/test_cli.py | 6 +-- tests/fastai/test_fastai_autolog.py | 4 +- .../fake_deployment_plugin.py | 4 +- .../model_registry/test_sqlalchemy_store.py | 48 +++++++++---------- tests/test_no_f_strings.py | 41 ++++++++++++++++ tests/tracking/test_model_registry.py | 4 +- 8 files changed, 92 insertions(+), 45 deletions(-) create mode 100644 tests/test_no_f_strings.py diff --git a/mlflow/store/model_registry/sqlalchemy_store.py b/mlflow/store/model_registry/sqlalchemy_store.py index dd061215d55c6..25475114cd7f8 100644 --- a/mlflow/store/model_registry/sqlalchemy_store.py +++ b/mlflow/store/model_registry/sqlalchemy_store.py @@ -316,8 +316,8 @@ def compute_next_token(current_size): else: supported_ops = ''.join(['(' + op + ')' for op in SearchUtils.VALID_REGISTERED_MODEL_SEARCH_COMPARATORS]) - sample_query = f'name {supported_ops} ""' - raise MlflowException(f'Invalid filter string: {filter_string}' + sample_query = 'name {} ""'.format(supported_ops) + raise MlflowException('Invalid filter string: {}'.format(filter_string) + 'Search registered models supports filter expressions like:' + sample_query, error_code=INVALID_PARAMETER_VALUE) with self.ManagedSessionMaker() as session: @@ -349,9 +349,9 @@ def _parse_search_registered_models_order_by(cls, order_by_list): field = SqlRegisteredModel.last_updated_time else: raise MlflowException( - f"Invalid order by key '{attribute_token}' specified." - f"Valid keys are " - f"'{SearchUtils.RECOMMENDED_ORDER_BY_KEYS_REGISTERED_MODELS}'", + "Invalid order by key '{}' specified.".format(attribute_token) + + "Valid keys are " + + "'{}'".format(SearchUtils.RECOMMENDED_ORDER_BY_KEYS_REGISTERED_MODELS), error_code=INVALID_PARAMETER_VALUE) if ascending: clauses.append(field.asc()) diff --git a/mlflow/utils/search_utils.py b/mlflow/utils/search_utils.py index dc5ab118e181c..f1873723827cc 100644 --- a/mlflow/utils/search_utils.py +++ b/mlflow/utils/search_utils.py @@ -340,10 +340,11 @@ def _validate_order_by_and_generate_token(cls, order_by): try: parsed = sqlparse.parse(order_by) except Exception: - raise MlflowException(f"Error on parsing order_by clause '{order_by}'", + raise MlflowException("Error on parsing order_by clause '{}'".format(order_by), error_code=INVALID_PARAMETER_VALUE) if len(parsed) != 1 or not isinstance(parsed[0], Statement): - raise MlflowException(f"Invalid order_by clause '{order_by}'. Could not be parsed.", + raise MlflowException("Invalid order_by clause '{}'. Could not be parsed." + .format(order_by), error_code=INVALID_PARAMETER_VALUE) statement = parsed[0] if len(statement.tokens) == 1 and isinstance(statement[0], Identifier): @@ -358,7 +359,8 @@ def _validate_order_by_and_generate_token(cls, order_by): and statement.tokens[-1].ttype == TokenType.Keyword.Order: token_value = cls.ORDER_BY_KEY_TIMESTAMP + ' ' + statement.tokens[-1].value else: - raise MlflowException(f"Invalid order_by clause '{order_by}'. Could not be parsed.", + raise MlflowException("Invalid order_by clause '{}'. Could not be parsed." + .format(order_by), error_code=INVALID_PARAMETER_VALUE) return token_value @@ -368,12 +370,14 @@ def _parse_order_by_string(cls, order_by): is_ascending = True tokens = token_value.split() if len(tokens) > 2: - raise MlflowException(f"Invalid order_by clause '{order_by}'. Could not be parsed.", + raise MlflowException("Invalid order_by clause '{}'. Could not be parsed." + .format(order_by), error_code=INVALID_PARAMETER_VALUE) elif len(tokens) == 2: order_token = tokens[1].lower() if order_token not in cls.VALID_ORDER_BY_TAGS: - raise MlflowException(f"Invalid ordering key in order_by clause '{order_by}'.", + raise MlflowException("Invalid ordering key in order_by clause '{}'." + .format(order_by), error_code=INVALID_PARAMETER_VALUE) is_ascending = (order_token == cls.ASC_OPERATOR) token_value = tokens[0] @@ -390,8 +394,10 @@ def parse_order_by_for_search_registered_models(cls, order_by): token_value, is_ascending = cls._parse_order_by_string(order_by) token_value = token_value.strip() if token_value not in cls.VALID_ORDER_BY_KEYS_REGISTERED_MODELS: - raise MlflowException(f"Invalid order by key '{token_value}' specified. Valid keys " - f"are '{cls.RECOMMENDED_ORDER_BY_KEYS_REGISTERED_MODELS}'", + raise MlflowException("Invalid order by key '{}' specified. Valid keys " + .format(token_value) + + "are '{}'" + .format(cls.RECOMMENDED_ORDER_BY_KEYS_REGISTERED_MODELS), error_code=INVALID_PARAMETER_VALUE) return token_value, is_ascending diff --git a/tests/deployments/test_cli.py b/tests/deployments/test_cli.py index 313dba4ab2ec7..3756f506b57c4 100644 --- a/tests/deployments/test_cli.py +++ b/tests/deployments/test_cli.py @@ -74,6 +74,6 @@ def test_run_local(): runner = CliRunner() res = runner.invoke(cli.run_local, ['-f', f_flavor, '-m', f_model_uri, '-t', f_target, '--name', f_name]) - assert f"Deployed locally at the key {f_name}" in res.stdout - assert f"using the model from {f_model_uri}." in res.stdout - assert f"It's flavor is {f_flavor} and config is {str({})}" in res.stdout + assert "Deployed locally at the key {}".format(f_name) in res.stdout + assert "using the model from {}.".format(f_model_uri) in res.stdout + assert "It's flavor is {} and config is {}".format(f_flavor, str({})) in res.stdout diff --git a/tests/fastai/test_fastai_autolog.py b/tests/fastai/test_fastai_autolog.py index 6c16063e7b3e2..c9404f8efdbf2 100644 --- a/tests/fastai/test_fastai_autolog.py +++ b/tests/fastai/test_fastai_autolog.py @@ -188,7 +188,7 @@ def test_fastai_autolog_early_stop_logs(fastai_random_data_run_with_callback, pa assert 'early_stop_mode' in params assert params['early_stop_mode'] == 'auto' assert 'early_stop_min_delta' in params - assert params['early_stop_min_delta'] == f'-{MIN_DELTA}' + assert params['early_stop_min_delta'] == '-{}'.format(MIN_DELTA) client = mlflow.tracking.MlflowClient() metric_history = client.get_metric_history(run.info.run_id, 'valid_loss') @@ -212,7 +212,7 @@ def test_fastai_autolog_early_stop_no_stop_does_not_log( assert params['early_stop_monitor'] == 'valid_loss' assert 'early_stop_mode' in params assert 'early_stop_min_delta' in params - assert params['early_stop_min_delta'] == f'-{99999999}' + assert params['early_stop_min_delta'] == '-{}'.format(99999999) """ assert 'stopped_epoch' in metrics assert metrics['stopped_epoch'] == 0 diff --git a/tests/resources/mlflow-test-plugin/mlflow_test_plugin/fake_deployment_plugin.py b/tests/resources/mlflow-test-plugin/mlflow_test_plugin/fake_deployment_plugin.py index e0c0b8e64ca20..0e21a36d4169f 100644 --- a/tests/resources/mlflow-test-plugin/mlflow_test_plugin/fake_deployment_plugin.py +++ b/tests/resources/mlflow-test-plugin/mlflow_test_plugin/fake_deployment_plugin.py @@ -29,8 +29,8 @@ def predict(self, deployment_name, df): def run_local(name, model_uri, flavor=None, config=None): - print(f"Deployed locally at the key {name} using the model from {model_uri}. " - f"It's flavor is {flavor} and config is {config}") + print("Deployed locally at the key {} using the model from {}. ".format(name, model_uri) + + "It's flavor is {} and config is {}".format(flavor, config)) def target_help(): diff --git a/tests/store/model_registry/test_sqlalchemy_store.py b/tests/store/model_registry/test_sqlalchemy_store.py index 98f8cfccec38c..142a3ee62e698 100644 --- a/tests/store/model_registry/test_sqlalchemy_store.py +++ b/tests/store/model_registry/test_sqlalchemy_store.py @@ -214,7 +214,7 @@ def test_list_registered_model(self): set(["A", "BB", "AB", "BBC"])) def test_list_registered_model_paginated_last_page(self): - rms = [self._rm_maker(f"RM{i:03}").name for i in range(50)] + rms = [self._rm_maker("RM{:03}".format(i)).name for i in range(50)] # test flow with fixed max_results returned_rms = [] @@ -228,7 +228,7 @@ def test_list_registered_model_paginated_last_page(self): self.assertEqual(set(rms), set(returned_rms)) def test_list_registered_model_paginated_returns_in_correct_order(self): - rms = [self._rm_maker(f"RM{i:03}").name for i in range(50)] + rms = [self._rm_maker("RM{:03}".format(i)).name for i in range(50)] # test that pagination will return all valid results in sorted order # by name ascending @@ -250,7 +250,7 @@ def test_list_registered_model_paginated_returns_in_correct_order(self): self.assertEqual(result, rms[35:]) def test_list_registered_model_paginated_errors(self): - rms = [self._rm_maker(f"RM{i:03}").name for i in range(50)] + rms = [self._rm_maker("RM{:03}".format(i)).name for i in range(50)] # test that providing a completely invalid page token throws with self.assertRaises(MlflowException) as exception_context: self._list_registered_models(page_token="evilhax", max_results=20) @@ -263,7 +263,7 @@ def test_list_registered_model_paginated_errors(self): self.assertIn("Invalid value for request parameter max_results", exception_context.exception.message) # list should not return deleted models - self.store.delete_registered_model(name=f"RM{0:03}") + self.store.delete_registered_model(name="RM{0:03}".format(0)) self.assertEqual(set(self._list_registered_models(max_results=100)), set(rms[1:])) @@ -717,62 +717,62 @@ def test_search_registered_models(self): self.assertEqual(rms, names) # equality search using name should return exactly the 1 name - rms, _ = self._search_registered_models(f"name='{names[0]}'") + rms, _ = self._search_registered_models("name='{}'".format(names[0])) self.assertEqual(rms, [names[0]]) # equality search using name that is not valid should return nothing - rms, _ = self._search_registered_models(f"name='{names[0] + 'cats'}'") + rms, _ = self._search_registered_models("name='{}'".format(names[0] + 'cats')) self.assertEqual(rms, []) # case-sensitive prefix search using LIKE should return all the RMs - rms, _ = self._search_registered_models(f"name LIKE '{prefix}%'") + rms, _ = self._search_registered_models("name LIKE '{}%'".format(prefix)) self.assertEqual(rms, names) # case-sensitive prefix search using LIKE with surrounding % should return all the RMs - rms, _ = self._search_registered_models(f"name LIKE '%RM%'") + rms, _ = self._search_registered_models("name LIKE '%RM%'") self.assertEqual(rms, names) # case-sensitive prefix search using LIKE with surrounding % should return all the RMs # _e% matches test_for_search_ , so all RMs should match - rms, _ = self._search_registered_models(f"name LIKE '_e%'") + rms, _ = self._search_registered_models("name LIKE '_e%'") self.assertEqual(rms, names) # case-sensitive prefix search using LIKE should return just rm4 - rms, _ = self._search_registered_models(f"name LIKE '{prefix + 'RM4A'}%'") + rms, _ = self._search_registered_models("name LIKE '{}%'".format(prefix + 'RM4A')) self.assertEqual(rms, [names[4]]) # case-sensitive prefix search using LIKE should return no models if no match - rms, _ = self._search_registered_models(f"name LIKE '{prefix + 'cats'}%'") + rms, _ = self._search_registered_models("name LIKE '{}%'".format(prefix + 'cats')) self.assertEqual(rms, []) # confirm that LIKE is not case-sensitive - rms, _ = self._search_registered_models(f"name lIkE '%blah%'") + rms, _ = self._search_registered_models("name lIkE '%blah%'") self.assertEqual(rms, []) - rms, _ = self._search_registered_models(f"name like '{prefix + 'RM4A'}%'") + rms, _ = self._search_registered_models("name like '{}%'".format(prefix + 'RM4A')) self.assertEqual(rms, [names[4]]) # case-insensitive prefix search using ILIKE should return both rm5 and rm6 - rms, _ = self._search_registered_models(f"name ILIKE '{prefix + 'RM4A'}%'") + rms, _ = self._search_registered_models("name ILIKE '{}%'".format(prefix + 'RM4A')) self.assertEqual(rms, names[4:]) # case-insensitive postfix search with ILIKE - rms, _ = self._search_registered_models(f"name ILIKE '%RM4a'") + rms, _ = self._search_registered_models("name ILIKE '%RM4a'") self.assertEqual(rms, names[4:]) # case-insensitive prefix search using ILIKE should return both rm5 and rm6 - rms, _ = self._search_registered_models(f"name ILIKE '{prefix + 'cats'}%'") + rms, _ = self._search_registered_models("name ILIKE '{}%'".format(prefix + 'cats')) self.assertEqual(rms, []) # confirm that ILIKE is not case-sensitive - rms, _ = self._search_registered_models(f"name iLike '%blah%'") + rms, _ = self._search_registered_models("name iLike '%blah%'") self.assertEqual(rms, []) # confirm that ILIKE works for empty query - rms, _ = self._search_registered_models(f"name iLike '%%'") + rms, _ = self._search_registered_models("name iLike '%%'") self.assertEqual(rms, names) - rms, _ = self._search_registered_models(f"name ilike '%RM4a'") + rms, _ = self._search_registered_models("name ilike '%RM4a'") self.assertEqual(rms, names[4:]) # cannot search by invalid comparator types @@ -800,18 +800,18 @@ def test_search_registered_models(self): self.assertEqual(self._search_registered_models(None, max_results=1000), (names[:-1], None)) # equality search using name should return no names - self.assertEqual(self._search_registered_models(f"name='{names[-1]}'"), ([], None)) + self.assertEqual(self._search_registered_models("name='{}'".format(names[-1])), ([], None)) # case-sensitive prefix search using LIKE should return all the RMs - self.assertEqual(self._search_registered_models(f"name LIKE '{prefix}%'"), + self.assertEqual(self._search_registered_models("name LIKE '{}%'".format(prefix)), (names[0:5], None)) # case-insensitive prefix search using ILIKE should return both rm5 and rm6 - self.assertEqual(self._search_registered_models(f"name ILIKE '{prefix + 'RM4A'}%'"), + self.assertEqual(self._search_registered_models("name ILIKE '{}%'".format(prefix + 'RM4A')), ([names[4]], None)) def test_search_registered_model_pagination(self): - rms = [self._rm_maker(f"RM{i:03}").name for i in range(50)] + rms = [self._rm_maker("RM{:03}".format(i)).name for i in range(50)] # test flow with fixed max_results returned_rms = [] @@ -859,7 +859,7 @@ def test_search_registered_model_order_by(self): # explicitly mock the creation_timestamps because timestamps seem to be unstable in Windows for i in range(50): with mock.patch("mlflow.store.model_registry.sqlalchemy_store.now", return_value=i): - rms.append(self._rm_maker(f"RM{i:03}").name) + rms.append(self._rm_maker("RM{:03}".format(i)).name) # test flow with fixed max_results and order_by (test stable order across pages) returned_rms = [] diff --git a/tests/test_no_f_strings.py b/tests/test_no_f_strings.py new file mode 100644 index 0000000000000..6ec4c41b0acfd --- /dev/null +++ b/tests/test_no_f_strings.py @@ -0,0 +1,41 @@ +# TODO: Remove this test script once we drop Python 3.5 support. + +import ast +import os + + +def search_python_scripts(dirs): + for d in dirs: + for root, dirs, files in os.walk(d): + for f in files: + if f.endswith('.py'): + yield os.path.join(root, f) + + +def read_file(path): + with open(path, encoding='utf-8') as f: + return f.read() + + +def search_f_strings(node): + for c in ast.iter_child_nodes(node): + if isinstance(c, ast.JoinedStr): + yield (c.lineno, c.col_offset) + yield from search_f_strings(c) + + +def test_no_f_strings(): + dirs = ['mlflow', 'tests', 'examples'] + f_strings_all = [] + + for path in search_python_scripts(dirs): + src = read_file(path) + root = ast.parse(src) + f_strings = list(search_f_strings(root)) + if len(f_strings) > 0: + f_strings_all += [ + '{}:{}:{}: {}'.format(path, *pos, 'Remove f-string') + for pos in f_strings + ] + + assert len(f_strings_all) == 0, '\n' + '\n'.join(f_strings_all) diff --git a/tests/tracking/test_model_registry.py b/tests/tracking/test_model_registry.py index 5940505a9cba1..8ee0febe7b2d0 100644 --- a/tests/tracking/test_model_registry.py +++ b/tests/tracking/test_model_registry.py @@ -148,7 +148,7 @@ def _verify_pagination(rm_getter_with_token, expected_rms): @pytest.mark.parametrize("max_results", [1, 6, 100]) def test_list_registered_model_flow_paginated(mlflow_client, backend_store_uri, max_results): - names = [f'CreateRMlist{i:03}' for i in range(20)] + names = ['CreateRMlist{:03}'.format(i) for i in range(20)] rms = [mlflow_client.create_registered_model(name) for name in names] for rm in rms: assert isinstance(rm, RegisteredModel) @@ -176,7 +176,7 @@ def test_list_registered_model_flow_paginated(mlflow_client, backend_store_uri, ]) def test_search_registered_model_flow_paginated(mlflow_client, backend_store_uri, max_results, filter_string, filter_func): - names = [f'CreateRMsearch{i:03}' for i in range(29)] + names = ['CreateRMsearch{:03}'.format(i) for i in range(29)] rms = [mlflow_client.create_registered_model(name) for name in names] for rm in rms: assert isinstance(rm, RegisteredModel)