Fix NameError when a pasted function attempts to access a variable in its outer scope

[fofoni]

Motivation

The motivation is the variable scope problem mentioned in the How I animate 3Blue1Brown | A Manim demo with Ben Sparks video.

This is what's going on:

>>> source_code = """
... x = 9
... def f():
...   print(x)
... f()
... """
>>> namespace = {}
>>> exec(source_code, namespace, namespace)
9
>>> exec(source_code, {}, {})
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<string>", line 5, in <module>
  File "<string>", line 4, in f
NameError: name 'x' is not defined
>>> 

When you run exec (or eval), those two last arguments are a globals namespace and a locals namespace. The x = 9 line modifies the locals namespace, but the print(x) line inside the function reads from the globals namespace. We need them to be the same.

When IPython runs our code, it calls exec/eval like this:

if async_:
    await eval(code_obj, self.user_global_ns, self.user_ns)
else:
    exec(code_obj, self.user_global_ns, self.user_ns)

In a vanilla ipython shell, these two attributes are the same dict instance:

$ ipython
Python 3.12.5 (main, Aug 11 2024, 21:06:20) [GCC 13.2.1 20230801]
Type 'copyright', 'credits' or 'license' for more information
IPython 8.26.0 -- An enhanced Interactive Python. Type '?' for help.

In [1]: shell = get_ipython()

In [2]: shell.user_global_ns is shell.user_ns is globals() is locals()
Out[2]: True

Our problem is that in an IPython shell acquired via InteractiveShellEmbed.instance(), these two namespaces are distinct, so the "function can't access outer scope variables" issue will always happen. (And no, nonlocal doesn't help -- nonlocal is only for nested functions.)

Directly instantiating a new shell is, I believe, the best way to avoid this problem. We can see here that the user_module and user_ns arguments are forwarded to InteractiveShellEmbed.init_create_namespaces, which will set the attributes shell.user_module (from which shell.user_global_ns takes the global namespace) and shell.user_ns, and also set shell.default_user_namespaces to False. Calling init_create_namespaces directly on an already-instantiated shell doesn't seem to be a good idea because of this.

Proposed changes

The order was:

• Acquire a shell instance via InteractiveShellEmbed.instance().
• Instantiate a new local_ns dict as a copy of caller_frame.f_locals.
• Add shortcuts to local_ns.
• Instantiate a new module object via get_module(caller_frame.f_globals["__file__"]), which will be the shell's global namespace.
• Call shell passing local_ns and the module object.

This is the new order:

• Get the module object (with the very same incantation).
• Instantiate a new shell object with InteractiveShellEmbed(user_module=module). This guarantees shell.user_global_ns is shell.user_ns.
• As before, instantiate local_ns as a copy of the caller locals.
• As before, update local_ns with the shortcuts.
• Update the module object's global namespace with everything from local_ns. This is not a problem, because this module object was instantiated exactly for being used as the shell's global ns, and, inside the shell scope, local_ns would shadow the module globals anyway. Also, module objects are never copied, so updating this instance here (after having already instantiated the shell using the module) does update the namespace that will be seen by the shell.
• Call shell() without any parameters:
  • local_ns: is already captured bt the updated module globals.
  • stack_depth: is not used, because shell.default_user_namespaces is False.
  • module: was already mentioned when we instantiated the shell.

Test

I tested my MWE, but I didn't test this fix in manim itself. I will remove the draft status from the PR after I have made sure that this fix actually helps manim.

[fofoni]

Tested on manim, looks like it works! (:

master branch: (omitting logs and other verbose stuff)

$ manimgl example_scenes.py OpeningManimExample -e
ManimGL v1.6.1
Exception reporting mode: Verbose
Python 3.12.5 (main, Aug 11 2024, 21:06:20) [GCC 13.2.1 20230801]
IPython 8.28.0 -- An enhanced Interactive Python. Type '?' for help.

In [1]: globals() is locals()
Out[1]: False

In [2]: new_text = TexText("Reproducing the issue on the master branch...")

In [3]: def make_blue():
   ...:     new_text.set_color(BLUE)
   ...: 

In [4]: make_blue()
NameError: name 'new_text' is not defined

In my branch:

$ manimgl example_scenes.py OpeningManimExample -e
ManimGL v1.6.1
Exception reporting mode: Verbose
Python 3.12.5 (main, Aug 11 2024, 21:06:20) [GCC 13.2.1 20230801]
IPython 8.28.0 -- An enhanced Interactive Python. Type '?' for help.

In [1]: globals() is locals()
Out[1]: True

In [2]: new_text = TexText("Testing the fix...")
                       
In [3]: def make_blue():
   ...:     new_text.set_color(BLUE)
   ...: 

In [4]: make_blue()

In [5]: self.play(Write(new_text))
Out[5]: <example_scenes_insert_embed.OpeningManimExample at 0x75dc040e7a70>

In the last line, the self variable is from the scope of the Scene.construct method, so looks like everything's working fine.

[3b1b]

Thank you, that’s a helpful explanation. For the use case of embedded animation, having globals match locals seems fine, but in making it more systematic, I’m inclined to ask what kind of unintended consequences could arise from those dictionaries always matching.

if this is the state of affairs in vanilla ipython, the answer may well simply be “none”.

[3b1b]

It works well for me, thanks!

[fofoni]

For the use case of embedded animation, having globals match locals seems fine, but in making it more systematic, I’m inclined to ask what kind of unintended consequences could arise from those dictionaries always matching.

I didn't understand exactly in what other environment this would happen, besides embedded animation. The fix only affects Scene.embed, so I thought it would only affect the use case of dropping the user into a shell for interacting with the animation. (Which by the way, I didn't say that because I wanted to focus on the scope issue, but this little interactive shell integrated with the animation is absolutely amazing, I'm completely in love with playing with that haha)

if this is the state of affairs in vanilla ipython, the answer may well simply be “none”.

Yeah, this is normal Python execution, I wouldn't worry about that. What I think that could be something to worry about is the fact that we're flattening two namespaces in one, and this could lose information. E.g., say you had a nested function that for some reason iterates through globals(), and (for whatever reason) say this function depends on not seeing the locals defined inside Scene.construct. Well, now this function can't distinguish anymore between globals() and locals(). Or, maybe a less improbable example, say you had a local inside your Scene.construct named some_name, and a global in the module scope with the same some_name, and say your nested function wants to interact with the global some_name. Then previously you could just say global some_name, but now the local some_name will have overridden the global variable with the same name, and you will have lost access to that global inside the interactive shell.

These are the kind of things that I think could go wrong, but (and now this is totally personal) I think that having the nested functions completely disconnected from the "locals()" namespace is worse.

Anyway, I'm glad that it worked for you!

[3b1b]

I didn't understand exactly in what other environment this would happen, besides embedded animation.

You're right that's the only environment this is meant for, I was just throwing out the question of what unintended consequences there could be within that environment.

The potentials you list out associated with making both namespaces into one feel like they would only arise in pathological situations, so I'm not too worried there. As you mentioned, it's way better than the previous state of affairs.

I'm glad you're enjoying playing with it!