microsoft / vscode-jupyter

VS Code Jupyter extension
https://marketplace.visualstudio.com/items?itemName=ms-toolsai.jupyter
MIT License
1.28k stars 286 forks source link

Run line by line in an untitled file prompts to save file #6898

Open miguelsolorio opened 3 years ago

miguelsolorio commented 3 years ago

Testing microsoft/vscode#129467

https://user-images.githubusercontent.com/35271042/127231501-0593e8d0-c401-4e40-8771-df7877e39333.mp4

I'm not sure what is expected here, but I got prompted to save an untitled notebook (with no workspace) but also clicked "cancel" and then it kept debugging? Either it should only work if you save it or allow to debug (I vote for the latter).

roblourens commented 3 years ago

@isidorn can we suppress the "save all" when a debug session starts? Maybe another startDebugging option?

isidorn commented 3 years ago

@roblourens you should do the same as powershell. More details and an example how to fix here https://github.com/PowerShell/vscode-powershell/issues/3338 So you should just do that for .ipynb files

roblourens commented 3 years ago

Hey @isidorn, that solution doesn't work because the language here is still Python, and we can't do it based on file extension. We don't want this for all Python debugging, just specifically in a notebook, so I think we need a flag on the launch config to set this dynamically. Does that make sense? It would be similar to the "simple UI" flag

roblourens commented 3 years ago

cc @weinand

isidorn commented 3 years ago

@roblourens a flag in the debugSessionOptions would work. However I am thinking if we can do this without introducing any new concepts. Or we could piggy back on the simpleUI flag and say if it is a simpleUI that means we are not saving untitled files before... Code pointer https://github.com/microsoft/vscode/blob/main/src/vs/workbench/contrib/debug/browser/debugService.ts#L315

weinand commented 3 years ago

I don't like piggybacking on the simpleUI flag because it waters down the semantics of the flag (not saving files is not really a feature of a "simple UI").

I would rather introduce a new mechanism for this.

A new flag in the debugSessionOptions is a pragmatic approach (which we should use here).

What I don't like about this approach is that the caller of the startDebugging API does not know, whether the underlying debugger/runtime supports debugging of unsaved files or not. So the caller is not really free to make an arbitrary decision. In the notebook case at hand this is not a problem because here the caller knows for sure that the save is not needed. But in general this is not the case...

It would be helpful if we could tie the "debug.saveBeforeStart" setting to "more than a language", basically a "when" context condition.

Similar for what is done for PowerShell:

"configurationDefaults": {
      "[powershell]": {
        "debug.saveBeforeStart": "nonUntitledEditorsInActiveGroup"
      }
},

we could do something like this for Python in notebooks:

"configurationDefaults": {
      "[alnguage == 'python' && editor == 'notebook']": {
        "debug.saveBeforeStart": "nonUntitledEditorsInActiveGroup"
      }
},

But this is just dreaming...

isidorn commented 3 years ago

fyi @sandy081 for a more powerful settings scoping mechanism. Also in case there might be some other mechanism to use here.

roblourens commented 3 years ago

Yeah, we previously had a conversation about making the setting scopes more powerful so we can set settings based on notebook kind.

I was thinking, is it ever useful to save an untitled file when I start debugging? Even if it's a .py file, it's not going to be imported by other files or in a launch.json if it is an untitled file that I just created. Can we save editors for files with unsaved changes but not save untitled files?

Alternatively, I'm not sure how to express what I want as a more complex option for saveBeforeStart. Maybe "activeEditorIsNotNotebook". Otherwise a debugSessionOptions would be best. And yeah the simpleUI flag also can't be reused because we want the same for full debugging.

isidorn commented 3 years ago

@roblourens yeah we explicitly added saving untitled files for beginners. Here's the scenario: a new user open a new untitled file, types some script and presses F5 to debug it The untitled file is not saved, and a lot of participants did not know that they have to save a file in order to debug it. So with the current approach we nicely prompt the user to save and then we start debugging. I would not change this flow.

Making saveBeforeStart more powerful with activeEditorIsNotNotebook sounds good to me. Though I would prefer not to have a notebook in the setting value. So something like activeEditorIsRegularEditor or whatever the name is for regular editors.

roblourens commented 3 years ago

I guess a problem is that in a language scope we would be setting this for all python files, and if the user configured this as something else that would override our setting. Given that, if we don't have a config scope for notebooks then it seems impossible to get it right with a setting.

isidorn commented 3 years ago

@roblourens you are correct, however that is a corner case. We can check numbers, but I think that a very low number of users actually touch this setting.

weinand commented 3 years ago

Yes, a setting tries to solve the problem on the wrong end: the user doesn't know the notebook cell debugger's requirements. Only the notebook cell debugger knows (this is already a problem for the PowerShell solution which ties the save requirement to the language instead of the debugger).

Since we don't have a contribution mechanism to communicate that information to the "save" code, using a debugSessionOptions seems to be the closest solution for now.

roblourens commented 3 years ago

the user doesn't know the notebook cell debugger's requirements. Only the notebook cell debugger knows

Well said, I agree that it should go in debugSessionOptions, I can implement that.

weinand commented 3 years ago

The only remaining question is how the debugSession option and the setting interact. IMO the setting takes precedence over the debugSession option. So a user could overrule the native behavior. But I'm not so sure about this...

isidorn commented 3 years ago

I think the configurationService does not let us know if the setting has been set by the user or it is just providing the default. So for pragmatic reasons I suggest that the debugSessionOption takes priority.

weinand commented 3 years ago

@isidorn yeah, that makes sense

sandy081 commented 3 years ago

Jumping in here (not having previous context)

I think the configurationService does not let us know if the setting has been set by the user or it is just providing the default.

You can use inspect api of configuration service to know if a setting is defined by user or not.

roblourens commented 3 years ago

Added a flag called suppressSaveBeforeStart?: boolean; to DebugSessionOptions, let me know if that sounds ok

isidorn commented 3 years ago

Sounds good to me.

DonJayamanne commented 2 years ago

The run by line icons is missing in untitled files.

Note: Opening an existing file will fix this (however if i wanted to try just RBL with a blank notebook, then its broken).

See below (kernel picker has the kernel & its a python cell) Screen Shot 2021-09-28 at 13 19 01

roblourens commented 2 years ago

I see this context key: jupyter.ispythonnotebook: false