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

WIP: Replace ptvsd with debugpy to match modern VS Code #7110

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

justinclift
Copy link
Member

@justinclift justinclift commented Aug 6, 2024

What type of PR is this?

  • Other

Description

VS Code has moved to using debugpy for debugging rather than the old (archived) ptvsd.

This (very simple) PR updates our code to use debugpy now too.

NOTE - This PR is a work in progress. 😄

How is this tested?

  • Unit tests (pytest, jest)
  • E2E Tests (Cypress)
  • Manually
  • N/A

Related Tickets & Documents

Useful reference info: https://github.com/microsoft/debugpy/wiki/Switching-over-from-%60ptvsd%60-to-%60debugpy%60

Redash documentation that will need updating: https://redash.io/help/open-source/dev-guide/debugging/ (source)

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

@justinclift
Copy link
Member Author

justinclift commented Sep 5, 2024

@gaecoli @wlach @eradman @lucydodo How are you doing interactive debugging of Redash these days?

I'm trying this debugpy approach on macOS with Visual Studio Code, and it seems to be working ok for the server process.

Haven't tried it for debugging a worker process yet though. My suspicion is we'll need to add some extra code for that, though I'm not 100% sure.

My initial thought for that is perhaps something like this in an appropriate class (TBD) in something common to all the workers:

if os.environ.get("REMOTE_DEBUG_WORKER"):
    import debugpy

    debugpy.listen(("0.0.0.0", 5679))
    debugpy.wait_for_client()

Note the different port (5679) there than the debug port used for the server (5678). I'm also not sure if we should do it that way - which would potentially enabling debugging both server and worker at the same time - or if we'd want to force just debugging a single thing at a time.

Thoughts?

@eradman
Copy link
Collaborator

eradman commented Sep 5, 2024

I have not used remote Python debugging, so I'll defer to others on this.

@justinclift
Copy link
Member Author

No worries Eric. What's the approach you're using for debugging? Starting the individual processes locally then?

@gaecoli
Copy link
Member

gaecoli commented Sep 6, 2024

I have not used remote Python debugging, so I'll defer to others on this.

me toooooo.

Copy link
Collaborator

@eradman eradman left a comment

Choose a reason for hiding this comment

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

In order debug a handler I had to use VSCode + debugpy, and found that it worked nicely!

To add a breakpoint I had to import debugpy in the module I was trying to trace

index 0a7b6749..bfdfb07a 100644
--- a/redash/handlers/query_results.py
+++ b/redash/handlers/query_results.py
@@ -36,6 +36,8 @@ from redash.utils import (
     to_filename,
 )

+import debugpy
+
 def error_response(message, http_status=400):
     return {"job": {"status": 4, "error": message}}, http_status

@@ -329,6 +331,7 @@ class QueryResultResource(BaseResource):
         if query_result:
+            debugpy.breakpoint()
             require_access(query_result.data_source, self.current_user, view_only)

Then restart the server using

docker compose stop server && docker compose run --rm --service-ports server debug

This is valuable; @justinclift I'd say go ahead and rebase this change

@justinclift
Copy link
Member Author

Thanks @eradman, I'll get to this in a few hours when I get to the co-working space. 😄

@justinclift
Copy link
Member Author

So much for "in a few hours". I'll get to this probably Tuesday instead. 😬

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