Code Reloading

[karthiknadig]

Allow code to be modified while at break-point, then continue executing the modified code.

[fabioz]

As a note, this is supported in the backend (reloading is not perfect, but good enough to be useful on many cases).

-- API: PyDevdAPI.request_reload_code(py_db, seq, module_name)

the main issue is that it doesn't seem like this there's a message from the debug adapter protocol notifying that a file was changed and should be reloaded, so, the client side would need to be implemented first and a custom message would be needed (i.e.: microsoft/vscode#6930 (comment))

[DonJayamanne]

so, the client side would need to be implemented first and a custom message would be needed

The python extension can send these custom messages.

[int19h]

What are the limitations of the current implementation? I'm assuming that the code that is currently on the stack would not be affected, since there's no way to rewrite bytecode for a code object being executed, or adjust the state properly afterwards?

The key part here is establishing the expectations right. The most obvious way for a new user to try to do E&C is to hit a breakpoint in a function, and then try to edit the code around the breakpoint. But that would usually be in the same frame that's currently running.

Part of this is the feature name. The corresponding pre-existing feature for .NET languages in VS, while having many limitations, does allow this scenario, so long as you don't touch the currently executing statement (i.e. editing lines around it is fine). So if we call this E&C, we might be inadvertently setting up for expectations that are higher than what we can realistically deliver.

Perhaps "code reloading" would be a better name? This is more consistent with the terminology used in the Python ecosystem, and does not imply the ability to edit code that is currently actively executing.

[fabioz]

@int19h you're right, we don't rewrite the bytecode for the code object being executed (so, after doing the change the user needs to leave the function and enter it again -- usually that's good when you are in a webserver so you do a new request or have some UI application where you redo some action or if you step return and use a goto to go back to the previous line (maybe doing a goto to the end of the current function first) and enter the function again, so, these are the good use cases, but there are a many cases when that's not possible and I agree we should set the expectations properly here).

Also, the reload will patch function and classes in place, but there are limitations -- see: https://github.com/microsoft/ptvsd/blob/master/src/ptvsd/_vendored/pydevd/_pydevd_bundle/pydevd_reload.py#L70 for some limitations).

So, the good use case is when a function is changed and the user has a way to exit/enter it again, which I think is already pretty useful -- to be able to change what's being currently executed we'd need more support in CPython itself... maybe something to think about in the future ;)

[puremourning]

I would be keen on supporting this in vimspector, so if adapter vendors go down this route i'd strongly advocate for adding to the base DAP protocol rather than custom code. Vimspector has no way to run adapter-specific code (nor should it!).

[fabioz]

I created microsoft/debug-adapter-protocol#176 to add a new message in the specification (we'll probably go with a custom message until that makes it into the official specification).

[puremourning]

Would it be possible to base it on the existing jdt.ls/java debug server message? They use hotcodereplace

Refs

puremourning/vimspector#252

https://github.com/microsoft/java-debug/search?q=hotcodereplace&type=

hoping not to have to back invidual custom messages for each server type in vimspector if poss. Hopefully some discussion will come from the DAP proposal too.

[fabioz]

I'm not sure if that's feasible... we don't really support hot code replace here (which would change the current executing frame), as that's not supported by Python itself.

What we aim to support is code reload, which means that the debugger is able to change code that's NOT currently executing (so, we'd change the contents of a class or function in-place -- with a bunch of caveats -- one of those is that it'll only work if that code isn't currently executing).

[puremourning]

