Add Torch Trace GPU profiling feature for PyTorch workers#60727
Add Torch Trace GPU profiling feature for PyTorch workers#60727aryan-verma-ai-2 wants to merge 1 commit intoray-project:masterfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable GPU profiling feature for PyTorch workers, accessible through the Ray Dashboard. The implementation is solid, adding new UI components and a command handler that correctly manages the profiling process, including state management and file download. I've identified a couple of areas for improvement to enhance robustness. One suggestion is to refine the regex for filename parsing to handle more complex content-disposition headers. Another, more critical suggestion, is to use the worker.language property for identifying Python workers, which is more reliable than checking the command line and ensures feature consistency across the dashboard.
| <CpuStackTraceLink pid={pid} nodeId={nodeId} type="" /> | ||
| <br /> | ||
| <MemoryProfilingButton pid={pid} nodeId={nodeId} /> | ||
| {cmdline[0] === "python" && coreWorker?.ipAddress && ( |
There was a problem hiding this comment.
The check cmdline[0] === "python" to determine if a worker is a Python worker is brittle. It will fail for commands like python3 or full paths like /usr/bin/python. The worker object has a language property which is more reliable for this check. You should destructure language from the worker object (around line 239) and use language === "PYTHON" here for a more robust check, which is also consistent with WorkerTable.tsx.
| {cmdline[0] === "python" && coreWorker?.ipAddress && ( | |
| {language === "PYTHON" && coreWorker?.ipAddress && ( |
| let filename = "gputrace.json"; | ||
| const contentDisposition = response.headers.get("content-disposition"); | ||
| if (contentDisposition) { | ||
| const filenameMatch = contentDisposition.match(/filename="(.+)"/); |
There was a problem hiding this comment.
The regex used to extract the filename from the content-disposition header is a bit too greedy. If the header contains other attributes after the filename, they might be incorrectly included in the matched filename. Using a more specific, non-greedy regex will make this more robust.
| const filenameMatch = contentDisposition.match(/filename="(.+)"/); | |
| const filenameMatch = contentDisposition.match(/filename="([^"]+)"/); |
|
This pull request has been automatically marked as stale because it has not had You can always ask for help on our discussion forum or Ray's public slack channel. If you'd like to keep this open, just leave any comment, and the stale label will be removed. |
|
This pull request has been automatically closed because there has been no more activity in the 14 days Please feel free to reopen or open a new pull request if you'd still like this to be addressed. Again, you can always ask for help on our discussion forum or Ray's public slack channel. Thanks again for your contribution! |
Description
Related issues
Additional information