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

ipython support: consider that ipython will create multiple frames (one for each line) for what should be logically a single frame #869

Closed
fabioz opened this issue Mar 11, 2022 · 10 comments
Labels
enhancement New feature or request

Comments

@fabioz
Copy link
Collaborator

fabioz commented Mar 11, 2022

In this case the request would be hiding/ignoring anything that's not called inside some function (namely async def run_code(self, code_obj, result=None, *, async_=False):: https://github.com/ipython/ipython/blob/de8729a984272b023768a8b85295ae06146da71d/IPython/core/interactiveshell.py#L3325).

We could rely on the __tracebackhide__ == "__ipython_bottom__" local variable in that function.

See: ipython/ipykernel#878 for details.

@subercui
Copy link

subercui commented Apr 30, 2022

Is there any following up for this? I think this feature is highly expected in vscode, microsoft/vscode-jupyter#8146.

@fabioz
Copy link
Collaborator Author

fabioz commented May 6, 2022

I've started to take a look at this.

@fabioz
Copy link
Collaborator Author

fabioz commented May 6, 2022

Some mental notes on the actual problem / approach I decided to take:

Problem:

  • The debugger needs to stop only when inside the ipython run_code, but only for the main ipython thread. For other threads, this shouldn't happen. This is needed so that when we're stepping we don't end up inside of ipython itself or somewhere in the standard library (especially with the async mode).

  • It's not possible to scope things simply entering/leaving that method because of coroutines (i.e.: the code may be paused in a method inside of run_code when a switch to a different coroutine is done and we don't want to pause anywhere in that coroutine while stepping -- note that we still want to pause in coroutines that are inside of run_code).

Given those constraints, I'm thinking about the following approach:

  1. Identify whether we're inside the run_code just when a thread is about to be resumed with a step operation (which means that we don't need to care for it on the regular case).
  2. If we identify we're inside the run_code, mark that info accordingly in PyDBAdditionalThreadInfo (which has debugger info for a given thread).
  3. When deciding whether to stop while stepping, verify if we've previously marked to stop inside the run_code. If that's the case verify if the current stack would need to be stopped or not (the actual check will still be a bit slower because it needs to go up in the stack to identify whether we're in the proper scope, but we only need to check when stepping and we can run the frame without any tracing afterwards if this is the case, so, it shouldn't have a big impact performance-wise).

Note: we don't really need to check for frame locals, which is much more expensive performance-wise (so, __tracebackhide__ == "__ipython_bottom__" won't be used), rather, the idea is that this is passed to the debugger in the attach/launch request arguments.

I'm thinking about something as:

scopedStepping: [
    {"<function name>": "<file basename>"}
]

In practice, for IPython this'd be something as:

scopedStepping: [
    {"run_code": "interactiveshell.py"}
]

@vidartf
Copy link

vidartf commented May 9, 2022

@fabioz Thanks for outlining the plan. It mostly seems good to me. Tagging @Carreau for any input he might have on this solution, as he was the one that added __tracebackhide__ originally to IPython (xref help). Some thoughts I had while reading:

  • Scoping on a hard-coded function in IPython might not be the most robust approach, as IPython currently has no guarantees that all kernel execution will pass through a function with this name, in a script with a given name. The frame local variable is the thing with the guarantee (which could survive a refactor with little issue). That said, the code base is pretty stable, and having it be a configurable pattern that is passed to the debugger on launch as you propose will make it more future proof 👍

  • I didn't realize that checking the frame locals was expensive. 👍

  • but we only need to check when stepping [...]

    I assume we would also need to filter on "stackframes" requests? I don't know how breakpoints work compared to stepping, but I assume the stepping logic also covers that?

@Carreau
Copy link

Carreau commented May 10, 2022

Yeah, it's a bit tricky, but I think a general "user"-definable set of predicate that basically respond to the question "should I really stop here" seem useful in general.
For performance reason you can't obviously run them on all frames, but when you know you are going to stop, I think it's reasonable to have a set of user provided rules that basically tell you that this frame is not interesting and just continue.

@fabioz
Copy link
Collaborator Author

fabioz commented May 13, 2022

  • but we only need to check when stepping [...]

    I assume we would also need to filter on "stackframes" requests? I don't know how breakpoints work compared to stepping, but I assume the stepping logic also covers that?
    

They're actually separate (the filtering logic can probably use the same info though... anyways, this is a 2nd step and much easier to do than controlling the stepping).

Yeah, it's a bit tricky, but I think a general "user"-definable set of predicate that basically respond to the question "should I really stop here" seem useful in general.
For performance reason you can't obviously run them on all frames, but when you know you are going to stop, I think it's reasonable to have a set of user provided rules that basically tell you that this frame is not interesting and just continue.

Agreed ;)

@fabioz
Copy link
Collaborator Author

fabioz commented May 13, 2022

Just to note, I'm currently still working on this. The basic idea is implemented locally, but I'm dealing with some corner cases dealing with coroutines (so, this will take a bit longer than I anticipated).

@fabioz
Copy link
Collaborator Author

fabioz commented May 19, 2022

As a note, the issue isn't just with coroutines, the issue is how IPython itself does the execution of the function (I got a bit sidetracked because I went first to tackle the coroutines case and became confused when each new line was showing a different frame under the debugger).

Let me exemplify. Given some code as:

a = 1
b = 2

IPython will actually make 2 calls to run_code, one with a = 1 and another with b = 2. This is problematic because for stepping the debugger relies on the frame and in this case the frame is no longer the same, so, for instance, if the debugger stopped at:

a = 1

and a step next is requested, the debugger will see that the frame with the code: a = 1 is about to leave and it'll revert it to a step into (which is the real reason why it gets into the internal IPython machinery) -- note that if the debugger didn't revert into a step into the step next would never stop because the frame that it references no longer exists, but reverting to step into will make it stop as soon as possible, which isn't ideal, but especially in the async case we can stop at odd places...

Later on the execution of b = 2 is on a completely new code/frame which has nothing to do with a = 1 aside from reusing the namespace, but for the user, we'd like to give him the illusion that when he does a step next, b = 2 is right after a = 1 (when in reality they're completely separate).

I'll have to thinker a bit more about the best approach here (this task already took me much more time than I anticipated, so, I'll probably have to reorganize my priorities to check if I can keep on it).

As a note, the original request (to debug only code called from some frame) may not really be needed to implement this feature (the actual request is debugging with IPython without the justMyCode flag without getting into the internal IPython machinery), what is needed is a way to communicate better that what's happening is that IPython is executing some code expression by expression but it should all be considered logically as a part of the original code (but I still need to think a bit more about what's a good approach here).

@fabioz fabioz changed the title ipython support: provide a way to debug code only called from some frame ipython support: consider that ipython will create multiple frames (one for each line) for what should be logically a single frame Jul 7, 2022
@fabioz
Copy link
Collaborator Author

fabioz commented Jul 8, 2022

I'm going back to tackle this issue. I'm still investigating which approach to take, so, I think it may take a while until I can find a good solution.

fabioz added a commit to fabioz/debugpy that referenced this issue Jul 30, 2022
fabioz added a commit to fabioz/debugpy that referenced this issue Jul 30, 2022
@fabioz
Copy link
Collaborator Author

fabioz commented Jul 30, 2022

@vidartf I've just provided a pull request which should fix it the issue (I still haven't tested properly with async code in IPython, so, it may still need work, but it should work for non-async code).

If you'd like, you can test it by getting the code in the PR: #998 (see comments in the PR for how to enable it).

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

No branches or pull requests

5 participants