justMyCode warning message is at the wrong level, not always accurate

[roblourens]

I noticed that when using "run by line" in a jupyter notebook in vscode, I sometimes get a notification with this message

debugpy/src/debugpy/_vendored/pydevd/_pydevd_bundle/pydevd_net_command_factory_json.py

Lines 435 to 438 in 6e247fb

	msg = 'Frame skipped from debugging during step-in.'
	if py_db.get_use_libraries_filter():
	msg += ('\nNote: may have been skipped because of "justMyCode" option (default == true). '
	'Try setting \"justMyCode\": false in the debug configuration (e.g., launch.json).\n')

There's a few things that seem off. First, there is no launch.json in this scenario, and the user can't disable justMyCode, because that's just how this feature works. I assume there isn't a real bug present when this message shows up but am still looking at it.

It feels wrong to refer to a launch.json in a message from a debug adapter. launch.json is not a DAP concept, it's a vscode concept, so to me this message exists at the wrong level, and would be better for it to originate from somewhere like vscode-python. Can't debugpy be used with other DAP clients besides vscode? Even in vscode, a launch.json may not always exist, like for dynamic launch configs from an extension.

Plus, if you show this message using vscode extension APIs, you can implement a "Don't show again" button - that seems important for users who know what they're doing.

[fabioz]

