-
Notifications
You must be signed in to change notification settings - Fork 159
feat: Add debugger flag that can be turned on via RAY_DEBUG=legacy #1312
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
Conversation
Signed-off-by: Guyue Huang <guyueh@nvidia.com>
📝 WalkthroughWalkthroughIntroduces RAY_DEBUGGER_ARGS, set to "--ray-debugger-external" when RAY_DEBUG="legacy", and injects it into the ray start commands for head and worker nodes before the --block flag. Includes minor formatting updates within the same script. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Script as ray.sub
participant Ray as ray start
User->>Script: Invoke ray start (head/worker)
Script->>Script: Check RAY_DEBUG
alt RAY_DEBUG="legacy"
Script->>Script: Set RAY_DEBUGGER_ARGS="--ray-debugger-external"
else
Script->>Script: Set RAY_DEBUGGER_ARGS=""
end
Script->>Ray: Start with args + $RAY_DEBUGGER_ARGS + --block
Ray-->>User: Process runs (blocked)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ray.sub (1)
67-70
: QuoteRAY_DEBUG
in the condition and rely on existing docs
Documentation forRAY_DEBUG=legacy
already exists indocs/debugging.md
. Guard against unset variables by quoting in the condition:- if [ $RAY_DEBUG = "legacy" ]; then + if [ "$RAY_DEBUG" = "legacy" ]; thenOptionally set a default or emit a warning when
RAY_DEBUG
is unset.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ray.sub
(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: sphinx-build / Build docs
- GitHub Check: Lint check
- GitHub Check: Lint check
- GitHub Check: Post submodule check comment / Comment on PR
- GitHub Check: Post automodel integration comment / Comment on PR
🔇 Additional comments (2)
ray.sub (2)
283-285
: LGTM: Debugger args correctly positioned before --block.The
$RAY_DEBUGGER_ARGS
is correctly inserted before the--block
flag, ensuring proper command structure for the head node.Note: Line 285 has a trailing space after
--block
, which is a minor formatting issue but does not affect functionality.
383-385
: LGTM: Debugger args correctly positioned before --block.The
$RAY_DEBUGGER_ARGS
is correctly inserted before the--block
flag for worker nodes, maintaining consistency with the head node configuration.Note: Line 385 has a trailing space after
--block
, which is a minor formatting issue but does not affect functionality.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Guyue Huang <140554423+guyueh1@users.noreply.github.com>
@guyueh1 for my education, what does this change enable that you couldn't do with https://docs.nvidia.com/nemo/rl/latest/debugging.html ? |
@terrykong this is an alternative way, won't enable more than the vscode debugger; it is just a way to run ray debug even without IDE. The way to debug is
|
oh interesting, so before when i set if so, that's awesome. do you think you could update https://github.com/NVIDIA-NeMo/RL/blob/main/docs/debugging.md with this new method? |
@terrykong yes, you need to set both RAY_DEBUG=legacy and I have updated our doc. |
@jgerh to review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Completed tech pubs review and left a few copyedits and suggested text revisions.
@terrykong i think this is ready for merge, if L0 test is enough |
What does this PR do ?
If user set RAY_DEBUG=legacy before running
ray.sub
, the debug mode will be turned on and user can attach to breakpoints viaray debug
.Issues
List issues that this PR closes (syntax):
Usage
# Add a code snippet demonstrating how to use this
Before your PR is "Ready for review"
Pre checks:
Additional Information
Summary by CodeRabbit