Catch missing classes early when loading config files

[ernestum]

Fixes #901

[thequilo]

Thank you for spotting and fixing this issue! I agree that this should be set to "error" here. Anything else results in unexpected behavior.

Do you know what the behavior was like before jsonpickle introduced the on_missing argument? If it was "ignore", then this fix could break some previously running code.

[ernestum]

Before jsonpickle would just leave the py/type keys in the dict and not replace it with an instance of the class.
This partially deserialized dictionary is then passed to

sacred/sacred/config/utils.py

Line 84 in f7311fc

def normalize_or_die(obj):

and finally crashes with a KeyError here:

sacred/sacred/config/utils.py

Line 52 in f7311fc

raise KeyError(

[ernestum]

TL/DR: I would still recommend applying the changes suggested by this PR. They won't change the behavior of scared but they will improve error messages for missing classes.

Upon further investigation the issue was a different one. We used the py/type key where we should have used the py/object key (see here: HumanCompatibleAI/imitation#664)
When the py/object key is used there is actually no difference between the jsonpickle versions.

This example demonstrates the different behaviors

import jsonpickle

config_with_unkown_class = """{
    "beta_schedule": {
      "decay_probability": 0.7,
      "py/object": "imitation.algorithms.dagger.ExponentialBetaSchedule"
    }
}"""

config_with_known_class = """{
    "beta_schedule": {
      "py/object": "imitation.algorithms.dagger.LinearBetaSchedule",
      "rampdown_rounds": 15
    }
}"""

print("Valid:")
print(jsonpickle.decode(config_with_known_class, keys=True))
print()

print("Invalid:")
print(jsonpickle.decode(config_with_unkown_class, keys=True))
print()

print("Invalid:")
print(jsonpickle.decode(config_with_unkown_class, keys=True, on_missing="error"))

generating the following output:

Valid:
{'beta_schedule': <imitation.algorithms.dagger.LinearBetaSchedule object at 0x7f90c676ad30>}

Invalid:
{'beta_schedule': {'decay_probability': 0.7, 'py/object': 'imitation.algorithms.dagger.ExponentialBetaSchedule'}}

Invalid:
Traceback (most recent call last):
  File "/home/m/Documents/CHAI/venv/lib/python3.8/site-packages/jsonpickle/unpickler.py", line 718, in _restore_object_instance
    instance = cls(*args)
TypeError: 'module' object is not callable

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/m/Documents/CHAI/venv/lib/python3.8/site-packages/jsonpickle/unpickler.py", line 721, in _restore_object_instance
    instance = make_blank_classic(cls)
  File "/home/m/Documents/CHAI/venv/lib/python3.8/site-packages/jsonpickle/unpickler.py", line 254, in make_blank_classic
    instance.__class__ = cls
TypeError: __class__ must be set to a class, not 'module' object

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/m/.config/JetBrains/PyCharmCE2022.3/scratches/scratch_8.py", line 26, in <module>
    print(jsonpickle.decode(config_with_unkown_class, keys=True, on_missing="error"))
  File "/home/m/Documents/CHAI/venv/lib/python3.8/site-packages/jsonpickle/unpickler.py", line 85, in decode
    return context.restore(data, reset=reset, classes=classes)
  File "/home/m/Documents/CHAI/venv/lib/python3.8/site-packages/jsonpickle/unpickler.py", line 355, in restore
    value = self._restore(obj)
  File "/home/m/Documents/CHAI/venv/lib/python3.8/site-packages/jsonpickle/unpickler.py", line 337, in _restore
    return restore(obj)
  File "/home/m/Documents/CHAI/venv/lib/python3.8/site-packages/jsonpickle/unpickler.py", line 783, in _restore_dict
    data[k] = self._restore(v)
  File "/home/m/Documents/CHAI/venv/lib/python3.8/site-packages/jsonpickle/unpickler.py", line 337, in _restore
    return restore(obj)
  File "/home/m/Documents/CHAI/venv/lib/python3.8/site-packages/jsonpickle/unpickler.py", line 756, in _restore_object
    return self._restore_object_instance(obj, cls, class_name)
  File "/home/m/Documents/CHAI/venv/lib/python3.8/site-packages/jsonpickle/unpickler.py", line 723, in _restore_object_instance
    self._process_missing(class_name)
  File "/home/m/Documents/CHAI/venv/lib/python3.8/site-packages/jsonpickle/unpickler.py", line 547, in _process_missing
    raise errors.ClassNotFoundError(
jsonpickle.errors.ClassNotFoundError: Unpickler.restore_object could not find imitation.algorithms.dagger.ExponentialBetaSchedule!

This error message at least explains what class could not be found.
With the current implementation we would just pass the dict with py/object keys to normalize_or_die() resulting in an error message like this, which is far less readable in my opinion:

Traceback (most recent call last):
  File "/home/m/Documents/CHAI/venv/lib/python3.8/site-packages/IPython/core/interactiveshell.py", line 3442, in run_code
    exec(code_obj, self.user_global_ns, self.user_ns)
  File "<ipython-input-2-1e778b2439c0>", line 1, in <module>
    runfile('/home/m/.config/JetBrains/PyCharmCE2022.3/scratches/scratch_8.py', wdir='/home/m/.config/JetBrains/PyCharmCE2022.3/scratches')
  File "/home/m/misc/pycharm-community-2022.2.3/plugins/python-ce/helpers/pydev/_pydev_bundle/pydev_umd.py", line 198, in runfile
    pydev_imports.execfile(filename, global_vars, local_vars)  # execute the script
  File "/home/m/misc/pycharm-community-2022.2.3/plugins/python-ce/helpers/pydev/_pydev_imps/_pydev_execfile.py", line 18, in execfile
    exec(compile(contents+"\n", file, 'exec'), glob, loc)
  File "/home/m/.config/JetBrains/PyCharmCE2022.3/scratches/scratch_8.py", line 23, in <module>
    normalize_or_die(jsonpickle.decode(config_with_unkown_class, keys=True))
  File "/home/m/Documents/CHAI/venv/lib/python3.8/site-packages/sacred/config/utils.py", line 89, in normalize_or_die
    res[key] = normalize_or_die(value)
  File "/home/m/Documents/CHAI/venv/lib/python3.8/site-packages/sacred/config/utils.py", line 88, in normalize_or_die
    assert_is_valid_key(key)
  File "/home/m/Documents/CHAI/venv/lib/python3.8/site-packages/sacred/config/utils.py", line 52, in assert_is_valid_key
    raise KeyError(
KeyError: 'Invalid key "py/object". Config-keys cannot be one of thereserved jsonpickle tags: {\'py/tuple\', \'py/iterator\', \'py/state\', \'py/newargs\', \'py/function\', \'py/reduce\', \'py/seq\', \'py/newargsex\', \'py/id\', \'py/repr\', \'py/ref\', \'py/object\', \'py/newobj\', \'py/type\', \'py/set\', \'py/initargs\', \'py/bytes\'}'

Btw this is an example where somebody drew wrong conclusions based on the obscure error message: HumanCompatibleAI/imitation#653
Instead of hunting for the missing class, they replaced py/object with py/type which, by chance, made the error go away.

[thequilo]

Thank you for the thorough analysis! I agree with you, the new error message is much better.