Repository: spacetx/starfish

Issue:
Warn on unknown config keys
If a config file is well-formed json but the key is misspelled, a warning should be issued.

Discussion:
codecov-io: # [Codecov](https://codecov.io/gh/spacetx/starfish/pull/988?src=pr&el=h1) Report
> Merging [#988](https://codecov.io/gh/spacetx/starfish/pull/988?src=pr&el=desc) into [master](https://codecov.io/gh/spacetx/starfish/commit/37c53d2f637d995fac613673eff93907cbdb3f2f?src=pr&el=desc) will **decrease** coverage by `31.12%`.
> The diff coverage is `67.7%`.

[![Impacted file tree graph](https://codecov.io/gh/spacetx/starfish/pull/988/graphs/tree.svg?width=650&token=kliQHejt6T&height=150&src=pr)](https://codecov.io/gh/spacetx/starfish/pull/988?src=pr&el=tree)

```diff
@@             Coverage Diff             @@
##           master     #988       +/-   ##
===========================================
- Coverage   88.59%   57.47%   -31.13%     
===========================================
  Files         162      162               
  Lines        5824     5916       +92     
===========================================
- Hits         5160     3400     -1760     
- Misses        664     2516     +1852
```


| [Impacted Files](https://codecov.io/gh/spacetx/starfish/pull/988?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [starfish/test/full\_pipelines/cli/test\_iss.py](https://codecov.io/gh/spacetx/starfish/pull/988/diff?src=pr&el=tree#diff-c3RhcmZpc2gvdGVzdC9mdWxsX3BpcGVsaW5lcy9jbGkvdGVzdF9pc3MucHk=) | `100% <ø> (ø)` | :arrow_up: |
| [starfish/util/click.py](https://codecov.io/gh/spacetx/starfish/pull/988/diff?src=pr&el=tree#diff-c3RhcmZpc2gvdXRpbC9jbGljay5weQ==) | `44.82% <ø> (-51.73%)` | :arrow_down: |
| [starfish/test/full\_pipelines/cli/test\_build.py](https://codecov.io/gh/spacetx/starfish/pull/988/diff?src=pr&el=tree#diff-c3RhcmZpc2gvdGVzdC9mdWxsX3BpcGVsaW5lcy9jbGkvdGVzdF9idWlsZC5weQ==) | `85.71% <ø> (-14.29%)` | :arrow_down: |
| [starfish/util/exec.py](https://codecov.io/gh/spacetx/starfish/pull/988/diff?src=pr&el=tree#diff-c3RhcmZpc2gvdXRpbC9leGVjLnB5) | `82.35% <100%> (-8.28%)` | :arrow_down: |
| [starfish/test/test\_config.py](https://codecov.io/gh/spacetx/starfish/pull/988/diff?src=pr&el=tree#diff-c3RhcmZpc2gvdGVzdC90ZXN0X2NvbmZpZy5weQ==) | `22.3% <20%> (-77.7%)` | :arrow_down: |
| [starfish/config/\_\_init\_\_.py](https://codecov.io/gh/spacetx/starfish/pull/988/diff?src=pr&el=tree#diff-c3RhcmZpc2gvY29uZmlnL19faW5pdF9fLnB5) | `82.53% <75%> (-12.11%)` | :arrow_down: |
| [sptx\_format/cli.py](https://codecov.io/gh/spacetx/starfish/pull/988/diff?src=pr&el=tree#diff-c3B0eF9mb3JtYXQvY2xpLnB5) | `75.8% <84.31%> (-8.81%)` | :arrow_down: |
| [starfish/util/config.py](https://codecov.io/gh/spacetx/starfish/pull/988/diff?src=pr&el=tree#diff-c3RhcmZpc2gvdXRpbC9jb25maWcucHk=) | `84.61% <90%> (-13.06%)` | :arrow_down: |
| [starfish/display/\_\_init\_\_.py](https://codecov.io/gh/spacetx/starfish/pull/988/diff?src=pr&el=tree#diff-c3RhcmZpc2gvZGlzcGxheS9fX2luaXRfXy5weQ==) | `0% <0%> (-100%)` | :arrow_down: |
| [starfish/test/image/test\_imagestack\_index.py](https://codecov.io/gh/spacetx/starfish/pull/988/diff?src=pr&el=tree#diff-c3RhcmZpc2gvdGVzdC9pbWFnZS90ZXN0X2ltYWdlc3RhY2tfaW5kZXgucHk=) | `13.79% <0%> (-86.21%)` | :arrow_down: |
| ... and [100 more](https://codecov.io/gh/spacetx/starfish/pull/988/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/spacetx/starfish/pull/988?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/spacetx/starfish/pull/988?src=pr&el=footer). Last update [37c53d2...63ccc36](https://codecov.io/gh/spacetx/starfish/pull/988?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

joshmoore: 31% _less_ coverage. 😱 
ambrosejcarr: > 31% _less_ coverage?! 😱

This has occurred on a few PRs. Kevin's had this too, and he only modified one file. Something is fishy. I would not worry about it. 

Patch:
diff --git a/docs/source/usage/configuration/index.rst b/docs/source/usage/configuration/index.rst
index a7b34215..ea57a410 100644
--- a/docs/source/usage/configuration/index.rst
+++ b/docs/source/usage/configuration/index.rst
@@ -22,7 +22,8 @@ If neither is true, then only the :ref:`default values <env_defaults>` will appl
 
 
 Additionally, the individual properties from the configuration JSON can be set by
-environment variable. For example:
+environment variable. These values take precedence if also set in the configuration
+file. For example:
 
 ::
 
@@ -38,7 +39,7 @@ can also be specified as:
 
     export STARFISH_VALIDATION_STRICT=true
 
-Other valid values for "true" are: "TRUE", "True", "yes", "y", and "1".
+Other valid values for "true" are: "TRUE", "True", "yes", "y", "1", "on", and "enabled".
 
 .. _env_defaults:
 
diff --git a/starfish/config/__init__.py b/starfish/config/__init__.py
index 80158c3f..46b1a997 100644
--- a/starfish/config/__init__.py
+++ b/starfish/config/__init__.py
@@ -1,8 +1,20 @@
 import os
+import warnings
 
 from starfish.util.config import Config, NestedDict
 
 
+def special_prefix(key):
+    """
+    All environment variables starting with these prefixes require
+    special handling
+    """
+    for x in ("STARFISH_", "SLICEDIMAGE_"):
+        if key.startswith(x):
+            return True
+    return False
+
+
 class environ(object):
     """Overrides environment variables (prefixed with ``STARFISH_``)
     for the duration of the call.
@@ -23,7 +35,7 @@ class environ(object):
     def __enter__(self):
         self.orig = dict()
         for k, newval in self.kwargs.items():
-            if not k.startswith("STARFISH_"):
+            if not special_prefix(k):
                 k = "STARFISH_%s" % k
             old = os.environ.get(k, None)
             try:
@@ -112,28 +124,47 @@ class StarfishConfig(object):
         """
         config = os.environ.get("STARFISH_CONFIG", "@~/.starfish/config")
         self._config_obj = Config(config)
+        self._env_keys = [
+            x for x in os.environ.keys()
+            if special_prefix(x) and x != "STARFISH_CONFIG"]
 
         # If no directory is set, then force the default
-        self._slicedimage = self._config_obj.lookup(("slicedimage",), NestedDict())
+        self._slicedimage = self._config_obj.lookup(("slicedimage",), NestedDict(), remove=True)
         if not self._slicedimage["caching"]["directory"]:
             self._slicedimage["caching"]["directory"] = "~/.starfish/cache"
         self._slicedimage_update(('caching', 'directory'))
         self._slicedimage_update(('caching', 'size_limit'), int)
 
         self._strict = self._config_obj.lookup(
-            ("validation", "strict"), self.flag("STARFISH_VALIDATION_STRICT", "false"))
+            ("validation", "strict"), self.flag("STARFISH_VALIDATION_STRICT", "false"), remove=True)
 
         self._verbose = self._config_obj.lookup(
-            ("verbose",), self.flag("STARFISH_VERBOSE", "true"))
+            ("verbose",), self.flag("STARFISH_VERBOSE", "true"), remove=True)
+
+        if self._config_obj.data:
+            warnings.warn(f"unknown configuration: {self._config_obj.data}")
+        if self._env_keys:
+            warnings.warn(f"unknown environment variables: {self._env_keys}")
 
     def _slicedimage_update(self, lookup, parse=lambda x: x):
         """ accept STARFISH_SLICEDIMAGE_ or SLICEDIMAGE_ prefixes"""
-        name = "SLICEDIMAGE_" + "_".join([x.upper() for x in lookup])
-        if name not in os.environ:
-            name = "STARFISH_" + name
-            if name not in os.environ:
-                return
-        value = parse(os.environ[name])
+
+        value = None
+
+        name1 = "SLICEDIMAGE_" + "_".join([x.upper() for x in lookup])
+        if name1 in os.environ:
+            self._env_keys.remove(name1)
+            value = parse(os.environ[name1])
+
+        name2 = "STARFISH_" + name1
+        if name2 in os.environ:
+            if value:
+                warnings.warn(f"duplicate variable: (STARFISH_){name1}")
+            self._env_keys.remove(name2)
+            value = parse(os.environ[name2])
+
+        if value is None:
+            return  # Nothing found
 
         v = self._slicedimage
         for k in lookup[:-1]:
diff --git a/starfish/util/config.py b/starfish/util/config.py
index fa05fc96..d19cbba2 100644
--- a/starfish/util/config.py
+++ b/starfish/util/config.py
@@ -56,7 +56,9 @@ class Config(object):
 
         self.data = data
 
-    def lookup(self, keys: Sequence[str], value: Any=__NO_VALUE_PASSED) -> Any:
+    def lookup(self, keys: Sequence[str],
+               value: Any=__NO_VALUE_PASSED,
+               remove: bool=False) -> Any:
         """
         Parameters
         ----------
@@ -68,10 +70,16 @@ class Config(object):
         value: Any
             Default value to return in the case that the lookup fails. An
             exception will be raised if none is provided.
+        remove: bool
+            If True, then delete the key from the object once it has been
+            looked up. If parent objects are left with no values, they will
+            be removed.
         """
         data: Any = self.data
+        stack = []
         for key in keys:
             try:
+                stack.append(data)
                 data = data.get(key)
             except (AttributeError, KeyError):
                 data = None  # Clear value
@@ -80,6 +88,13 @@ class Config(object):
         if data:
             # If we've reached here without exception,
             # then we've found the value.
+            if remove:
+                for idx in reversed(range(len(keys))):
+                    key = keys[idx]
+                    source = stack[idx]
+                    source.pop(key)
+                    if source:
+                        break
             return data
         elif value is not Config.__NO_VALUE_PASSED:
             return value


Test patch:
diff --git a/starfish/test/test_config.py b/starfish/test/test_config.py
index d921ecde..9396cef1 100644
--- a/starfish/test/test_config.py
+++ b/starfish/test/test_config.py
@@ -1,4 +1,5 @@
 import os
+import warnings
 from json import dump, loads
 
 from diskcache import Cache
@@ -74,29 +75,57 @@ def test_cache_config():
     assert cache_config["size_limit"] == 5 * 10 ** 9
 
 
+def test_cache_remove_step_wise():
+    config = Config("""{
+        "a": {
+             "b": {
+                "c": true,
+                "d": true
+             },
+             "e": 1
+         },
+         "f": 2
+    }""")
+    config.lookup(("a", "b", "c"), {}, remove=True)
+    assert "c" not in config.data["a"]["b"]
+    config.lookup(("a", "b"), {}, remove=True)
+    assert "b" not in config.data["a"]
+    config.lookup(("a",), {}, remove=True)
+    assert "a" not in config.data
+
+
+def test_cache_remove_all():
+    config = Config("""{
+        "a": {
+             "b": {
+                "c": true
+             }
+         }
+    }""")
+    config.lookup(("a", "b", "c"), {}, remove=True)
+    assert not config.data
+
+
 def test_starfish_config_value_default_key(monkeypatch):
     monkeypatch.setenv("STARFISH_CONFIG", simple_str)
     config = StarfishConfig()._config_obj
     assert config.data["a"] == 1
 
 
-@mark.parametrize("name,config", (
-    ("enabled", {
-        "expected": (2658848, 4e6),
+@mark.parametrize("name,expected,config", (
+    ("enabled", (2658848, 4e6), {
         "validation": {"strict": True},
         "slicedimage": {
             "caching": {
                 "directory": "REPLACEME",
             }}}),
-    ("disabled", {
-        "expected": (0, 0),
+    ("disabled", (0, 0), {
         "validation": {"strict": True},
         "slicedimage": {
             "caching": {
                 "size_limit": 0,
             }}}),
-    ("limited", {
-        "expected": (1e5, 3e6),
+    ("limited", (1e5, 3e6), {
         "validation": {"strict": True},
         "slicedimage": {
             "caching": {
@@ -104,7 +133,7 @@ def test_starfish_config_value_default_key(monkeypatch):
                 "size_limit": 1e5,
             }}}),
 ))
-def test_cache_merfish(tmpdir, name, config, monkeypatch):
+def test_cache_merfish(tmpdir, name, expected, config, monkeypatch):
 
     cache_enabled = (0 != config["slicedimage"]["caching"].get("size_limit", None))
     if cache_enabled:
@@ -126,7 +155,7 @@ def test_cache_merfish(tmpdir, name, config, monkeypatch):
         cache.cull()
 
     cache_size = get_size(tmpdir / "caching")
-    min, max = config["expected"]
+    min, max = expected
     assert (min <= cache_size) and (cache_size <= max)
 
 def test_starfish_config(tmpdir, monkeypatch):
@@ -158,12 +187,38 @@ def test_starfish_config(tmpdir, monkeypatch):
                  STARFISH_VALIDATION_STRICT="false")
     assert not StarfishConfig().strict
 
+def test_starfish_warn(tmpdir, monkeypatch):
+    config = {"unknown": True}
+    setup_config(config, tmpdir, monkeypatch,
+                 STARFISH_SLICEDIMAGE_CACHING_SIZE_LIMIT="1")
+    with warnings.catch_warnings(record=True) as warnings_:
+        StarfishConfig()
+        assert len(warnings_) == 1  # type: ignore
+
 def test_starfish_environ(monkeypatch):
     monkeypatch.delitem(os.environ, "STARFISH_VALIDATION_STRICT", raising=False)
     assert not StarfishConfig().strict
     with environ(VALIDATION_STRICT="true"):
         assert StarfishConfig().strict
 
+def test_starfish_environ_warn(tmpdir, monkeypatch):
+    setup_config({}, tmpdir, monkeypatch)
+    with environ(UNKNOWN="true"):
+        with warnings.catch_warnings(record=True) as warnings_:
+            StarfishConfig()
+            assert len(warnings_) == 1  # type: ignore
+
+def test_starfish_environ_warn_on_duplicate(tmpdir, monkeypatch):
+    setup_config({}, tmpdir, monkeypatch,
+                 SLICEDIMAGE_CACHING_DIRECTORY=None,
+                 STARFISH_SLICEDIMAGE_CACHING_DIRECTORY=None)
+    assert "directory" not in StarfishConfig().slicedimage
+    with environ(SLICEDIMAGE_CACHING_DIRECTORY="foo",
+                 STARFISH_SLICEDIMAGE_CACHING_DIRECTORY="bar"):
+        with warnings.catch_warnings(record=True) as warnings_:
+            assert "bar" == StarfishConfig().slicedimage["caching"]["directory"]
+            assert len(warnings_) == 1  # type: ignore
+
 #
 # HELPERS
 #