@roblourens the message still needs to originate from the debugger (which is doing the stepping) and the actual message shown (along with a Don't show again) can be already implemented by a client by capturing the output message (which is marked with the `category="important") and doing any processing needed.

I'm ok with changing that message to make it less context-dependent on launch.json and leaving the client with the work of adding more info depending on how the launch was done though... Do you have any suggestion on what'd be a better message?

@int19h @karthiknadig any thoughts on this?

p.s.: @roblourens with debugpy 1.6.3 and the fixes with the stepping in ipython it probably makes sense to support the justMyCode setting now (the related issue was just closed, but I think it was by mistake and should probably be reopened: microsoft/vscode-jupyter#8146 -- the issue which prevented justMyCode to work with ipython is fixed in debugpy, but it still needs to be implemented in the vscode-jupyter side).

[roblourens]

I would suggest sending a custom DAP message that the extension can intercept and translate into a showInformationMessage call. I'd rather not have vscode-jupyter intercepting certain output events with a certain message and filtering them out. Reopened that issue, thanks

[fabioz]

I'm not sure... I think I'd rather have other clients showing the message that step in didn't go into some function because the filters are enabled and having the clients that don't want act on it it rather than posting a custom message which would be usually silent for everyone that's not paying attention (in general, before we added this message this was confusing for some users, so, it's indeed something important to show in general and probably something interesting for vscode-jupyter too once justMyCode support is added).

Unfortunately, from the messages available by default in the DAP, the only way I can see which makes sense is showing an output message marked as critical, which clients can detect and act accordingly (I think clients should implement showing that along with a Don't show again, but this is up to clients and not something we can control at the debugger).

So, the way I see it, we can make the message more generic hoping that clients interested translate it better for their scenario, but not removing it altogether nor changing to a custom message which would be usually not shown to users (or we can just leave as is and expect clients that think the message should be different intercept it and present a different message regardless).

Maybe we could set something as data: "STEP_IN_SKIPPED_DUE_TO_JUST_MY_CODE" to the current output event so that clients can recognize better that this is the case (so that you wouldn't need to match through the output attribute).

[int19h]

We do support third-party clients. @puremourning, is generic error codes something that Vimspector might be interested in?

It should be noted that this is something that applies even to our own clients, since VS uses different UX to enable/disable Just-My-Code.

So far as I can tell, a brand new custom message would be worse for any client that doesn't specifically support debugpy. OTOH a custom property on our "output" events would allow clients to filter and/or translate them consistently if they want. And then debugpy and pydevd can switch to generic, non-client-specific messages as a fallback.

Another advantage is that the clients could then localize those messages.

[puremourning]

Maybe we could set something as data: "STEP_IN_SKIPPED_DUE_TO_JUST_MY_CODE" to the current output event so that clients can recognize better that this is the case (so that you wouldn't need to match through the output attribute).

Well, not according to the spec...

 * Additional data to report. For the `telemetry` category the data is sent
 * to telemetry, for the other categories the data is shown in JSON format.

is generic error codes something that Vimspector might be interested in?

vimspector, no probably not. I'm kind of lazy, so I actually just stuff all output events in an output window and never show any kind of popup or popover. I've strongly argued against introducing a 'showMesssage' API in DAP (like the one in LSP) because it feels like micromanaging the UI.

That said, something on the Output event that says "the user might be interested in seeing this" seems a reasonable thing to put in DAP, and checking quickly, it looks like someone did - there's this important category hint:

* 'important': A hint for the client to show the output in the client's UI
* for important and highly visible information, e.g. as a popup
* notification. This category should only be used for important messages
* from the debugger (as opposed to the debuggee). Since this category value
* is a hint, clients might ignore the hint and assume the `console`
* category.

I'd suggest however that a message which happens frequently, such as on stepping probably isn't that important....

That said, DAP has this weird Message thing that's quite buff, but currently only used for reporting errors. Maybe an Output event could include an embedded Message (which includes things like showUser and formatting parameters etc.). The reason I mention it is that this Message thing contains an id which you could use in clients-that-know-things-about-servers to do something custom (including localisation).

Just a thought on that.

So far as I can tell, a brand new custom message would be worse

yep, in one sense; there are few things worse than custom messages to generic clients for important features. custom properties on existing requests aren't likely a problem for me, but you gamble that your property doesn't get used for something else later of course.

[puremourning]

FWIW I actually don't think the current message is all that problematic in practice. It says "e.g. launch.json" and most users of clients like vimspector, nvim-dap, etc. will work out that means the client's equivalent way of specifying launch config.

[int19h]

Using "data" for the error code might not be a good idea, but we can always add (namespaced) custom properties, so long as it's not mandatory for the clients to be aware of them.

[roblourens]

For the "data field",

for the other categories the data is shown in JSON format.

I don't really understand this- vscode doesn't do this. Trying to get some clarity on the intent of this field because using the data field (setting an object with some errorCode property) seems like a fine way to attach some metadata to an OutputEvent.

If using data isn't ok then I think I'd be fine with a custom property on OutputEvent too.

there's this important category hint:

Yeah, this message is using the important category

[puremourning]

FWIW I agree with you that the data item seems a good place, but if the spec says it's displayed as json any client might do that. That said I'm not sure the spec comments are normative so changing it probably wouldn't break anything important (one might hope)

[fabioz]

I think a custom field seems safer given the description of data.

How about something as: pydevd_output_message_type: "STEP_IN_METHOD_SKIPPED_DUE_TO_FILTERING" in the output event?

I'd suggest however that a message which happens frequently, such as on stepping probably isn't that important....

@puremourning note that this shouldn't happen frequently... it should only appear at most once per debug session and only if the step in is done and a method isn't traced during the step in due to some filtering (which the user may not be expecting -- the justMyCode is the active filter by default, so, that's in the message, but it could be due to other filters too, but those are opt-in and the user is usually expecting them).

[puremourning]

note that this shouldn't happen frequently... it should only appear at most once per debug session

Ah ok, cool.

[roblourens]

at most once per debug session

So... dozens of times per day? 😁

But anyway, this works fine for the Jupyter scenario that I am mainly concerned about. I will filter out the message since it's not relevant for notebook debugging/run by line (maybe it will become relevant when we properly support justMyCode).

I guess I should loop in @karthiknadig too because I'm not sure whether vscode-python is positioned to filter or modify the message before it's received by the client. When debugging a .py file, is vscode talking directly to debugpy or is vscode-python in the middle?

[karthiknadig]

@roblourens There is no middleware between DAP client and server (like there is in LSP). So, currently VS Code directly communicates with DAP server. There is a DebugAdapterTracker class that can be used to observe messages going from and to the DAP client, but I don't know if it allows modification.

[int19h]

The vscode-nodebook debugger extension sample uses DebugAdapterTracker specifically to rewrite debugger messages (file paths in them), so it seems that this is in fact the intended behavior:

https://github.com/microsoft/vscode-nodebook/blob/main/README.md

[roblourens]

Yeah, so you can use a DebugAdapterTracker to modify messages, but not to filter them out entirely, so some OutputEvent is going to go to vscode. I think this is ok for our purposes here. The message could be changed, the category could be changed to prevent it from becoming a notification if needed... and I don't think that work even necessarily has to happen for vscode-python, that's up to you. Just thinking through the scenarios.

[roblourens]

Revisiting this issue, and dumping some of my thoughts.

Could we think about the user experience and the problem being solved here, separately from talking about the shape of the message over DAP? If people are confused about this and filing issues, could you share an example? I wonder what people are confused about - since justMyCode appears to be enabled by default, do people not even know the feature exists, and want to step into library code? Or do they enable it and forget, misunderstand it, or not know which code is "my" code?

Even if I am in the targeted scenario for this message - debugging a python script, with justMyCode enabled, and I step in somewhere where it won't have an effect, I don't think that I want to see a notification. I think that having a "don't show again" button or finding another UI solution is critical.

I was wondering whether there could be some other UI to tell users that they are in this mode, besides a notification. For example, we recently added this icon to give context about unbound breakpoints: https://code.visualstudio.com/updates/v1_69#:~:text=UNBOUND%20BREAKPOINT%20WARNING%20ICON%20IN%20JAVASCRIPT%20DEBUGGING What if there was an icon that appeared in the callstack view header, advising users that they are in this special stepping mode?

If not that, do you think that it has to be a notification- is a normal console message sufficient?

If the solution is to have a notification, would it be enough to only show this message to a user once?

[roblourens]

Wondering whether anybody has thoughts on the above - otherwise I may try to turn this github issue into a meeting, and I know nobody wants that 😁

[fabioz]

If not that, do you think that it has to be a notification- is a normal console message sufficient?

I think that having an output event marked with important is the correct approach... I'm ok in tweaking the message per se to fit more scenarios or adding more info to it if the message itself is the issue.

Now, how clients interpret an output event marked as important is up to the client, so, if some particular clients wants to add a don't show again, that seems fair to me and it should still fit the DAP spec.

As for justMyCode not being available in Jupyter itself, that is an issue that should definitely be fixed but is independent on the message (i.e.: microsoft/vscode-jupyter#8146) -- i.e.: it should be actionable in this use case and not really not applicable just because the user can't change that setting right now.

Also, we did have reports from users prior to this because users do get confused, which is why I think it's important that it's shown to users.

The fact that it's enabled by default is one issue, but sometimes users would put their own code inside a standard library directory and would be confused because stepping wouldn't work (so, I remember we had multiple issues due to those prior to adding that notification and no issue afterwards).

I agree that clients should strive to implement a don't show again so that users can disable it in some particular scenario (the DAP could probably be extended so that it's possible to add an some kind of reason to the output event so that clients could ignore based on that reason and not the actual message string being shown).

[roblourens]

Also, we did have reports from users prior to this because users do get confused, which is why I think it's important that it's shown to users.

Could you share some examples? I'd just like to better understand what they were saying.

I still don't think that a notification is the best way to help the user in this sort of situation. Yes, you can show a notification to 100,000 users, and then 5 of them will not files issues, but 999,995 users got annoyed by having to close yet another notification. I think that we are sometimes quick to add notifications in vscode to put some info in front of the user's face, but the end result is feeling that you constantly have to dismiss notifications. A "Don't show again" button helps but makes the user do the work to figure out whether the notification is useful, and next month what if they just forget about the notification in the first place?

I think there are some ways to give users a hint that "step in" in operating in a different mode, without getting in the way of users who already know this. We could put an icon with a tooltip message in the debug viewlet, like our hint about unbound breakpoints. Or a non-notification message in the debug console seems like an appropriate place for hints to the user. If that isn't obvious enough, would changing the styling in vscode help? I wonder about changing "important" output events to not be notifications, but be flagged as special in a different way.

This one is more out there, but maybe the python extension could even register a custom "step-in" icon that is different in some way to show that justMyCode is enabled.

js-debug allows dynamically disabling justMyCode through a context menu option on the callstack, if that is possible for debugpy I'd like to see vscode-python add the same thing.

[fabioz]

Could you share some examples? I'd just like to better understand what they were saying.

The first 3 that I could find:

microsoft/ptvsd#2106
microsoft/ptvsd#1563
microsoft/ptvsd#1467

I agree that if the UI can be improved so that we don't need the notification, that may be even better ;)
-- or if the UI wants to intercept that message and act differently on it, that'd be Ok too...

Note: this message was added in: microsoft/ptvsd#1298

[roblourens]

Sorry for the late response. A while back, had a discussion with @karthiknadig about this. Something we agree on is that the current experience has one setting that changes multiple things, and the overall experience could be more dynamic and granular.

Some thoughts:

• Is it possible to dynamically enable/disable justMyCode in a debug session? Even just toggling the one global switch would be an improvement. js-debug can do this, and you can right click a file in the callstack to toggle skipping that one individual file.
• Beyond a global toggle, would it be feasible to implement this as separate "step in" action? The user could click the normal "step in" button to perform the action with justMyCode enabled, or they could invoke a separate "step in to library code" command which would step in with JMC disabled. We could possibly reuse StepInTargetsRequest for this, or a separate custom action.
• If I set a breakpoint in library code, I should be able to hit that breakpoint, whether JMC is on or not
• Currently when JMC is enabled, frames from library code are completely omitted from the callstack. They could be included but styled differently, which js-debug does, using presentationHint on the source.

[fabioz]

• Is it possible to dynamically enable/disable justMyCode in a debug session? Even just toggling the one global switch would be an improvement. js-debug can do this, and you can right click a file in the callstack to toggle skipping that one individual file.

The debugger does support dynamically enabling/disabling justMyCode internally already (but it can't do that for individual files right now).

• Beyond a global toggle, would it be feasible to implement this as separate "step in" action? The user could click the normal "step in" button to perform the action with justMyCode enabled, or they could invoke a separate "step in to library code" command which would step in with JMC disabled. We could possibly reuse StepInTargetsRequest for this, or a separate custom action.

Internally this is actually already implemented as a different step in action (there's a step in and there a step into my code -- when the global toggle is used one or the other is selected), but the DAP doesn't really support it. If we go this way I think we should do a separate action instead of subverting existing ones...

• If I set a breakpoint in library code, I should be able to hit that breakpoint, whether JMC is on or not

Agreed... we should have a way to whitelist such paths (this is probably the biggest cause of frustration for users which don't know about it, so, big +1 from me to this).

• Currently when JMC is enabled, frames from library code are completely omitted from the callstack. They could be included but styled differently, which js-debug does, using presentationHint on the source.

I'm +0 here, I think that sometimes that's better and sometimes it's not. Maybe this should be a separate option.

[roblourens]

The debugger does support dynamically enabling/disabling justMyCode internally already (but it can't do that for individual files right now).

Oh, can I trigger that, or is it only internal?

Internally this is actually already implemented as a different step in action (there's a step in and there a step into my code -- when the global toggle is used one or the other is selected), but the DAP doesn't really support it. If we go this way I think we should do a separate action instead of subverting existing ones...

That's also cool. I think it could be a fair usage of StepInTargets, although I'm guessing you can't know whether there is actually library code behind a particular step in location before actually stepping in, and so debugpy would always return two targets that might step in to the same place. That doesn't bother me that much, but I'm also open to making it a custom request.

I'm +0 here, I think that sometimes that's better and sometimes it's not. Maybe this should be a separate option.

Probably some users never want to see that stuff, a setting is reasonable.

[fabioz]

The debugger does support dynamically enabling/disabling justMyCode internally already (but it can't do that for individual files right now).

Oh, can I trigger that, or is it only internal?

It's internal... that API isn't very straightforward to consume from the outside.

Internally this is actually already implemented as a different step in action (there's a step in and there a step into my code -- when the global toggle is used one or the other is selected), but the DAP doesn't really support it. If we go this way I think we should do a separate action instead of subverting existing ones...

That's also cool. I think it could be a fair usage of StepInTargets, although I'm guessing you can't know whether there is actually library code behind a particular step in location before actually stepping in, and so debugpy would always return two targets that might step in to the same place. That doesn't bother me that much, but I'm also open to making it a custom request.

The debugger actually uses StepInTargets already where the user can select to go into a given call (but it still respects the justMyCode flag and won't go into library code even if the user selected to do so -- which isn't ideal).

[fabioz]

p.s.: In case you check it, StepInTargets still isn't working for Python 3.11 (as it needs bytecode manipulation, so, it's waiting for MatthieuDartiailh/bytecode#103 to be done first).

[judej]

Thank you for the report. Given our current resources are working on the Python 3.12 debugger, we will not be fixing this issues in the pre Python 3.12 debugger.

[roblourens]

I think this would still be relevant in Python 3.12. I personally am not focused on python/jupyter debugging at the moment though.

[int19h]

The 3.12 rewrite basically replaces pydevd with our own in-house debug server, so this entire codepath is getting reworked.