Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.4k
gh-74929: Implement PEP 667#115153
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
gh-74929: Implement PEP 667#115153
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks so much for doing this.
I've a few comments, but nothing major.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
carljm commentedFeb 9, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Thanks for doing this! I was hoping this would get done for 3.13 but hadn't found time to do it yet.
EDIT: on second thought, I realized the problem is likely that |
Yes, that's why the test fails. The test is |
I cleaned up the code a bit to address the review and added the GC part in. This is still in draft state, which only aims to demostrate the possibility of PEP 667. There's plenty of work to be done before this can be merged in. The current goal is to make it enough for the PSF to decide whether to accept PEP 667. Let me know if there's still missing pieces (like do we want to actually remove all the FAST/LOCAL conversion calls to prove the point). |
Removing the fast locals <--> slow locals conversion is key to fixing#74929. I think that is worth doing before submitting PEP 667 to the SC. @gaogaotiantian since you are doing most of the work on this, would you like to be added as co-author of PEP 667? |
Sure, I'm happy to be listed as the co-author. I'll work on the fast vs local thing and check if#74929 is fixed. |
FWIW the discussion has been started athttps://discuss.python.org/t/pep-667-consistent-views-of-namespaces/46631. We now need at least the following updates to the PEP:
|
I've already added myself to the list of authors with Mark's guidance. Let me know if you want me to make the PR to update the PEP. |
Go right ahead. |
@markshannon okay I made all fast/local related functions no-op. It did not break anything new and#74929 (comment) is not an issue anymore. The original test case in#74929 has been accidentally fixed in 3.11, is there any repro case that I can test against? |
Is there a reason you're leaving the failing tests as failing? I think it is easier for reviewers to evaluate the compatibility impact on tests by seeing the required changes to tests in the PR, rather than by having to go look at the CI test results. |
The current implementation is just a prototype and far from being done. I can make some modifications to the test so they can pass with the change if that's preferred. |
We have two more months until feature freeze. If we want to land this on 3.13, we probably should push it forward soon. |
How is the Discourse discussion on this topic going? Is it substantial enough to send the PEP to the SC? Do we need to make any PEP updates first? |
I made a PR to the PEP and it's approved but not merged (I think Mark has to review and approve it?), if you meant the link changes mentioned before. I'm not sure if the discussion thread attracts enough attention. What point should we get for the Disclosure discussion? |
Uh oh!
There was an error while loading.Please reload this page.
markshannon commentedMar 5, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
We need to decide which mapping methods we want Dunder methods
I think we need to support all the above, except pickling, and that Normal methods
Methods that remove items are tricky as we cannot properly delete fast locals; we just set them to |
We don't need to implement them all before submitting the PEP though, just decide on which methods to support. |
Should we follow some standard for the methods we should implement? The original PEP mentioned that we should do a For the remove related methods - should the debugger be able to decref the local variable? Just like |
gaogaotiantian commentedMay 2, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Okay I resolved most of the comments, and I believe there's one thing left here - comprehensions. I'm just going to jump to my proposal and see if that makes sense. If not, we can discuss it more. importsysdeff():x=1 [print(sys._getframe().f_locals)forain [0]]print([sys._getframe().f_localsforain [0]][0])print(sys._getframe().f_locals)f() will print
Notice all the values above are proxies. importsysclassC:x=1 [print(sys._getframe().f_locals)forain [0]]print([sys._getframe().f_localsforain [0]][0])print(sys._getframe().f_locals) will print (ignore
An alternative would be:
The latter seems more consistent with the function level result, and it makes Maybe we could stick to a simple rule here - However, this will break the backwards compatibility for |
I think that proposal looks reasonable. I think your first proposal for the class behavior is better. Comprehensions in class scope cannot see class-level variables (as if the comprehension were still a function, since functions defined in a class scope can't see variables in the enclosing class scope either), so those class variables should not show up when |
gaogaotiantian commentedMay 2, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
But we will probably treat module-level the same as class-level. Do you think it's reasonable to have a module-level comprehension to return a proxy without the global variables? edit: I guess that makes sense as well. Even in a function in a module, |
Okay I finished the last piece and cleaned up the code a bit. Now we have some very consistent behavior for
This breaks the behavior introduced by pep 709 that
Let me know if there are missing pieces that I don't know of. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This looks good. Thanks for your perseverance.
last chance for comments.
I'm going to merge this in 12 hours or so, if no one objects.
…te locals()"PEP 667: Consistent views of namespaces" caused locals() to beinconsistent between uses since it is now created afresh every time youinvoke it and writes to it are dropped. `sys._getframe().f_locals` isequivalent but preserves writes (it doesn't create a new dict) andunfortunately doesn't help at all as it's documented to be a privateimplementation detail of CPython that "should be used for internal andspecialized purposes only".Work around this by saving locals to a variable reference and bothpassing it into runctx and reusing it in lookups of the result. Thisworks okay for both new and older versions of python.Bug:python/cpython#115153
…te locals()"PEP 667: Consistent views of namespaces" caused locals() to beinconsistent between uses since it is now created afresh every time youinvoke it and writes to it are dropped. `sys._getframe().f_locals` isequivalent but preserves writes (it doesn't create a new dict) andunfortunately doesn't help at all as it's documented to be a privateimplementation detail of CPython that "should be used for internal andspecialized purposes only".Work around this by saving locals to a variable reference and bothpassing it into runctx and reusing it in lookups of the result. Thisworks okay for both new and older versions of python.Per the documentation for locals():> The contents of this dictionary should not be modified; changes may> not affect the values of local and free variables used by the> interpreter.So... lesson learned? :) This was introduced in commitc34ee37; before that, we still usedlocals() but only to pass local variables *in*.Bug:python/cpython#115153
henryiii commentedMay 23, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
FYI, docs are missing here:https://docs.python.org/3.13/c-api/reflection.html The back-compat functions were causing a few destructors to not be called properly in three tests in pybind11; switching to the new functions fixed it, though. Not sure if that's expected. I believe it’s because the back compat function for eval frame locals is basically the same as the new function and returns a new reference. |
…te locals()"PEP 667: Consistent views of namespaces" caused locals() to beinconsistent between uses since it is now created afresh every time youinvoke it and writes to it are dropped. `sys._getframe().f_locals` isequivalent but preserves writes (it doesn't create a new dict) andunfortunately doesn't help at all as it's documented to be a privateimplementation detail of CPython that "should be used for internal andspecialized purposes only".Work around this by saving locals to a variable reference and bothpassing it into runctx and reusing it in lookups of the result. Thisworks okay for both new and older versions of python.Per the documentation for locals():> The contents of this dictionary should not be modified; changes may> not affect the values of local and free variables used by the> interpreter.So... lesson learned? :) This was introduced in commitc34ee37; before that, we still usedlocals() but only to pass local variables *in*.Bug:python/cpython#115153
…te locals()"PEP 667: Consistent views of namespaces" caused locals() to beinconsistent between uses since it is now created afresh every time youinvoke it and writes to it are dropped. `sys._getframe().f_locals` isequivalent but preserves writes (it doesn't create a new dict) andunfortunately doesn't help at all as it's documented to be a privateimplementation detail of CPython that "should be used for internal andspecialized purposes only".Work around this by saving locals to a variable reference and bothpassing it into runctx and reusing it in lookups of the result. Thisworks okay for both new and older versions of python.Per the documentation for locals():> The contents of this dictionary should not be modified; changes may> not affect the values of local and free variables used by the> interpreter.So... lesson learned? :) This was introduced in commitc34ee37; before that, we still usedlocals() but only to pass local variables *in*.Bug:python/cpython#115153
Uh oh!
There was an error while loading.Please reload this page.
This is a prototype implementation of PEP 667.
All the examples given in PEP 667 passed.
The code only serves as a prototype, the proxy object is not even tracked by GC. However, the most essential features are implemented.
frame.f_locals
is now a real-time proxy for the actual local variables in the frame (there's no mapping yet but that's an implementation detail).locals()
behaves basically like before.All tests passed except for 3 - all expected
test_sys
for frame size - yes it changedtest_frame
- Thepop
method is not implemented yettest_listcomps
- the behavior offrame.f_locals
changed so the corresponding test is not valid anymore