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

Debug Adapter in Python #6017

Closed
3 tasks
DonJayamanne opened this issue Jun 13, 2019 · 8 comments
Closed
3 tasks

Debug Adapter in Python #6017

DonJayamanne opened this issue Jun 13, 2019 · 8 comments
Assignees
Labels
area-debugging feature-request Request for new features or functionality meta Issue that is tracking an overall project

Comments

@DonJayamanne
Copy link

DonJayamanne commented Jun 13, 2019

This issue is used to track discussions related to using PTVSD as the debug adapter:

  • File issue on VSC for reverse request (custom request from DA to VSC). One example of existing item is runInTerminalReqeust
  • File issue on VSC for logging of DA messages (every adapter needs to write custom code to enable this)
  • When debugging multiple procs (parent & multiple children), then VSC will display a single debug session.
    • Will clicking Stop terminate all debug sessions?
    • Will the parent session be the only one that gets the terminate session
@DonJayamanne DonJayamanne added feature-request Request for new features or functionality triage-needed Needs assignment to the proper sub-team labels Jun 13, 2019
@DonJayamanne DonJayamanne self-assigned this Jun 13, 2019
@DonJayamanne DonJayamanne added area-debugging needs PR debt Covers everything internal: CI, testing, refactoring of the codebase, etc. triage-needed Needs assignment to the proper sub-team and removed triage-needed Needs assignment to the proper sub-team needs PR labels Jun 13, 2019
@DonJayamanne
Copy link
Author

DonJayamanne commented Jun 13, 2019

Legend

  • DA = Debug Adapter
  • PVSC = Python VS Code (extension)
  • Jupyter/Jupyter Debugging = Debugging code in Interactive Window (using Jupyter)
  • psutils = Python Package used for terminating process tree.

