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

issue #3182: replace deprecated ptvsd debugger by debugpy #3183

Merged
merged 6 commits into from
Aug 22, 2024

Conversation

rngadam
Copy link
Contributor

@rngadam rngadam commented Aug 19, 2024

issue #3182

keeping ptvsd environment variables for backward compatibility

@rngadam rngadam force-pushed the rngadam/issue3182 branch 4 times, most recently from 6a1bbe7 to b9bb410 Compare August 19, 2024 21:26
@swcurran
Copy link
Contributor

Thanks for the update. Can you please search the documentation (*.md files) for references to ptvsd, and update changes in those as needed. Also, are there any places in the dockerfiles that need to be updated? Might not, but I have a faint memory there were some things there.

@rngadam
Copy link
Contributor Author

rngadam commented Aug 20, 2024

Thanks for the update. Can you please search the documentation (*.md files) for references to ptvsd, and update changes in those as needed. Also, are there any places in the dockerfiles that need to be updated? Might not, but I have a faint memory there were some things there.

ENABLE_PTVSD, PTVSD_HOST, PTVSD_PORT should keep working as is, but with debugpy as the underlying implementation. Is that ok? If that's acceptable, then we don't need to change wherever those environment variables are used. Otherwise yes, there are a couple places where we'd need to rename PTVSD for DEBUGPY.

There are two places I ought to update in any cases:

  • docs/features/DevReadMe.md (link to debugpy instead of ptvsd)
  • conftest.py (the test won't work here)

@swcurran
Copy link
Contributor

I think it is OK to keep the existing config parameters, although in retrospect it is unfortunate that they weren’t just “DEBUG” in the first place. Who knew the implementation would change :-). Suggest at least updating the config parameters information to note that debugpy is being used instead.

OK — one more question. Is it easy in argparse to have two names for the same config? If that were possible we could have both the PTVSD* and DEBUG* for the config parameters. Not a big deal...

Thanks!

@rngadam
Copy link
Contributor Author

rngadam commented Aug 21, 2024

I think it is OK to keep the existing config parameters, although in retrospect it is unfortunate that they weren’t just “DEBUG” in the first place. Who knew the implementation would change :-). Suggest at least updating the config parameters information to note that debugpy is being used instead.

both PTVSD and debugpy are implementation of DAP. The reason it is not debug I think is because there is an alternative to debug with Pycharm too.

OK — one more question. Is it easy in argparse to have two names for the same config? If that were possible we could have both the PTVSD* and DEBUG* for the config parameters. Not a big deal...

I added DAP_HOST and DAP_PORT as alternates environment variables. Note that this is done in main, not argparse before anything else starts.

Updated doc and conftest.py (although I'm not sure how to test the latter?).

@rngadam
Copy link
Contributor Author

rngadam commented Aug 21, 2024

Also, seems like the reason for ENABLE_PTVSD was to get around the fact that some of the modes of the agent would reject the --debug flag. Now that it is in its own group Debugger, all modes can be supported.

@swcurran
Copy link
Contributor

Thanks — I think that looks good, but leave to the dev maintainers to weigh in — @jamshale @dbluhm @amanji @ianco .

Please address the file conflict — required before merging — which looks like is also causing the synk issue.

@rngadam rngadam force-pushed the rngadam/issue3182 branch from 69060c9 to 7e019ae Compare August 22, 2024 13:05
@rngadam rngadam force-pushed the rngadam/issue3182 branch from 0c2158e to 494acf2 Compare August 22, 2024 13:22
Copy link

@rngadam
Copy link
Contributor Author

rngadam commented Aug 22, 2024

Please address the file conflict — required before merging — which looks like is also causing the synk issue.

done. BTW seems like poetry content-hash merge conflict is a long-standing issue:

python-poetry/poetry#496

Copy link
Contributor

@jamshale jamshale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't develop or debug with this method so i'm not really sure how to test it properly. I did run the example command and the debugger attached.

It looks tested and that you know what you're doing so I'll approve.

@jamshale jamshale merged commit 3c79197 into openwallet-foundation:main Aug 22, 2024
8 checks passed
jamshale pushed a commit to jamshale/acapy that referenced this pull request Aug 22, 2024
…y debugpy (openwallet-foundation#3183)

* fixes openwallet-foundation#3182: migrate from ptvsd to debugpy

Signed-off-by: Ricky Ng-Adam <[email protected]>

* issue openwallet-foundation#3182: replace underlying debugger with debugpy

Signed-off-by: Ricky Ng-Adam <[email protected]>

* issue openwallet-foundation#3182: allow to debug provision and upgrade

Signed-off-by: Ricky Ng-Adam <[email protected]>

* issue openwallet-foundation#3182: introduce DAP_HOST and DAP_PORT

Signed-off-by: Ricky Ng-Adam <[email protected]>

* issue openwallet-foundation#3182: fix up conftest.py to use debugpy

Signed-off-by: Ricky Ng-Adam <[email protected]>

* issue openwallet-foundation#3182: update command-line help for --debug

Signed-off-by: Ricky Ng-Adam <[email protected]>

---------

Signed-off-by: Ricky Ng-Adam <[email protected]>
Signed-off-by: jamshale <[email protected]>
ff137 pushed a commit to ff137/acapy that referenced this pull request Aug 30, 2024
…y debugpy (openwallet-foundation#3183)

* fixes openwallet-foundation#3182: migrate from ptvsd to debugpy

Signed-off-by: Ricky Ng-Adam <[email protected]>

* issue openwallet-foundation#3182: replace underlying debugger with debugpy

Signed-off-by: Ricky Ng-Adam <[email protected]>

* issue openwallet-foundation#3182: allow to debug provision and upgrade

Signed-off-by: Ricky Ng-Adam <[email protected]>

* issue openwallet-foundation#3182: introduce DAP_HOST and DAP_PORT

Signed-off-by: Ricky Ng-Adam <[email protected]>

* issue openwallet-foundation#3182: fix up conftest.py to use debugpy

Signed-off-by: Ricky Ng-Adam <[email protected]>

* issue openwallet-foundation#3182: update command-line help for --debug

Signed-off-by: Ricky Ng-Adam <[email protected]>

---------

Signed-off-by: Ricky Ng-Adam <[email protected]>
darshilnb pushed a commit to Northern-Block/aries-cloudagent-python that referenced this pull request Sep 5, 2024
…y debugpy (openwallet-foundation#3183)

* fixes openwallet-foundation#3182: migrate from ptvsd to debugpy

Signed-off-by: Ricky Ng-Adam <[email protected]>

* issue openwallet-foundation#3182: replace underlying debugger with debugpy

Signed-off-by: Ricky Ng-Adam <[email protected]>

* issue openwallet-foundation#3182: allow to debug provision and upgrade

Signed-off-by: Ricky Ng-Adam <[email protected]>

* issue openwallet-foundation#3182: introduce DAP_HOST and DAP_PORT

Signed-off-by: Ricky Ng-Adam <[email protected]>

* issue openwallet-foundation#3182: fix up conftest.py to use debugpy

Signed-off-by: Ricky Ng-Adam <[email protected]>

* issue openwallet-foundation#3182: update command-line help for --debug

Signed-off-by: Ricky Ng-Adam <[email protected]>

---------

Signed-off-by: Ricky Ng-Adam <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants