Make config read only (#472)

* Add custom read only containers and use them for the config storage

* Flake8, python2.7 compatibility and testcases for read-only containers

docs/configuration.rst

@@ -402,6 +402,14 @@ You will still get an appropriate error in the following cases:
     hide some missing value errors from you, by (unintentionally) filling them
     in from the configuration.
 
+.. note::
+    Configuration values should not be changed in a captured function
+    because those changes cannot be recorded by the sacred experiment and can
+    lead to confusing and unintended behaviour.
+    Sacred will raise an Exception if you try to write to a nested
+    configuration item. You can disable this (not recommended) by setting
+    ``SETTINGS.CONFIG.READ_ONLY_CONFIG = False``.
+
 .. _special_values:
 
 Special Values

docs/settings.rst

@@ -37,6 +37,10 @@ Here is a brief list of all currently available options.
   * ``IGNORED_COMMENTS`` *(default: ['^pylint:', '^noinspection'])*
     list of regex patterns to filter out certain IDE or linter directives
     from in-line comments in the documentation.
+  * ``READ_ONLY_CONFIG`` *(default: True)*
+    Make the configuration read-only inside of captured functions. This
+    only works to a limited extend because custom types cannot be
+    controlled.
 
 * ``HOST_INFO``
 

sacred/config/custom_containers.py

@@ -3,7 +3,7 @@
 from __future__ import division, print_function, unicode_literals
 
 import sacred.optional as opt
-from sacred.utils import join_paths
+from sacred.utils import join_paths, SacredError
 
 
 class FallbackDict(dict):
@@ -226,6 +226,80 @@ def revelation(self):
         return set()
 
 
+class ReadOnlyContainer:
+    def _readonly(self, *args, **kwargs):
+        raise SacredError(
+            self.message,
+            filter_traceback='always'
+        )
+
+
+class ReadOnlyDict(dict, ReadOnlyContainer):
+    """
+    A read-only variant of a `dict`
+    """
+    # Overwrite all methods that can modify a dict
+    clear = ReadOnlyContainer._readonly
+    pop = ReadOnlyContainer._readonly
+    popitem = ReadOnlyContainer._readonly
+    setdefault = ReadOnlyContainer._readonly
+    update = ReadOnlyContainer._readonly
+    __setitem__ = ReadOnlyContainer._readonly
+    __delitem__ = ReadOnlyContainer._readonly
+
+    def __init__(self, *args, **kwargs):
+        # Python 2.7 compatibility
+        self.message = kwargs.pop('message', None) or \
+            'This ReadOnlyDict is read-only!'
+
+        # Call dict init
+        super(ReadOnlyDict, self).__init__(*args, **kwargs)
+
+
+class ReadOnlyList(list, ReadOnlyContainer):
+    """
+    A read-only variant of a `list`
+    """
+    append = ReadOnlyContainer._readonly
+    clear = ReadOnlyContainer._readonly
+    extend = ReadOnlyContainer._readonly
+    insert = ReadOnlyContainer._readonly
+    pop = ReadOnlyContainer._readonly
+    remove = ReadOnlyContainer._readonly
+    reverse = ReadOnlyContainer._readonly
+    sort = ReadOnlyContainer._readonly
+    __setitem__ = ReadOnlyContainer._readonly
+    __delitem__ = ReadOnlyContainer._readonly
+
+    def __init__(self, *iterable, **kwargs):
+        # Python 2.7 compatibility
+        self.message = kwargs.pop('message', None) or \
+            'This ReadOnlyList is read-only!'
+
+        # Call list init
+        super(ReadOnlyList, self).__init__(*iterable)
+
+
+def make_read_only(o, error_message=None):
+    """
+    Converts every `list` and `dict` into `ReadOnlyList` and `ReadOnlyDict` in
+    a nested structure of `list`s, `dict`s and `tuple`s. Does not modify `o`
+    but returns the converted structure.
+    """
+    if isinstance(o, dict):
+        return ReadOnlyDict(
+            {k: make_read_only(v, error_message) for k, v in o.items()},
+            message=error_message)
+    elif isinstance(o, list):
+        return ReadOnlyList(
+            [make_read_only(v, error_message) for v in o],
+            message=error_message)
+    elif isinstance(o, tuple):
+        return tuple(map(make_read_only, o))
+    else:
+        return o
+
+
 SIMPLIFY_TYPE = {
     type(None): type(None),
     bool: bool,

sacred/initialize.py

@@ -9,6 +9,7 @@
 from sacred.config import (ConfigDict, chain_evaluate_config_scopes, dogmatize,
                            load_config_file, undogmatize)
 from sacred.config.config_summary import ConfigSummary
+from sacred.config.custom_containers import make_read_only
 from sacred.host_info import get_host_info
 from sacred.randomness import create_rnd, get_seed
 from sacred.run import Run
@@ -17,6 +18,7 @@
                           iterate_flattened, set_by_dotted_path,
                           recursive_update, iter_prefixes, join_paths,
                           NamedConfigNotFoundError, ConfigAddedError)
+from sacred.settings import SETTINGS
 
 
 class Scaffold(object):
@@ -162,12 +164,20 @@ def finalize_initialization(self, run):
         self.rnd = create_rnd(self.seed)
 
         for cfunc in self._captured_functions:
+            # Setup the captured function
             cfunc.logger = self.logger.getChild(cfunc.__name__)
-            cfunc.config = get_by_dotted_path(self.get_fixture(), cfunc.prefix,
-                                              default={})
             seed = get_seed(self.rnd)
             cfunc.rnd = create_rnd(seed)
             cfunc.run = run
+            cfunc.config = get_by_dotted_path(self.get_fixture(), cfunc.prefix,
+                                              default={})
+
+            # Make configuration read only if enabled in settings
+            if SETTINGS.CONFIG.READ_ONLY_CONFIG:
+                cfunc.config = make_read_only(
+                    cfunc.config,
+                    error_message='The configuration is read-only in a '
+                                  'captured function!')
 
         if not run.force:
             self._warn_about_suspicious_changes()

sacred/settings.py

@@ -20,6 +20,10 @@
         'ENFORCE_STRING_KEYS': False,
         # make sure no config key contains an equals sign
         'ENFORCE_KEYS_NO_EQUALS': True,
+        # if true, all dicts and lists in the configuration of a captured
+        # function are replaced with a read-only container that raises an
+        # Exception if it is attempted to write to those containers
+        'READ_ONLY_CONFIG': True,
 
         # regex patterns to filter out certain IDE or linter directives from
         # inline comments in the documentation

tests/test_config/test_readonly_containers.py

@@ -0,0 +1,128 @@
+import pytest
+
+from sacred.config.custom_containers import make_read_only, ReadOnlyList, \
+    ReadOnlyDict
+from sacred.utils import SacredError
+
+
+def _check_read_only_dict(d):
+    assert isinstance(d, ReadOnlyDict)
+
+    raises_dict = pytest.raises(
+        SacredError, match='This ReadOnlyDict is read-only!')
+
+    if len(d) > 0:
+        # Test removal of entries and overwrite an already present entry
+        key = list(d.keys())[0]
+
+        with raises_dict:
+            d[key] = 42
+
+        with raises_dict:
+            del d[key]
+
+        with raises_dict:
+            d.pop(key)
+
+    # Test direct writes
+    with raises_dict:
+        d['abcdefg'] = 42
+
+    # Test other functions that modify the dict
+    with raises_dict:
+        d.clear()
+
+    with raises_dict:
+        d.update({'abcdefg': 42})
+
+    with raises_dict:
+        d.popitem()
+
+    with raises_dict:
+        d.setdefault('a', 0)
+
+
+def _check_read_only_list(l):
+    assert isinstance(l, ReadOnlyList)
+
+    raises_list = pytest.raises(
+        SacredError, match='This ReadOnlyList is read-only!')
+
+    if len(l):
+        with raises_list:
+            del l[0]
+
+        with raises_list:
+            l[0] = 42
+
+        with raises_list:
+            l.pop(0)
+
+    with raises_list:
+        l.pop()
+
+    with raises_list:
+        l.clear()
+
+    with raises_list:
+        l.append(42)
+
+    with raises_list:
+        l.extend([1, 2, 3, 4])
+
+    with raises_list:
+        l.insert(0, 0)
+
+    with raises_list:
+        l.remove(1)
+
+    with raises_list:
+        l.sort()
+
+    with raises_list:
+        l.reverse()
+
+
+def test_readonly_dict():
+    d = dict(a=1, b=2, c=3)
+    d = make_read_only(d)
+    _check_read_only_dict(d)
+
+
+def test_nested_readonly_dict():
+    d = dict(a=1, b=dict(c=3))
+    d = make_read_only(d)
+    _check_read_only_dict(d)
+    _check_read_only_dict(d['b'])
+
+
+def test_readonly_list():
+    l = [1, 2, 3, 4]
+    l = make_read_only(l)
+    _check_read_only_list(l)
+
+
+def test_nested_readonly_list():
+    l = [1, [2, [3, [4]]]]
+    l = make_read_only(l)
+    _check_read_only_list(l)
+    _check_read_only_list(l[1])
+    _check_read_only_list(l[1][1])
+    _check_read_only_list(l[1][1][1])
+
+
+def test_nested_readonly_containers():
+    container = ([0, [], {}, ()], {0: (), 1: [], 2: {}})
+    container = make_read_only(container)
+    _check_read_only_list(container[0])
+    _check_read_only_list(container[0][1])
+    _check_read_only_dict(container[0][2])
+    _check_read_only_dict(container[1])
+    _check_read_only_dict(container[1][2])
+    _check_read_only_list(container[1][1])
+
+    # Check explicitly for tuple (and not isinstance) to be sure that the type
+    # is not altered
+    assert type(container) == tuple
+    assert type(container[0][3]) == tuple
+    assert type(container[1][0]) == tuple

tests/test_experiment.py

@@ -11,7 +11,8 @@
 import sys
 
 from sacred.experiment import Experiment
-from sacred.utils import apply_backspaces_and_linefeeds, ConfigAddedError
+from sacred.utils import apply_backspaces_and_linefeeds, ConfigAddedError, \
+    SacredError
 
 
 @pytest.fixture
@@ -290,3 +291,56 @@ def foo():
 
     r = ex.run()
     assert r.config['a'] == 'me'
+
+
+def test_fails_on_config_write(ex):
+    @ex.config
+    def cfg():
+        a = 'hello'
+        nested_dict = {'dict': {'dict': 1234, 'list': [1, 2, 3, 4]}}
+        nested_list = [{'a': 42}, (1, 2, 3, 4), [1, 2, 3, 4]]
+        nested_tuple = ({'a': 42}, (1, 2, 3, 4), [1, 2, 3, 4])
+
+    @ex.main
+    def main(_config, nested_dict, nested_list, nested_tuple):
+        raises_list = pytest.raises(
+            SacredError, match='The configuration is read-only in a captured function!')
+        raises_dict = pytest.raises(
+            SacredError, match='The configuration is read-only in a captured function!')
+
+        print('in main')
+
+        # Test for ReadOnlyDict
+        with raises_dict:
+            _config['a'] = 'world!'
+
+        with raises_dict:
+            nested_dict['dict'] = 'world!'
+
+        with raises_dict:
+            nested_dict['list'] = 'world!'
+
+        with raises_dict:
+            nested_dict.clear()
+
+        with raises_dict:
+            nested_dict.update({'a': 'world'})
+
+        # Test ReadOnlyList
+        with raises_list:
+            nested_dict['dict']['list'][0] = 1
+
+        with raises_list:
+            nested_list[0] = 'world!'
+
+        with raises_list:
+            nested_dict.clear()
+
+        # Test nested tuple
+        with raises_dict:
+            nested_tuple[0]['a'] = 'world!'
+
+        with raises_list:
+            nested_tuple[2][0] = 123
+
+    ex.run()

tests/test_modules.py

@@ -204,6 +204,7 @@ def test_double_nested_config():
     @ex.config
     def config():
         a = 1
+        seed = 42
 
     @ing.config
     def config():
@@ -240,12 +241,12 @@ def ing_main(_config):
 
     @ex.main
     def main(_config):
-        _config.pop('seed')
         assert _config == {
             'a': 1,
             'sub_sub_ing': {'d': 3},
             'sub_ing': {'c': 2},
-            'ing': {'b': 1}
+            'ing': {'b': 1},
+            'seed': 42
         }, _config
 
         ing_main()