Oh and you need the client to do the file watching? that will be potentially tricky (eg to determine which files should be watched). Could debugpy watch the files with [watchdog] (https://pypi.org/project/watchdog/) and then use the java model (ask the client ‘should I reload this’)? That means debugpy gets to decide which reloads would work too. Just thinking out loud - I’m hoping that DAP can adopt something general, and I’m not wed to the java model - just don’t want lots of individual servers coming up with something different like in LSP

[fabioz]

My initial idea is that only the file changed on the editor will have code reload applied (so, when the contents of the current editor change and the user saves the file in the editor, then code reload will take place if the editor implementor is interested in doing that -- the initial implementation would be having vscode-python call the a custom message when that happens as I suspect it'll take more time to have that added to the official DAP spec).

So, no file watching should be required (but that's of course up to the editor implementor, debugpy will just provide the support for the reload and can't really dictate how it'll be used).

[int19h]

I think we'll have to do it on debugpy side if only because it won't work otherwise in remote attach scenarios. But the bonus would be that it'd also automatically work for all clients.

It does mean we'd have to vendor something like fs-watcher though, which has dependencies of its own. It might be possible to reduce the amount of vendored code if the feature targets Python 3 specifically.

[fabioz]

I think we'll have to do it on debugpy side if only because it won't work otherwise in remote attach scenarios. But the bonus would be that it'd also automatically work for all clients.

Why wouldn't it work with remote attach scenarios (given that the file path would be translated as usual)?

We can do a file watcher, but that comes with lots of caveats by itself, so, if we can avoid it, I think it'd be better...

[int19h]

In remote attach, the filesystem is remote relative to the IDE, so it wouldn't be able to watch it. And if it just reports files being changed in the editor, that wouldn't make much sense, since they'd only be changed on IDE side - the debuggee would still be dealing with old code, until the user synchronizes it manually somehow.

[fabioz]

Humm, but then editing those contents on the server side (instead of in the editor) would probably be kind of a pain too (and it'd become unsynchronized with the contents of the editor in the client side).

Maybe we could send the contents of the file in the code reload message and have the debug server update that file?

[int19h]

At that point, it's probably better to just use VSCode's own remoting support over SSH. In which case, from debugger's perspective, the debug session becomes local.

Where the debugging remote scenario can be interesting is when there's no path mapping set up (intentionally), and thus source code is fetched over the wire via Source display. If I remember correctly, such source is considered read-only from the IDE perspective - but DAP has messages for the debugger to report changes, implying that it can change on the other side during the session. This is kind of an edge case, I think, but since it's specifically accommodated in the protocol, it might not be as rare as I think.

The other thing we'll have to think about is how it'll work with PTVS, since it also uses debugpy. While it can also monitor files, of course, the implementation of any custom messages etc would have to be done from scratch there, since we can't reuse TS code from vscode-python.

[fabioz]

Ok, I'll take a look at supporting this with a file watcher.

[fabioz]

As a note, I'm working on this.

My idea is starting with a simple polling-based file watcher (there are many pitfalls with the native-based implementations such as watchdog, and polling must always be a fallback because on some scenarios the native-based approach doesn't work).

watchgod is the one that came closer to what I expected, but it has some issues -- i.e.: no support for python 2.7, and I think the scanning could try to cache based on the folder modified time instead of always scanning full trees to make things a bit more performant.

So, I'm currently taking a look at implementing the poll-based file watcher to use here.

[fabioz]

I've finished the poll-based file watcher and I'm thinking about how to do the tracking.

My initial idea is to support something as the following in the launch configuration arguments:

autoReload: {
    enable: true,
    watchDirs: ['c:/temp'],
    pollTargetTime: 3,
    ignoreDirectories: ['node_modules'],
    fileExtensions: ['.py', '.pyw']
}

Notes:
enable (default=true), used to enable/disable the code reload.
watchDirs (default: all the entries of the PYTHONPATH if justMyCode=false, otherwise all the non library entries of the PYTHONPATH), used to specify the directories to recursively watch (the extension could pass the ${workspace} here if it wanted when justMyCode=true, but that's optional).
pollTargetTime (default=3), this is the target time for a single poll in seconds (the polling will throttle to avoid consuming cpu while scanning for changes -- lower values would be able to find about the changes earlier, but would also consume more cpu).
ignoreDirectories (default: ['.git', '__pycache__', 'node_modules']), this can be used to skip directories for a faster directory scanning.
fileExtensions (default: ['.py']), this can be used in case the user deals with some custom file extension.

So, with that, by default any client would have auto-reload turned on.

When a file change is detected, I'm thinking about just sending an output event with a console category with the details on how the reload proceeded (i.e.: if it was possible to do the reload and what was updated), so, no further integration would be needed from the client side.

[int19h]

(@luabud, can you also take a look?)

I like the approach in general! Some bikeshedding on property names follows.

"watchDirs" should be "watchDirectories", for consistency with "ignoreDirectories".

I wonder if perhaps we could make "ignoreDirectories" and "fileExtensions" to use globs a la gitignore - does it complicate the implementation much? If it can be done, then we could turn them into two complementary properties like so:

"include": ["*.py", "*.pyw"],
"exclude": ["**/node_modules/**"]

And "pollTargetTime" could be something like "pollingInterval" for better clarity, and also for consistency with some other extensions (like WSL support) that use this terminology in similar circumstances.

[luabud]

This is super cool! +1 to Pavel's suggestions.

Quick basic question, since this would be enabled by default, we wouldn't have to add this argument to our current existing launch.json entries, right? So users would only declare it they want to change something, e.g. which directories to watch?

[fabioz]

Quick basic question, since this would be enabled by default, we wouldn't have to add this argument to our current existing launch.json entries, right? So users would only declare it they want to change something, e.g. which directories to watch?

Yes, it should work out of the box by default.

I'll take a look at supporting the glob format (which should be straightforward) and renaming those property names (if everything goes well I'll submit a pull request on Friday).

[karthiknadig]

We already have the "rules" which allow include and exclude patterns. I am wondering if this will add confusion, since this will be yet another set of rules to include or exclude files/dirs

[luabud]

@karthiknadig but rules isn't documented in the schema, is it? maybe we can add it too following the same convention?

[int19h]

I think it's fine, so long as we use top-level objects to provide clear scoping for "include" and "exclude", and we're consistent in how it works for both scopes.

[fabioz]

As a note, I got the code working but it still needs some cleanup (which should take one more day) before submitting a pull request (so, my plan is providing the pull request on Thursday next week).

[vviikk]

Is there any documentation regarding this?

[karthiknadig]

@vviikk If you want to enable this, you can do so from the debug configuration. Look for the following setting:

These are the available values. This feature is not enabled by default. So you have to set "enable": true

      "autoReload": {
          "enable": true,
          "exclude": [
              "**/.git/**",
              "**/__pycache__/**",
              "**/node_modules/**",
              "**/.metadata/**",
              "**/site-packages/**"
          ],
          "include": [
              "**/*.py",
              "**/*.pyw"
          ]
      }