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

[Observability / Doc] Add support of ray debugger on windows #44902

Closed
scottsun94 opened this issue Apr 22, 2024 · 8 comments · Fixed by #48117
Closed

[Observability / Doc] Add support of ray debugger on windows #44902

scottsun94 opened this issue Apr 22, 2024 · 8 comments · Fixed by #48117
Assignees
Labels
enhancement Request for new feature and/or capability observability Issues related to the Ray Dashboard, Logging, Metrics, Tracing, and/or Profiling P2 Important issue, but not time-critical QS Quantsight triage label windows

Comments

@scottsun94
Copy link
Contributor

Description

According to https://discuss.ray.io/t/solution-found-using-rays-debugger-on-windows/14474, users need to modify the source code for ray debugger to work on windows.

  1. We should just make it work on windows (it seems a trivial change)
  2. if we don't get time to do 1, update the doc to clarify the limitation.

Link

No response

@scottsun94 scottsun94 added triage Needs triage (eg: priority, bug/not-bug, and owning component) docs An issue or change related to documentation observability Issues related to the Ray Dashboard, Logging, Metrics, Tracing, and/or Profiling labels Apr 22, 2024
@scottsun94
Copy link
Contributor Author

@anyscalesam

@OpenCoderX
Copy link

Nice, I came here to propose my own very similar fix to the _connect_pdb_client function, i've been using a locally patched version for a few weeks and have not found an issue. I'm glad to see a windows fix come in.

@JiangpengLI86
Copy link

Hi @OpenCoderX,

Have you tried my solution on your Windows PC? This modified function works on my computer, but I am not sure about the others.

Regards,

JL

@OpenCoderX
Copy link

I'll test your version.

@anyscalesam anyscalesam added windows QS Quantsight triage label labels Apr 25, 2024
@anyscalesam anyscalesam removed the triage Needs triage (eg: priority, bug/not-bug, and owning component) label Apr 25, 2024
@anyscalesam
Copy link
Contributor

@mattip this seems related to the rpdb cross platform problem we discussed earlier in the morning; can you confirm? than prioritize to work on it after the 3-4 items we discussed.

@anyscalesam anyscalesam added the P1 Issue that should be fixed within a few weeks label Apr 25, 2024
@mattip
Copy link
Contributor

mattip commented Apr 26, 2024

The suggested change is this, which should also fix the previously mentioned #37145. I will turn this into a PR. Is this code tested?

diff --git a/python/ray/util/rpdb.py b/python/ray/util/rpdb.py
index e6a9769966..c5979d2c88 100644
--- a/python/ray/util/rpdb.py
+++ b/python/ray/util/rpdb.py
@@ -338,16 +338,24 @@ def _post_mortem():
 
 
 def _connect_pdb_client(host, port):
+    if sys.platform == 'win32':
+        import msvcrt
+
     s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
     s.connect((host, port))
 
     while True:
         # Get the list of sockets which are readable.
-        read_sockets, write_sockets, error_sockets = select.select(
-            [sys.stdin, s], [], []
+        if sys.platform == 'win32':
+            ready_to_read = select.select([s], [], [], 1)[0]
+            if msvcrt.kbhit():
+                ready_to_read.append(sys.stdin)
+        else:
+            ready_to_read, write_sockets, error_sockets = select.select(
+                [sys.stdin, s], [], []
         )
 
-        for sock in read_sockets:
+        for sock in ready_to_read:
             if sock == s:
                 # Incoming message from remote debugger.
                 data = sock.recv(4096)

@anyscalesam anyscalesam added P2 Important issue, but not time-critical and removed P1 Issue that should be fixed within a few weeks labels Jul 31, 2024
@anyscalesam
Copy link
Contributor

merge it! :) @mattip

@anyscalesam anyscalesam added enhancement Request for new feature and/or capability P1.5 Issues that will be fixed in a couple releases. It will be bumped once all P1s are cleared and removed docs An issue or change related to documentation P2 Important issue, but not time-critical labels Oct 9, 2024
@mattip
Copy link
Contributor

mattip commented Oct 20, 2024

Is this code tested?

This is tested in test_ray_debugger_commands, test_ray_debugger_stepping, and test_ray_debugger_recursive in python/ray/tests/test_ray_debugger.py. They are currently skipped on windows. I see they use pexpect.spawn, which does not work on windows. I will see if I can get the alternative mentioned in the documentation, with many caveats, to work.

@jjyao jjyao added P2 Important issue, but not time-critical and removed P1.5 Issues that will be fixed in a couple releases. It will be bumped once all P1s are cleared labels Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Request for new feature and/or capability observability Issues related to the Ray Dashboard, Logging, Metrics, Tracing, and/or Profiling P2 Important issue, but not time-critical QS Quantsight triage label windows
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants