From d90571f0c9ef57ffec73f805dcd46942748975c7 Mon Sep 17 00:00:00 2001 From: Honnix Date: Thu, 22 Aug 2019 17:29:26 +0200 Subject: [PATCH 1/7] Update pyflyte.py Is it possible to ignore config file and use environment variables (if exist or fail loudly) instead? The reasoning is to hide config file from workflow developers, so that doesn't need to be in the repository. --- flytekit/clis/sdk_in_container/pyflyte.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/flytekit/clis/sdk_in_container/pyflyte.py b/flytekit/clis/sdk_in_container/pyflyte.py index 8d439364729..73ae266a865 100644 --- a/flytekit/clis/sdk_in_container/pyflyte.py +++ b/flytekit/clis/sdk_in_container/pyflyte.py @@ -83,8 +83,9 @@ def update_configuration_file(config_file_path): set_flyte_config_file(configuration_file.as_posix()) click.secho('Flyte Admin URL {}'.format(_URL.get()), fg='green') else: - click.secho("Configuration file '{}' could not be loaded.".format(CONFIGURATION_PATH.get()), color='red') - exit(-1) + set_flyte_config_file(None) + click.secho("Configuration file '{}' could not be loaded. Reading values from environment.".format(CONFIGURATION_PATH.get()), + color='red') main.add_command(register) From 34b191ef482fe21b13353577a11dd9f733615b76 Mon Sep 17 00:00:00 2001 From: Hongxin Liang Date: Mon, 26 Aug 2019 16:44:41 +0200 Subject: [PATCH 2/7] make TemporaryConfiguration accept None or nonexist file --- .gitignore | 2 +- flytekit/clis/sdk_in_container/pyflyte.py | 6 ++-- flytekit/configuration/__init__.py | 15 ++++++-- .../test_temporary_configuration.py | 35 +++++++++++++++++++ 4 files changed, 52 insertions(+), 6 deletions(-) create mode 100644 tests/flytekit/unit/configuration/test_temporary_configuration.py diff --git a/.gitignore b/.gitignore index 21499114042..0a53e2ec9b6 100644 --- a/.gitignore +++ b/.gitignore @@ -16,4 +16,4 @@ build/ .ipynb_checkpoints/ .pytest_cache/ dist - +*.iml diff --git a/flytekit/clis/sdk_in_container/pyflyte.py b/flytekit/clis/sdk_in_container/pyflyte.py index 73ae266a865..df8640bcb28 100644 --- a/flytekit/clis/sdk_in_container/pyflyte.py +++ b/flytekit/clis/sdk_in_container/pyflyte.py @@ -81,11 +81,11 @@ def update_configuration_file(config_file_path): click.secho('Using configuration file at {}'.format(configuration_file.absolute().as_posix()), fg='green') set_flyte_config_file(configuration_file.as_posix()) - click.secho('Flyte Admin URL {}'.format(_URL.get()), fg='green') else: - set_flyte_config_file(None) - click.secho("Configuration file '{}' could not be loaded. Reading values from environment.".format(CONFIGURATION_PATH.get()), + click.secho("Configuration file '{}' could not be loaded. Using values from environment.".format(CONFIGURATION_PATH.get()), color='red') + set_flyte_config_file(None) + click.secho('Flyte Admin URL {}'.format(_URL.get()), fg='green') main.add_command(register) diff --git a/flytekit/configuration/__init__.py b/flytekit/configuration/__init__.py index ac34666d8b0..a9f89f2ca8b 100644 --- a/flytekit/configuration/__init__.py +++ b/flytekit/configuration/__init__.py @@ -2,6 +2,11 @@ import os as _os import six as _six +try: + from pathlib import Path +except ImportError: + from pathlib2 import Path # python 2 backport + def set_flyte_config_file(config_file_path): """ @@ -29,7 +34,7 @@ def __init__(self, new_config_path, internal_overrides=None): _common.format_section_key('internal', k): v for k, v in _six.iteritems(internal_overrides or {}) } - self._new_config_path = _os.path.abspath(new_config_path) + self._new_config_path = _os.path.abspath(new_config_path) if new_config_path else None self._old_config_path = None self._old_internals = None @@ -42,7 +47,7 @@ def __enter__(self): } self._old_config_path = _os.environ.get(_internal.CONFIGURATION_PATH.env_var) _os.environ.update(self._internal_overrides) - set_flyte_config_file(self._new_config_path) + self._set_flyte_config_file() def __exit__(self, exc_type, exc_val, exc_tb): set_flyte_config_file(self._old_config_path) @@ -52,3 +57,9 @@ def __exit__(self, exc_type, exc_val, exc_tb): else: _os.environ.pop(k, None) self._old_internals = None + + def _set_flyte_config_file(self): + if self._new_config_path and Path(self._new_config_path).is_file(): + set_flyte_config_file(self._new_config_path) + else: + set_flyte_config_file(None) diff --git a/tests/flytekit/unit/configuration/test_temporary_configuration.py b/tests/flytekit/unit/configuration/test_temporary_configuration.py new file mode 100644 index 00000000000..f360c8173ce --- /dev/null +++ b/tests/flytekit/unit/configuration/test_temporary_configuration.py @@ -0,0 +1,35 @@ +from __future__ import absolute_import +from flytekit.configuration import set_flyte_config_file, common, TemporaryConfiguration +import os + + +def test_configuration_file(): + with TemporaryConfiguration( + os.path.join(os.path.dirname(os.path.realpath(__file__)), 'configs/good.config')): + assert common.CONFIGURATION_SINGLETON.get_string('sdk', 'workflow_packages') == \ + 'this.module,that.module' + assert common.CONFIGURATION_SINGLETON.get_string('sdk', 'workflow_packages') is None + + +def test_internal_overrides(): + with TemporaryConfiguration( + os.path.join(os.path.dirname(os.path.realpath(__file__)), 'configs/good.config'), + {'foo': 'bar'}): + assert os.environ.get('FLYTE_INTERNAL_FOO') == 'bar' + assert os.environ.get('FLYTE_INTERNAL_FOO') is None + + +def test_no_configuration_file(): + set_flyte_config_file(os.path.join(os.path.dirname(os.path.realpath(__file__)), 'configs/good.config')) + with TemporaryConfiguration(None): + assert common.CONFIGURATION_SINGLETON.get_string('sdk', 'workflow_packages') is None + assert common.CONFIGURATION_SINGLETON.get_string('sdk', 'workflow_packages') == \ + 'this.module,that.module' + + +def test_nonexist_configuration_file(): + set_flyte_config_file(os.path.join(os.path.dirname(os.path.realpath(__file__)), 'configs/good.config')) + with TemporaryConfiguration('/foo/bar'): + assert common.CONFIGURATION_SINGLETON.get_string('sdk', 'workflow_packages') is None + assert common.CONFIGURATION_SINGLETON.get_string('sdk', 'workflow_packages') == \ + 'this.module,that.module' From 474a86cc894fea96bc7f3cd1934b03fa2f1ba37e Mon Sep 17 00:00:00 2001 From: Hongxin Liang Date: Mon, 26 Aug 2019 17:06:58 +0200 Subject: [PATCH 3/7] add mock --- requirements.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/requirements.txt b/requirements.txt index 9f7330208f1..ef72d60a8b6 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,2 +1,3 @@ pytest +mock six From df4b974f1f3737ec1c10fde34c054cd12dfe5d4d Mon Sep 17 00:00:00 2001 From: Hongxin Liang Date: Mon, 26 Aug 2019 17:08:23 +0200 Subject: [PATCH 4/7] parametrizing fixture --- tests/flytekit/unit/cli/pyflyte/conftest.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/tests/flytekit/unit/cli/pyflyte/conftest.py b/tests/flytekit/unit/cli/pyflyte/conftest.py index 9ed73cd0699..ce87bb2d0dd 100644 --- a/tests/flytekit/unit/cli/pyflyte/conftest.py +++ b/tests/flytekit/unit/cli/pyflyte/conftest.py @@ -13,11 +13,14 @@ def _fake_module_load(name): yield simple -@pytest.yield_fixture(scope='function', autouse=True) -def mock_ctx(): - with _config.TemporaryConfiguration( - os.path.join(os.path.dirname(os.path.realpath(__file__)), '../../../common/configs/local.config') - ): +@pytest.yield_fixture(scope='function', autouse=True, + params=[ + os.path.join(os.path.dirname(os.path.realpath(__file__)), '../../../common/configs/local.config'), + '/foo/bar', + None + ]) +def mock_ctx(request): + with _config.TemporaryConfiguration(request.param): sys.path.append(os.path.join(os.path.dirname(os.path.realpath(__file__)), '../../..')) try: with _mock.patch('flytekit.tools.module_loader.iterate_modules') as mock_module_load: From bb3602d62b071537ac5e6a8e08479e08dcb37340 Mon Sep 17 00:00:00 2001 From: Hongxin Liang Date: Wed, 28 Aug 2019 08:55:58 +0200 Subject: [PATCH 5/7] underscore imported stuff --- flytekit/configuration/__init__.py | 6 +-- .../test_temporary_configuration.py | 38 ++++++++++--------- 2 files changed, 23 insertions(+), 21 deletions(-) diff --git a/flytekit/configuration/__init__.py b/flytekit/configuration/__init__.py index a9f89f2ca8b..a81df7ff1de 100644 --- a/flytekit/configuration/__init__.py +++ b/flytekit/configuration/__init__.py @@ -3,9 +3,9 @@ import six as _six try: - from pathlib import Path + import pathlib as _pathlib except ImportError: - from pathlib2 import Path # python 2 backport + import pathlib2 as _pathlib # python 2 backport def set_flyte_config_file(config_file_path): @@ -59,7 +59,7 @@ def __exit__(self, exc_type, exc_val, exc_tb): self._old_internals = None def _set_flyte_config_file(self): - if self._new_config_path and Path(self._new_config_path).is_file(): + if self._new_config_path and _pathlib.Path(self._new_config_path).is_file(): set_flyte_config_file(self._new_config_path) else: set_flyte_config_file(None) diff --git a/tests/flytekit/unit/configuration/test_temporary_configuration.py b/tests/flytekit/unit/configuration/test_temporary_configuration.py index f360c8173ce..c569c2bc237 100644 --- a/tests/flytekit/unit/configuration/test_temporary_configuration.py +++ b/tests/flytekit/unit/configuration/test_temporary_configuration.py @@ -1,35 +1,37 @@ from __future__ import absolute_import -from flytekit.configuration import set_flyte_config_file, common, TemporaryConfiguration -import os +from flytekit.configuration import set_flyte_config_file as _set_flyte_config_file, \ + common as _common, \ + TemporaryConfiguration as _TemporaryConfiguration +import os as _os def test_configuration_file(): - with TemporaryConfiguration( - os.path.join(os.path.dirname(os.path.realpath(__file__)), 'configs/good.config')): - assert common.CONFIGURATION_SINGLETON.get_string('sdk', 'workflow_packages') == \ + with _TemporaryConfiguration( + _os.path.join(_os.path.dirname(_os.path.realpath(__file__)), 'configs/good.config')): + assert _common.CONFIGURATION_SINGLETON.get_string('sdk', 'workflow_packages') == \ 'this.module,that.module' - assert common.CONFIGURATION_SINGLETON.get_string('sdk', 'workflow_packages') is None + assert _common.CONFIGURATION_SINGLETON.get_string('sdk', 'workflow_packages') is None def test_internal_overrides(): - with TemporaryConfiguration( - os.path.join(os.path.dirname(os.path.realpath(__file__)), 'configs/good.config'), + with _TemporaryConfiguration( + _os.path.join(_os.path.dirname(_os.path.realpath(__file__)), 'configs/good.config'), {'foo': 'bar'}): - assert os.environ.get('FLYTE_INTERNAL_FOO') == 'bar' - assert os.environ.get('FLYTE_INTERNAL_FOO') is None + assert _os.environ.get('FLYTE_INTERNAL_FOO') == 'bar' + assert _os.environ.get('FLYTE_INTERNAL_FOO') is None def test_no_configuration_file(): - set_flyte_config_file(os.path.join(os.path.dirname(os.path.realpath(__file__)), 'configs/good.config')) - with TemporaryConfiguration(None): - assert common.CONFIGURATION_SINGLETON.get_string('sdk', 'workflow_packages') is None - assert common.CONFIGURATION_SINGLETON.get_string('sdk', 'workflow_packages') == \ + _set_flyte_config_file(_os.path.join(_os.path.dirname(_os.path.realpath(__file__)), 'configs/good.config')) + with _TemporaryConfiguration(None): + assert _common.CONFIGURATION_SINGLETON.get_string('sdk', 'workflow_packages') is None + assert _common.CONFIGURATION_SINGLETON.get_string('sdk', 'workflow_packages') == \ 'this.module,that.module' def test_nonexist_configuration_file(): - set_flyte_config_file(os.path.join(os.path.dirname(os.path.realpath(__file__)), 'configs/good.config')) - with TemporaryConfiguration('/foo/bar'): - assert common.CONFIGURATION_SINGLETON.get_string('sdk', 'workflow_packages') is None - assert common.CONFIGURATION_SINGLETON.get_string('sdk', 'workflow_packages') == \ + _set_flyte_config_file(_os.path.join(_os.path.dirname(_os.path.realpath(__file__)), 'configs/good.config')) + with _TemporaryConfiguration('/foo/bar'): + assert _common.CONFIGURATION_SINGLETON.get_string('sdk', 'workflow_packages') is None + assert _common.CONFIGURATION_SINGLETON.get_string('sdk', 'workflow_packages') == \ 'this.module,that.module' From ae57c6e55ae2d681ae99218a6fd4e081765a49a2 Mon Sep 17 00:00:00 2001 From: Hongxin Liang Date: Wed, 28 Aug 2019 08:56:38 +0200 Subject: [PATCH 6/7] warning should be yellow --- flytekit/clis/sdk_in_container/pyflyte.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/flytekit/clis/sdk_in_container/pyflyte.py b/flytekit/clis/sdk_in_container/pyflyte.py index df8640bcb28..7d0998ffa86 100644 --- a/flytekit/clis/sdk_in_container/pyflyte.py +++ b/flytekit/clis/sdk_in_container/pyflyte.py @@ -83,7 +83,7 @@ def update_configuration_file(config_file_path): set_flyte_config_file(configuration_file.as_posix()) else: click.secho("Configuration file '{}' could not be loaded. Using values from environment.".format(CONFIGURATION_PATH.get()), - color='red') + color='yellow') set_flyte_config_file(None) click.secho('Flyte Admin URL {}'.format(_URL.get()), fg='green') From 115168040ae5f38da1f5d32e055625ae2c22ebc3 Mon Sep 17 00:00:00 2001 From: Hongxin Liang Date: Wed, 28 Aug 2019 09:04:53 +0200 Subject: [PATCH 7/7] no need to read from env var in this way --- flytekit/configuration/common.py | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/flytekit/configuration/common.py b/flytekit/configuration/common.py index 6139aea9f40..8bb6a819ef8 100644 --- a/flytekit/configuration/common.py +++ b/flytekit/configuration/common.py @@ -22,15 +22,12 @@ class FlyteConfigurationFile(object): def __init__(self, location=None): """ - This singleton is initialized on module load with empty location. It'll attempt to use the env var to load - config at that time. If pyflyte is called with a config flag, it'll reload the singleton with the passed - config path. + This singleton is initialized on module load with empty location. If pyflyte is called with + a config flag, it'll reload the singleton with the passed config path. - :param Text location: used to load config from this location. If location is None, it'll attempt to use - FLYTE_INTERNAL_CONFIGURATION_PATH env var. + :param Text location: used to load config from this location. """ - # Can't use internal module here to get env var name to avoid circular dependency. - self._location = location or _os.environ.get("FLYTE_INTERNAL_CONFIGURATION_PATH") + self._location = location self._config = None def _load_config(self):