Discussions:

  • Environment variables will be built by Extension and sent in launch just as part of the env variables. I.e extension will be responsible for reading & parsing env variables defined in a .env file or similar.
    • The envFile will not be sent to PTVSD DA (to ensure PTVSD doesn't attempt to load that .env file)
  • PTVSD DA can send custom requests to Python Extension as follows:
    • Raise a custom event
    • PVSC handles this custom event
    • PVSC sends a custom request to PTVSD DA
    • PTVSD DA sees this custom request and treats it as a response to the original event.
  • Extension will use new VSC API to provide the path to python executable use to start the DA (see registerDebugAdapterDescriptorFactory
  • PTVSD DA will handle killing sub processes (via psutils)
  • PVSC to add a custom launch.json setting for interpreter arguments.

Needs discussion/review later:

  • Shipping of psutils with PTVSD needs to be reviewed as this will impact the version of PTVSD shipped in extension.
  • PTVSD DA will need to send a custom request back to Extension asking whether to launch user code or not. Possible extension needs to launch the code.
    • This is for Jupyter debugging (where extension is responsible for launching the user code).
    • @int19h @karthiknadig

PVSC changes:

  • Registering the DA Adapter executable (see registerDebugAdapterDescriptorFactory Launch new Python Debug Adapter in Extension #6817
    • Enable new debug adatper via an experiment (not to be treated as an experiment, but a controlled roll out).
  • Env variables will be parsed and provided in debug config resolve method. Adapter does not handle "envFile" #6770
  • No need to kill sub processes using psutil
  • Delete old code Remove obsolete debug adapter code (when ready) #7136
  • Extension to dynamically define environment variable for PTVSD_LOG_DIR pointing to a location in extension (this is required for backwards compatibility & based on the existing logToFile setting).
    • Users won't need to provide path to the file, just enable/disable.
    • Extension shouldn't overwrite existing PTVSD_LOG_DIR variable if already defined.
  • program in launch.json can be both string or array. Example passing script args. ["myscript.py", "--my-args"]
  • python in launch.json can be both string or array. Example passing interpreter args.["/usr/bin/python3", "-O"]
  • Review impact on other extensions (Azure Functions launches debugger by manually invoking PTVSD then starting the debugger).
  • Deprecate logToFile and use logDir/debuggerLog /<other> or similar (pass this as a CLI arg to PTVSD).
    This will be the directory where PTVSD will log stuff (extension will specify directory as root directory of extension)
  • Install new version (beta/release) of PTVSD that has the debug adapter.
    • Ship wheels for this new version of PTVSD (not older version)
    • Install older version of PTVSD (without wheels) for typescript debug adapter (stable code, but no wheels)
  • Deprecate customDebugger and use something like ptvsd/ptvsdPath that'll point to the path of PTVSD that is to be used.
    • Having customDebugger and expecting users to pip install a version of PTVSD into their environments is dangerous (they might forget about it).

@DonJayamanne DonJayamanne added needs PR and removed triage-needed Needs assignment to the proper sub-team needs PR labels Jun 17, 2019
@luabud luabud added the meta Issue that is tracking an overall project label Jul 30, 2019
@karrtikr
Copy link

karrtikr commented Jul 31, 2019

Prescribed solution

  • Registering the DA Adapter executable (see registerDebugAdapterDescriptorFactory - https://code.visualstudio.com/api/references/vscode-api#debug)
    • Executable will be the python interpreter
    • Program to be launched is path to ptvsd
  • Enable the new debug adapter in insiders (check applicationEnvironment.ts - extensionChannel) by default

@DonJayamanne DonJayamanne added this to the 2019 - July Sprint 15 milestone Jul 31, 2019
@DonJayamanne DonJayamanne removed the debt Covers everything internal: CI, testing, refactoring of the codebase, etc. label Jul 31, 2019
@karthiknadig
Copy link
Member

karthiknadig commented Aug 1, 2019

Another thing to do:

  • if logToFile is set in the debug config then extension should pass --log-dir <path-to-extension-dir> when launching the debug adapter. createDebugAdapterDescriptor receives both the debugSession and the executable.

@int19h
Copy link

int19h commented Aug 2, 2019

There's no backwards compatibility issue with using the command line switch for it rather than the environment variable, since VSCode will always use the adapter that is bundled with it. For as long as we have both adapters in place, so long as the old one still handles it, the new one can always get the switch - thus, no need for the env var for either one.

@int19h
Copy link

int19h commented Aug 2, 2019

My suggestion is to allow "logToFile" to be either a boolean (with the same semantics as before) or a string, so that users can place the logs wherever they want, rather than the default location.

Also, perhaps it could use a better name? As it is, it's a bit confusing, because it's unclear what is getting logged to the file, especially since some other VSCode debuggers have facilities to redirect the standard logging facilities in the language itself (i.e. for user app logs, not debugger logs), and we might want to do something like that in the future as well for the Python logging standard module. So maybe something like "debuggerLog"?

OTOH if we want to match what the other debuggers do, the Node.js and Go ones call this setting "trace" - although that is even more vague. C++ debugger seems to not have it in launch.json at all, and instead uses an extension setting - I suspect it's less discoverable, but on the other hand, it will come up when searching global settings, so it's hard to say.

(Renaming or moving it shouldn't be a backwards compatibility issue, since it's strictly a diagnostic tool - nobody should have that floating around in their debug configs, it's only meant to be enabled to report a bug. )

@luabud
Copy link
Member

luabud commented Aug 13, 2019

A/B test starting with 30% in each experiment group

@luabud luabud added the experimenting Feature is part of an experiment label Aug 13, 2019
@DonJayamanne
Copy link
Author

DonJayamanne commented Aug 14, 2019

@karthiknadig
Copy link
Member

karthiknadig commented Oct 17, 2019

  • Create a single Package containing binaries for following: This can be done by downloading wheels for each of the platforms and merging the ptvsd directory across all the wheels. This prevents us for shipping multiple copies of the debugger to support wheels. See karthiknadig@9d8f463 for reference

    • Manylinux1 32/64
    • Darwin 10_13 64
    • Windows 32/64
  • DebugAdapterDescriptor creates these two when in wheels experiment

    • For Attach: DebugAdapterServer(config.port, config.host). Note python interpreter is not needed for attach.
    • For Launch: DebugAdapterExecutable(extensionRoot\PythonFiles\lib\python\ptvsd\adapter). Remember if logToFile is set then append --log-dir <dir>. Note python interpreter is needed for launch
  • Logging should also be done via DebugAdapterTracker

    • If logToFile then enable logging using debugAdapterTracker. Note that we need logs in both attach and launch case. In attach case with the new adapter, there is nothing running on the local machine. To get local logs we need to use tracker. I had suggested this a long time ago as a preparatory. But now we will need it. The tracker exists for this purpose so we should make use of it.
  • Telemetry for wheels.

    • Wheels telemetry should be generated in-proc in the debugger. Currently the telemetry assumes that wheels are being used just because we point python to the directory that has wheels. Since python decides when to load the binary version, and it can also decide to not load it. So the only way to get correct telemetry is inproc. There are also some cases where we disable binary loading even if ti is available. Telemetry will be accurate inproc.

@kimadeline kimadeline removed their assignment Nov 12, 2019
@luabud luabud closed this as completed May 7, 2020
@ghost ghost removed the experimenting Feature is part of an experiment label May 7, 2020
@lock lock bot locked as resolved and limited conversation to collaborators May 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-debugging feature-request Request for new features or functionality meta Issue that is tracking an overall project
Projects
None yet
Development

No branches or pull requests

7 participants