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

Add dynamicscope items, keys, values, update, and iter #566

Closed
wants to merge 1 commit into from

Conversation

frmdstryr
Copy link
Contributor

This at least fixes #565 and makes ipython embed work in an expression/event handler.

I don't think doing the full scope (walking parents and getting members) is necessary but I can try that if needed.

Copy link

codecov bot commented Jan 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.92%. Comparing base (3bcd2a4) to head (960a739).
Report is 21 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #566   +/-   ##
=======================================
  Coverage   70.92%   70.92%           
=======================================
  Files         287      287           
  Lines       25710    25710           
  Branches     3643     3643           
=======================================
  Hits        18234    18234           
  Misses       6385     6385           
  Partials     1091     1091           

@sccolbert
Copy link
Member

Before we go this route, I'd like it if someone investigated the change between Py2 and Py3 to see if that really is the problem. It may be (but probably unlikely) that everyone assumes f_locals is a dict when that's really not the case. We really need to understand what protocol is expected of f_locals by the interpreter. Because if the interpreter is now expecting a proper dict actual, then we are in trouble, and will need to re-think all this magic we are doing.

@frmdstryr
Copy link
Contributor Author

frmdstryr commented Jan 11, 2025

Both frame.f_locals and PyFrame_GetLocals say mapping.

The traceback module uses list(frame.f_locals) and locals.items() which should be part of mapping at least according to collections.abc.

For reasons I don't understand the Dynamicscope has Py_TPFLAGS_DICT_SUBCLASS set but not Py_TPFLAGS_MAPPING so it is a dict according to PyDict_Check. Idk how it passes the PyMapping_Check yet. Edit: Looks like just having an mp_subscript makes it pass that.

Either way it at least needs to implement the tp_iter?

@sccolbert
Copy link
Member

sccolbert commented Jan 11, 2025 via email

@sccolbert
Copy link
Member

It looks like PyMapping_Check is deficient in actually fully checking for the mapping protocol. You can see what it checks here:
https://github.com/python/cpython/blob/5e65a1acc0b630397f1d190aed279114e6e99612/Include/cpython/object.h#L125
https://github.com/python/cpython/blob/5e65a1acc0b630397f1d190aed279114e6e99612/Objects/abstract.c#L2257

But then the mapping functions go on to call other methods that should exist on the object (just read thru the rest of the functions below PyMapping_Check.

@frmdstryr
Copy link
Contributor Author

Agree on PyMapping_Check. I looked through bytecodes.c for uses of locals and it seems to only use PyObject_SetItem, PyObject_GetItemOptional, and PyObject_DelItem.

So what do you think should be done?

@MatthieuDartiailh
Copy link
Member

For each of the added methods, I would like a clear view of when they are expected to be used and why we are fine paying the price of creating a dict.
Regarding the traversal of parents, I believe it could be valuable as otherwise figuring out the names accessible from parents can be quite tough.

@sccolbert
Copy link
Member

To do this properly, and without over-allocating upfront, we'll need to produce a proper iterator over the dynamic scope.

Likely the easiest way to do this is to keep a _DynamicScope class in c++ which implements the high-perf __getitem__ methods and friends. Then subclass that class in Python to implement the rest of the mapping protocol. You can make the f_locals f_globals f_builtins etc... as readonly attributes on the base class, then access those attributes from the subclass to implement key items iter etc. and leverage itertools.chain to cover everything.

Those python methods don't need to be fast, they just need to be present.

@sccolbert
Copy link
Member

But one other thing to keep in mind is that DynamicScope has behavior that supports the tracing machinery, so if you expose DynamicScope to python code, it could be a foot-gun where something isn't traced properly.

I would also be fine with DynamicScope just returning an empty iterator for all of the iterator/dict protocol stuff.

@MatthieuDartiailh
Copy link
Member

MatthieuDartiailh commented Jan 18, 2025

@sccolbert do you have any specific concern regarding tracing ? I do not really see what could happen from exposing interfaces that do not allow to mutate the scope.
Personally, I have two concerns:

  • not providing anything makes the debugging experience worse and since we already suffer from zero IDE integration, I find it sad.
  • providing these methods and matching the semantic expectations people may have (items reflect the actual dict content even is the dict is mutated) is a huge work.

A frozen items like in Python 2 would probably be sufficient for debugging purposes and hopefully the limitations won't be noticed.

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.

Errors cannot be printed/formatted in 3.12+ because DynamicScope object is not iterable
3 participants