From 6b1a9fe6b6099bd29bbb9b61d249bf0c1465595e Mon Sep 17 00:00:00 2001
From: Lonnie Liu <lonnie@anyscale.com>
Date: Wed, 3 Jan 2024 16:59:26 +0000
Subject: [PATCH] [profile manager] add verbose option

and prints profiler output on test failure

Signed-off-by: Lonnie Liu <lonnie@anyscale.com>
---
 dashboard/modules/reporter/profile_manager.py | 19 +++++-
 .../reporter/tests/test_profile_manager.py    | 68 ++++++++++---------
 2 files changed, 52 insertions(+), 35 deletions(-)

diff --git a/dashboard/modules/reporter/profile_manager.py b/dashboard/modules/reporter/profile_manager.py
index db50facef43a..bb750b1cc97d 100644
--- a/dashboard/modules/reporter/profile_manager.py
+++ b/dashboard/modules/reporter/profile_manager.py
@@ -263,7 +263,11 @@ async def get_profile_result(
         return True, open(profile_visualize_path, "rb").read()
 
     async def attach_profiler(
-        self, pid: int, native: bool = False, trace_python_allocators: bool = False
+        self,
+        pid: int,
+        native: bool = False,
+        trace_python_allocators: bool = False,
+        verbose: bool = False,
     ) -> (bool, str):
         """
         Attach a Memray profiler to a specified process.
@@ -275,6 +279,8 @@ async def attach_profiler(
                 Default is False.
             trace_python_allocators (bool, optional): If True, includes Python
                 stack frames. Default is False.
+            verbose (bool, optional): If True, enables verbose output.
+                Default is False.
 
         Returns:
             Tuple[bool, str]: A tuple containing a boolean indicating the success
@@ -293,6 +299,8 @@ async def attach_profiler(
             cmd.append("--native")
         if trace_python_allocators:
             cmd.append("--trace-python-allocators")
+        if verbose:
+            cmd.append("--verbose")
         if await _can_passwordless_sudo():
             cmd = ["sudo", "-n"] + cmd
 
@@ -321,6 +329,7 @@ async def attach_profiler(
     async def detach_profiler(
         self,
         pid: int,
+        verbose: bool = False,
     ) -> (bool, str):
         """
         Detach a profiler from a specified process.
@@ -328,6 +337,8 @@ async def detach_profiler(
         Args:
             pid: The process ID (PID) of the target process the
                 profiler detached from.
+            verbose (bool, optional): If True, enables verbose output.
+                Default is False.
 
         Returns:
             Tuple[bool, str]: A tuple containing a boolean indicating the success
@@ -337,7 +348,11 @@ async def detach_profiler(
         if memray is None:
             return False, "Failed to execute: memray is not installed"
 
-        cmd = [memray, "detach", str(pid)]
+        cmd = [memray, "detach"]
+        if verbose:
+            cmd.append("--verbose")
+        cmd.append(str(pid))
+
         process = await asyncio.create_subprocess_exec(
             *cmd,
             stdout=subprocess.PIPE,
diff --git a/dashboard/modules/reporter/tests/test_profile_manager.py b/dashboard/modules/reporter/tests/test_profile_manager.py
index 38e4ed8fa6bd..c7e04de76679 100644
--- a/dashboard/modules/reporter/tests/test_profile_manager.py
+++ b/dashboard/modules/reporter/tests/test_profile_manager.py
@@ -1,7 +1,7 @@
 import sys
 import pytest
-import shutil
 import os
+import tempfile
 import time
 from unittest.mock import patch
 
@@ -12,26 +12,22 @@
 
 @pytest.fixture
 def setup_memory_profiler():
-    profiler_result_path = os.getcwd()
-    memory_profiler = MemoryProfilingManager(profiler_result_path)
+    with tempfile.TemporaryDirectory() as tmpdir:
+        memory_profiler = MemoryProfilingManager(tmpdir)
 
-    @ray.remote
-    class Actor:
-        def getpid(self):
-            return os.getpid()
+        @ray.remote
+        class Actor:
+            def getpid(self):
+                return os.getpid()
 
-        def long_run(self):
-            print("Long-running task began.")
-            time.sleep(1000)
-            print("Long-running task completed.")
+            def long_run(self):
+                print("Long-running task began.")
+                time.sleep(1000)
+                print("Long-running task completed.")
 
-    actor = Actor.remote()
+        actor = Actor.remote()
 
-    yield actor, memory_profiler
-
-    cleanup_dir = memory_profiler.profile_dir_path
-    if os.path.exists(cleanup_dir):
-        shutil.rmtree(cleanup_dir)
+        yield actor, memory_profiler
 
 
 @pytest.mark.asyncio
@@ -50,9 +46,11 @@ async def test_basic_attach_profiler(self, setup_memory_profiler, shutdown_only)
         actor, memory_profiler = setup_memory_profiler
         pid = ray.get(actor.getpid.remote())
         actor.long_run.remote()
-        success, profiler_filename, message = await memory_profiler.attach_profiler(pid)
+        success, profiler_filename, message = await memory_profiler.attach_profiler(
+            pid, verbose=True
+        )
 
-        assert success
+        assert success, message
         assert f"Success attaching memray to process {pid}" in message
         assert profiler_filename in os.listdir(memory_profiler.profile_dir_path)
 
@@ -61,14 +59,16 @@ async def test_profiler_multiple_attach(self, setup_memory_profiler, shutdown_on
         actor, memory_profiler = setup_memory_profiler
         pid = ray.get(actor.getpid.remote())
         actor.long_run.remote()
-        success, profiler_filename, message = await memory_profiler.attach_profiler(pid)
+        success, profiler_filename, message = await memory_profiler.attach_profiler(
+            pid, verbose=True
+        )
 
-        assert success
+        assert success, message
         assert f"Success attaching memray to process {pid}" in message
         assert profiler_filename in os.listdir(memory_profiler.profile_dir_path)
 
         success, _, message = await memory_profiler.attach_profiler(pid)
-        assert success
+        assert success, message
         assert f"Success attaching memray to process {pid}" in message
 
     async def test_detach_profiler_successful(
@@ -78,11 +78,11 @@ async def test_detach_profiler_successful(
         actor, memory_profiler = setup_memory_profiler
         pid = ray.get(actor.getpid.remote())
         actor.long_run.remote()
-        success, _, message = await memory_profiler.attach_profiler(pid)
-        assert success
+        success, _, message = await memory_profiler.attach_profiler(pid, verbose=True)
+        assert success, message
 
-        success, message = await memory_profiler.detach_profiler(pid)
-        assert success
+        success, message = await memory_profiler.detach_profiler(pid, verbose=True)
+        assert success, message
         assert f"Success detaching memray from process {pid}" in message
 
     async def test_detach_profiler_without_attach(
@@ -93,7 +93,7 @@ async def test_detach_profiler_without_attach(
         pid = ray.get(actor.getpid.remote())
 
         success, message = await memory_profiler.detach_profiler(pid)
-        assert not success
+        assert not success, message
         assert "Failed to execute" in message
         assert "no previous `memray attach`" in message
 
@@ -116,7 +116,7 @@ async def test_profiler_attach_process_not_found(
         _, memory_profiler = setup_memory_profiler
         pid = 123456
         success, _, message = await memory_profiler.attach_profiler(pid)
-        assert not success
+        assert not success, message
         assert "Failed to execute" in message
         assert "The given process ID does not exist" in message
 
@@ -127,8 +127,10 @@ async def test_profiler_get_profiler_result(
         actor, memory_profiler = setup_memory_profiler
         pid = ray.get(actor.getpid.remote())
         actor.long_run.remote()
-        success, profiler_filename, message = await memory_profiler.attach_profiler(pid)
-        assert success
+        success, profiler_filename, message = await memory_profiler.attach_profiler(
+            pid, verbose=True
+        )
+        assert success, message
         assert f"Success attaching memray to process {pid}" in message
 
         # get profiler result in flamegraph and table format
@@ -139,10 +141,10 @@ async def test_profiler_get_profiler_result(
                 pid, profiler_filename=profiler_filename, format=format
             )
             if format in supported_formats:
-                assert success
+                assert success, message
                 assert f"{format} report" in message.decode("utf-8")
             else:
-                assert not success
+                assert not success, message
                 assert f"{format} is not supported" in message
 
     async def test_profiler_result_not_exist(
@@ -156,7 +158,7 @@ async def test_profiler_result_not_exist(
         success, message = await memory_profiler.get_profile_result(
             pid, profiler_filename=profiler_filename, format=format
         )
-        assert not success
+        assert not success, message
         assert f"process {pid} has not been profiled" in message