Merge pull request flyteorg#3 from honnix/patch-2

Make config file optional

.gitignore

@@ -16,4 +16,4 @@ build/
 .ipynb_checkpoints/
 .pytest_cache/
 dist
-
+*.iml

flytekit/clis/sdk_in_container/pyflyte.py

@@ -81,10 +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:
-        click.secho("Configuration file '{}' could not be loaded.".format(CONFIGURATION_PATH.get()), color='red')
-        exit(-1)
+        click.secho("Configuration file '{}' could not be loaded. Using values from environment.".format(CONFIGURATION_PATH.get()),
+                    color='yellow')
+        set_flyte_config_file(None)
+    click.secho('Flyte Admin URL {}'.format(_URL.get()), fg='green')
 
 
 main.add_command(register)

flytekit/configuration/__init__.py

@@ -2,6 +2,11 @@
 import os as _os
 import six as _six
 
+try:
+    import pathlib as _pathlib
+except ImportError:
+    import pathlib2 as _pathlib # 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 _pathlib.Path(self._new_config_path).is_file():
+            set_flyte_config_file(self._new_config_path)
+        else:
+            set_flyte_config_file(None)

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):

requirements.txt

@@ -1,2 +1,3 @@
 pytest
+mock
 six

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:

tests/flytekit/unit/configuration/test_temporary_configuration.py

@@ -0,0 +1,37 @@
+from __future__ import absolute_import
+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') == \
+            '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'