IDSIA / sacred

Sacred is a tool to help you configure, organize, log and reproduce experiments developed at IDSIA.
MIT License
4.26k stars 383 forks source link

Problem with square brackets generators in config function #306

Open Mehanik opened 6 years ago

Mehanik commented 6 years ago

Initially I found this problem when tried to execute https://github.com/keras-team/keras-contrib/blob/master/keras_contrib/datasets/coco.py

Investigating this error I found that slightly modified basic example

from sacred import Experiment

ex = Experiment('hello_config')

@ex.config
def my_config():
    recipient = "world"
    message = "Hello %s!" % recipient
    [print(message, i) for i in range(10)]

@ex.automain
def my_main(message):
    print(message)

cause error:

NameError: name 'message' is not defined

Ubuntu 18.04 Python 3.6.5

Qwlouse commented 6 years ago

Wow, that is a strange issue! It seems that list-comprehensions (and all other comprehensions actually) somehow fail to access the custom locals dictionary. Thanks for reporting this! I'll have to look into it in more detail after my vacation.

absolutelyNoWarranty commented 6 years ago

I did some digging and I believe I found the problem. In Py3 list comprehensions were changed so that their scope is just like a function scope. So for example using this config will also fail.

@ex.config
def my_config():
    recipient = "world"
    def make_message():
        return "Hello %s!" %recipient
    message = make_message()

I messed around with the code and managed to fix it in a hacky way which passes all tests but I'm not sure exactly why it works. I can submit a PR if you want.

diff --git a/sacred/config/config_scope.py b/sacred/config/config_scope.py
index ece27af..96e6ae7 100644
--- a/sacred/config/config_scope.py
+++ b/sacred/config/config_scope.py
@@ -73,7 +73,11 @@ class ConfigScope(object):
                 fallback_view[arg] = fallback[arg]

         cfg_locals.fallback = fallback_view
-        eval(self._body_code, copy(self._func.__globals__), cfg_locals)
+        g = copy(self._func.__globals__)
+        cfg_locals.update(g)
+        eval(self._body_code, cfg_locals)
+        for key in g:
+            del cfg_locals[key]
Qwlouse commented 6 years ago

This is a pretty strange issue indeed. Thanks @absolutelyNoWarranty for digging in a bit further. Based on your comment I have run a few tests, and it seems like comprehensions within the exec can access only the globals and not locals.

body = '''
a = 8
b = [1, 2, 3]
assert 'a' not in globals(), 'global'
print([a for _ in b])
'''

exec(body, {}, {})  # NameError: name 'a' is not defined
exec(body, {})  # AssertionError: global

Interestingly, accessing b works just fine, it is only about a, and in Python2 it also runs.

The bug seems to be known and to be the same as what happens in a class definition: https://bugs.python.org/issue3692 So this actually raises the same NameError:

class Foo:
    a = 8
    b = [1, 2, 3]
    print([a for _ in b])

From the discussion in the above issue, this is a rather complex problem, and I am unsure when and if it is going to be resolved. So we probably need to find a workaround.

Regarding your solution: It works since you only pass globals that contain everything. But with the copying and deleting it will be rather fragile. If you for example define a config variable that shadows (has the same name as) a global variable, it will be deleted in your solution.

A better fix would be to use a fallback dictionary that provides read access to globals but stores to locals. Unfortunately the docs for exec say that globals have to be a dictionary (not just a mapping). Locals can be any mapping, so it might be worth a shot to make locals a fallback dictionary. But I haven't tested any of this.

absolutelyNoWarranty commented 6 years ago

According to Stack Overflow:

Before python 3.3, you cannot use a custom dict subclass for the globals value of an exec statement.

So passing in only one custom mapping as globals to exec won't work for all Python versions.

As for passing in both globals and locals to exec:

class ScopeOfExec:
    a = 8
    # passed in locals is updated with 'a'
    # passed in globals is NOT updated ... unless passed in locals did some special magic
    def func_inside_config_function():
        # can see globals but 'a' is not in globals
        # new empty locals, cannot see the original passed in locals

so this means the custom mapping that serves as exec 's locals needs to update the passed in globals as well so that inside the inner scope it can see any updates made in the outer scope Can Fallback dict do this already?

ArcCha commented 4 years ago

I've encountered this issue recently, when I tried to use list comprehension in my config. I've found a workaround on stackoverflow. So instead of

samples_ratio = [x * ((cv - 1) / cv) for x in [1/3, 2/3, 1.0]]

I have

cv = 5
def temp(cv):
    return [x * ((cv - 1) / cv) for x in [1/3, 2/3, 1.0]]
samples_ratio = temp(cv)

which achieves what I want, but it stores temp unnecessarily. Is there a better way? Or maybe I can help?

Qwlouse commented 4 years ago

No clean solution unfortunately, but you do hav a few choices:

  1. You can move temp outside the config function
  2. You can delete temp later:
    cv = 5
    def temp(cv):
    return [x * ((cv - 1) / cv) for x in [1/3, 2/3, 1.0]]
    samples_ratio = temp(cv)
    del temp
  3. you can use a regular for loop:
    cv = 5
    samples_ratio = []
    for x in [1/3, 2/3, 1.0]:
    samples_ratio.append(x * ((cv - 1) / cv))
    del x # note that you will have to delete x