Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow function definition locals() to be modified in the shell #571

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

asmeurer
Copy link
Collaborator

This still requires more testing, especially for the different shells.

For the IPython shell, this requires not making a copy of locals(), but this also causes the IPython shell to pollute the locals() namespace with all its internal variables (like _0 and so on), so a better fix is needed.

Fixes #26.

This still requires more testing, especially for the different shells.

For the IPython shell, this requires not making a copy of locals(), but this
also causes the IPython shell to pollute the locals() namespace with all its
internal variables (like _0 and so on), so a better fix is needed.

Fixes inducer#26.
This also causes IPython to inject all its magic variables (_, _0,
get_ipython, etc.) into the locals namespace every time it is started. Unless
there is some way to disable this, this is not worth this functionality, since
it pollutes the variables view and also would break any code that happens to
use those same variable names.
@asmeurer
Copy link
Collaborator Author

Probably going to just revert the change to the IPython shell, meaning this won't work there (but it will work in the inline shell). Unless IPython has a method to turn this off, we would have to manually remove them every time IPython is started. It would also break any code that happens to use the same variable names as the ones IPython injects (which includes _, which is not uncommon).

@asmeurer
Copy link
Collaborator Author

asmeurer commented Nov 17, 2022

So based on some (very basic) testing:

  • Internal shell and classic shell work great. Even though they both use the _ magic variable, they don't seem to actually inject it into the locals.
    • Only downside of the internal shell is that variables aren't updated automatically in the variables view until you step the debugger. This should be fixable by making it update whenever a command is executed. EDIT: fixed
  • IPython would work if we remove that copy() line, but that also makes it inject all the IPython magic variables every time it is started (of which there are a lot).
  • IPython kernel works and also injects the magic variables. I will probably fix this to also copy() like IPython. As an aside, IPython kernel seems to be broken for me. When I exit the kernel, pudb restarts in a broken terminal state, which can only be fixed by exiting pudb and typing the reset command.
  • bpython works, but injects the help variable into the locals. I'm surprised that's the only variable it injects. This is probably fine to just leave alone
  • ptpython doesn't work. It would work if we removed the copy() line, but this would also make it inject its magic variables (only _0 and so on).
  • ptipython doesn't work at all. As in, it's broken even in main. When you try to access a local variable in ptpython, it doesn't see it. I don't know why because it looks like locals is being passed through correctly, so it seems like the problem is likely upstream. Does anyone even use ptipython? It seems defunct now that IPython itself uses prompt-toolkit, which has been the case for a while.

We'll probably want some way to disable this behavior, as you might not actually want the variables you type in the shell to inject themselves into your code. Although note that this is already the behavior at the module level because writing to globals() works just fine. We could easily make it not do that there by copying globals before passing it to the shell. IMO we should do that, so that the option is either "shell edits variables" which causes it to edit both globals and locals, or "shell doesn't edit variables" which causes the shells to always get a copy of globals and locals. That way the behavior is effectively the same whether you are at the module level or inside a function.

What should the config look like? We can force it to work with any of the shells, even the ones that inject a lot of variables. So what should the default look like? Ideally for the internal and classic shells it would be enabled by default and for IPython it should be disabled by default.

@asmeurer
Copy link
Collaborator Author

This could probably use some more testing, but for now, the main thing I know is missing is a configuration option, which I'd like feedback on how to best do (see my previous comment). Should it just default to the behavior being on (or off) or try to set a "best guess" default depending on which shell is enabled? Or should it default to on but only actually work in the shells that don't inject variables (the rest would always copy locals() to prevent it from working).

CC @inducer

@inducer
Copy link
Owner

inducer commented Nov 20, 2022

  • ptipython doesn't work at all. As in, it's broken even in main. When you try to access a local variable in ptpython, it doesn't see it. I don't know why because it looks like locals is being passed through correctly, so it seems like the problem is likely upstream. Does anyone even use ptipython? It seems defunct now that IPython itself uses prompt-toolkit, which has been the case for a while.

I would be OK with just removing ptipython.

setting

I would vote for keeping this simple to start. Either propagate or don't. And if ipython is being used with "yes, propagate", maybe print a one-line warning every time it starts, stating that IPython spews copious amounts of variables.

@@ -327,6 +327,7 @@ def set_frame_index(self, index):
return

self.curframe, lineno = self.stack[index]
self.curframe_locals = self.curframe.f_locals
Copy link
Owner

@inducer inducer Nov 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a comment explaining what it's good for? TBH, I'm not sure I understand why the proposed changes have the described effect. (at least with just looking at the diff)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might try to dig deeper to figure out exactly what is going on. From what I can tell, as long as you reuse the same locals object, any modifications you make to it are reflected when the frame is run. But if you access frame.f_locals again, it recomputes the locals and resets them. I'm unclear exactly how it works, but it does.

I'm actually unclear why your example at #26 (comment) didn't work for you. I tested it and even in Python 2, it has the desired effect when you set i on the given line.

Copy link
Owner

@inducer inducer Nov 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, you've nerd-sniped me. Grr. 🙂

Here's what I've found so far:

Some notes:

  • Interestingly, it seems that all this code was significantly rewritten for 3.12.
  • There also seem to have been some changes here for 3.11. Search for PyFrame_LocalsToFast here.

Whatever we do here, we should make sure to preserve a summary of it in comments, so that we don't have to rediscover it some other time.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found https://peps.python.org/pep-0558/, which mentions this behavior (footnote 3), although the link there is broken.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That PEP is very technical, but I think what happens is:

  • locals is designed to be snapshotted. That's why when you do something like
    def f():
        x = 1
        locals()['x'] = 2
        print(x)
    
    f() 
    it prints 1 instead of 2.
  • frame.f_locals is designed to be a write-through proxy (probably exactly for this feature).
  • What I think happens is that the descriptor for f_locals computes this write-through proxy from a snapshot. That means that when you access it, it recomputes it from what it was, and throws away whatever was there before. Since it's a write-through proxy, this effectively resets the locals in the frame.

But I'm not completely sure about this. Honestly I think a core dev would need to confirm the details. I think I'll just cross-reference this PEP in the comments since that should be a good jumping off point for anyone who is interested in what is going on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Changing runtime state
2 participants