Address PR review feedback

- Simplify get_threshold_for_case() to only handle interactive mode
- Rename notified_30s to notified_interactive for clarity
- Add type hints to function parameters for consistency
- Improve docstrings for better documentation
- Change 'Running (long): -' to 'Running (long): none'
- Fix progress tracker spacing for consistent alignment
- Improve time_label formatting to handle non-integer minutes

Author: sbryngelson

toolchain/mfc/sched.py

@@ -47,10 +47,10 @@ class WorkerThreadHolder:
     task:    typing.Optional['Task'] = None
     start:   float = 0.0
     # Track which milestones we've already logged
-    notified_30s: bool = False  # for interactive mode
-    notified_2m:  bool = False
-    notified_10m: bool = False
-    notified_30m: bool = False
+    notified_interactive: bool = False  # First notification in interactive mode (time varies by dimensionality)
+    notified_2m:  bool = False  # Headless mode: 2 minute milestone
+    notified_10m: bool = False  # Headless mode: 10 minute milestone
+    notified_30m: bool = False  # Headless mode: 30 minute milestone
 
 
 @dataclasses.dataclass
@@ -66,8 +66,13 @@ def sched(tasks: typing.List[Task], nThreads: int, devices: typing.Set[int] = No
 
     sched.LOAD = { id: 0.0 for id in devices or [] }
 
-    def get_case_dimensionality(case) -> int:
-        """Determine if a test case is 1D, 2D, or 3D based on m, n, p parameters."""
+    def get_case_dimensionality(case: typing.Any) -> int:
+        """
+        Determine if a test case is 1D, 2D, or 3D based on grid parameters.
+        
+        Grid parameters (m, n, p) represent the number of cells in x, y, z directions.
+        Returns 3 if p != 0, 2 if n != 0, otherwise 1. Defaults to 1D if params unavailable.
+        """
         if not hasattr(case, 'params'):
             return 1  # Default to 1D if we can't determine
 
@@ -82,16 +87,27 @@ def get_case_dimensionality(case) -> int:
         else:
             return 1  # 1D
 
-    def get_threshold_for_case(case, interactive: bool) -> float:
-        """Get the appropriate threshold for a test case."""
-        if interactive:
-            dim = get_case_dimensionality(case)
-            return INTERACTIVE_THRESHOLDS.get(dim, INTERACTIVE_THRESHOLDS[1])
-        else:
-            # Headless mode uses fixed thresholds
-            return HEADLESS_THRESHOLDS[0][0]  # 2 minutes
-
-    def notify_long_running_threads(progress, running_tracker, interactive: bool) -> None:
+    def get_threshold_for_case(case: typing.Any) -> float:
+        """
+        Get the dimension-aware time threshold (in seconds) for interactive mode notifications.
+        
+        Returns 30s for 1D, 60s for 2D, 120s for 3D tests.
+        """
+        dim = get_case_dimensionality(case)
+        return INTERACTIVE_THRESHOLDS.get(dim, INTERACTIVE_THRESHOLDS[1])
+
+    def notify_long_running_threads(
+        progress: rich.progress.Progress,
+        running_tracker: typing.Optional[rich.progress.TaskID],
+        interactive: bool
+    ) -> None:
+        """
+        Monitor and notify about long-running tests.
+        
+        In interactive mode: prints once when a test crosses its dimension-aware threshold
+        and updates the live progress bar. In headless mode: prints milestone notifications
+        at 2, 10, and 30 minutes.
+        """
         now = time.time()
         long_running_for_progress = []
 
@@ -106,21 +122,21 @@ def notify_long_running_threads(progress, running_tracker, interactive: bool) ->
 
             # --- interactive: dimension-aware thresholds ---
             if interactive:
-                threshold = get_threshold_for_case(case, interactive=True)
+                threshold = get_threshold_for_case(case)
 
                 if elapsed >= threshold:
                     long_running_for_progress.append((case_uuid, case_trace))
 
                     # Print explicit line once when crossing threshold
-                    if not holder.notified_30s:
+                    if not holder.notified_interactive:
                         dim = get_case_dimensionality(case)
                         dim_label = f"{dim}D"
-                        time_label = f"{int(threshold)}s" if threshold < 60 else f"{int(threshold/60)}min"
+                        time_label = f"{int(threshold)}s" if threshold < 60 else f"{threshold/60:.0f}min"
                         cons.print(
                             f"  [italic yellow]Still running[/italic yellow] ({dim_label}, >{time_label}) "
                             f"[bold magenta]{case_uuid}[/bold magenta]  {case_trace}"
                         )
-                        holder.notified_30s = True
+                        holder.notified_interactive = True
 
             # --- headless: milestone notifications at 2, 10, 30 minutes ---
             else:
@@ -156,7 +172,7 @@ def notify_long_running_threads(progress, running_tracker, interactive: bool) ->
                     summary += f", +{len(long_running_for_progress) - 5} more"
                 progress.update(running_tracker, description=f"Running (long): {summary}")
             else:
-                progress.update(running_tracker, description="Running (long): -")
+                progress.update(running_tracker, description="Running (long): none")
 
     def join_first_dead_thread(progress, complete_tracker, interactive: bool) -> None:
         nonlocal threads, nAvailable
@@ -197,7 +213,7 @@ def join_first_dead_thread(progress, complete_tracker, interactive: bool) -> Non
                         raise threadHolder.thread.exc
 
                 # Print completion message for long-running tests in interactive mode
-                if interactive and threadHolder.notified_30s:
+                if interactive and threadHolder.notified_interactive:
                     elapsed = time.time() - threadHolder.start
                     case = threadHolder.task.args[0] if threadHolder.task and threadHolder.task.args else None
                     case_uuid  = case.get_uuid() if hasattr(case, "get_uuid") else "unknown"
@@ -219,9 +235,9 @@ def join_first_dead_thread(progress, complete_tracker, interactive: bool) -> Non
 
     with rich.progress.Progress(console=cons.raw, transient=True) as progress:
         interactive      = cons.raw.is_terminal
-        queue_tracker    = progress.add_task("Queued   ", total=len(tasks))
-        complete_tracker = progress.add_task("Completed", total=len(tasks))
-        running_tracker  = progress.add_task("Running  ", total=None) if interactive else None
+        queue_tracker    = progress.add_task("Queued    ", total=len(tasks))
+        complete_tracker = progress.add_task("Completed ", total=len(tasks))
+        running_tracker  = progress.add_task("Running   ", total=None) if interactive else None
 
         # Queue Tests
         for task in tasks: