fix: time conversion for efficiency reports with jobs taking longer than 1 day (#362)

based on PR #360 and issue #361:

The current time conversion for the efficiency report does not consider
long jobs, where `sacct` output may be in the D-HH:MM:SS format. Also,
the conversion of fractional seconds was not taken into account. While
the latter is actually ridiculously playing with accuracy, the HPC world
sometimes is concerned about it.

Included test cases.

This PR is based upon #360 and might replace it.

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **Bug Fixes**
* Improved SLURM time parsing in efficiency reports to robustly handle
many sacct formats (days, hours, minutes, seconds), fractional seconds,
varied HH:MM:SS variants and whitespace; treats invalid/empty/NA inputs
as 0 to improve runtime and CPU metric accuracy.

* **Tests**
* Added comprehensive tests validating diverse sacct time formats,
fractional seconds, whitespace and NA handling, and integration with
existing parsing checks.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Co-authored-by: FelixKnopp13 <[email protected]>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

Author: 3 people

snakemake_executor_plugin_slurm/efficiency_report.py

@@ -3,24 +3,67 @@
 from pathlib import Path
 import subprocess
 import shlex
-
-import os  # only temporarily needed for printf debugging
+from datetime import datetime
 import numpy as np
+import os
 
 
 def time_to_seconds(time_str):
-    """Convert SLURM time format to seconds."""
-    if pd.isna(time_str) or time_str.strip() == "":
+    """
+    Convert SLURM sacct time format to seconds.
+
+    Handles sacct output formats:
+    - Elapsed: [D-]HH:MM:SS or [DD-]HH:MM:SS (no fractional seconds)
+    - TotalCPU: [D-][HH:]MM:SS or [DD-][HH:]MM:SS (with fractional seconds)
+
+    Examples:
+    - "1-12:30:45" -> 131445 seconds (1 day + 12h 30m 45s)
+    - "23:59:59" -> 86399 seconds
+    - "45:30" -> 2730 seconds (45 minutes 30 seconds)
+    - "30.5" -> 30.5 seconds (fractional seconds for TotalCPU)
+    """
+    if (
+        pd.isna(time_str)
+        or str(time_str).strip() == ""
+        or str(time_str).strip() == "invalid"
+    ):
         return 0
-    parts = time_str.split(":")
-
-    if len(parts) == 3:  # H:M:S
-        return int(parts[0]) * 3600 + int(parts[1]) * 60 + float(parts[2])
-    elif len(parts) == 2:  # M:S
-        return int(parts[0]) * 60 + float(parts[1])
-    elif len(parts) == 1:  # S
-        return float(parts[0])
-    return 0
+
+    time_str = str(time_str).strip()
+
+    # Try different SLURM time formats with datetime parsing
+    time_formats = [
+        "%d-%H:%M:%S.%f",  # D-HH:MM:SS.ffffff (with fractional seconds)
+        "%d-%H:%M:%S",  # D-HH:MM:SS
+        "%d-%M:%S",  # D-MM:SS
+        "%d-%M:%S.%f",  # D-MM:SS.ffffff (with fractional seconds)
+        "%H:%M:%S.%f",  # HH:MM:SS.ffffff (with fractional seconds)
+        "%H:%M:%S",  # HH:MM:SS
+        "%M:%S.%f",  # MM:SS.ffffff (with fractional seconds)
+        "%M:%S",  # MM:SS
+        "%S.%f",  # SS.ffffff (with fractional seconds)
+        "%S",  # SS
+    ]
+
+    for fmt in time_formats:
+        try:
+            time_obj = datetime.strptime(time_str, fmt)
+
+            total_seconds = (
+                time_obj.hour * 3600
+                + time_obj.minute * 60
+                + time_obj.second
+                + time_obj.microsecond / 1000000
+            )
+
+            # Add days if present (datetime treats day 1 as the first day)
+            if fmt.startswith("%d-"):
+                total_seconds += time_obj.day * 86400
+
+            return total_seconds
+        except ValueError:
+            continue
+    return 0  # If all parsing attempts fail, return 0
 
 
 def parse_maxrss(maxrss):

tests/tests.py

@@ -9,10 +9,14 @@
 import pytest
 
 from snakemake_executor_plugin_slurm import ExecutorSettings
-from snakemake_executor_plugin_slurm.efficiency_report import parse_sacct_data
+from snakemake_executor_plugin_slurm.efficiency_report import (
+    parse_sacct_data,
+    time_to_seconds,
+)
 from snakemake_executor_plugin_slurm.utils import set_gres_string
 from snakemake_executor_plugin_slurm.submit_string import get_submit_command
 from snakemake_interface_common.exceptions import WorkflowError
+import pandas as pd
 
 
 class TestWorkflows(snakemake.common.tests.TestWorkflowsLocalStorageBase):
@@ -59,6 +63,115 @@ def test_parse_sacct_data():
     assert df.iloc[0]["MaxRSS_MB"] > 0  # Should have actual memory usage from job step
 
 
+class TestTimeToSeconds:
+    """Test the time_to_seconds function with SLURM sacct time formats."""
+
+    def test_elapsed_format_with_days(self):
+        """
+        Test Elapsed format: [D-]HH:MM:SS or
+        [DD-]HH:MM:SS (no fractional seconds).
+        """
+        # Single digit days
+        assert time_to_seconds("1-00:00:00") == 86400  # 1 day
+        assert (
+            time_to_seconds("1-12:30:45") == 86400 + 12 * 3600 + 30 * 60 + 45
+        )  # 131445
+        assert time_to_seconds("9-23:59:59") == 9 * 86400 + 23 * 3600 + 59 * 60 + 59
+
+        # Double digit days
+        assert (
+            time_to_seconds("10-01:02:03") == 10 * 86400 + 1 * 3600 + 2 * 60 + 3
+        )  # 867723
+
+    def test_elapsed_format_hours_minutes_seconds(self):
+        """Test Elapsed format: HH:MM:SS (no fractional seconds)."""
+        assert time_to_seconds("00:00:00") == 0
+        assert time_to_seconds("01:00:00") == 3600  # 1 hour
+        assert time_to_seconds("23:59:59") == 23 * 3600 + 59 * 60 + 59  # 86399
+        assert time_to_seconds("12:30:45") == 12 * 3600 + 30 * 60 + 45  # 45045
+
+    def test_totalcpu_format_with_days(self):
+        """
+        Test TotalCPU format: [D-][HH:]MM:SS or [DD-][HH:]MM:SS
+        (with fractional seconds).
+        """
+        # With days and hours
+        assert time_to_seconds("1-12:30:45.5") == 86400 + 12 * 3600 + 30 * 60 + 45.5
+        assert (
+            time_to_seconds("10-01:02:03.123") == 10 * 86400 + 1 * 3600 + 2 * 60 + 3.123
+        )
+
+        # With days, no hours (MM:SS format)
+        assert time_to_seconds("1-30:45") == 86400 + 30 * 60 + 45
+        assert time_to_seconds("1-30:45.5") == 86400 + 30 * 60 + 45.5
+
+    def test_totalcpu_format_minutes_seconds(self):
+        """Test TotalCPU format: MM:SS with fractional seconds."""
+        assert time_to_seconds("00:00") == 0
+        assert time_to_seconds("01:00") == 60  # 1 minute
+        assert time_to_seconds("59:59") == 59 * 60 + 59  # 3599
+        assert time_to_seconds("30:45") == 30 * 60 + 45  # 1845
+        assert time_to_seconds("30:45.5") == 30 * 60 + 45.5  # 1845.5
+
+    def test_totalcpu_format_seconds_only(self):
+        """Test TotalCPU format: SS or SS.sss (seconds only with fractional)."""
+        assert time_to_seconds("0") == 0
+        assert time_to_seconds("1") == 1
+        assert time_to_seconds("30") == 30
+        assert time_to_seconds("59") == 59
+
+        # Fractional seconds
+        assert time_to_seconds("30.5") == 30.5
+        assert time_to_seconds("0.5") == 0.5
+
+    def test_real_world_sacct_examples(self):
+        """Test with realistic sacct time values from actual output."""
+        # From your test data
+        assert time_to_seconds("00:01:31") == 91  # 1 minute 31 seconds
+        assert time_to_seconds("00:24.041") == 24.041  # 24.041 seconds
+        assert time_to_seconds("00:03.292") == 3.292  # 3.292 seconds
+        assert time_to_seconds("00:20.749") == 20.749  # 20.749 seconds
+
+        # Longer running jobs
+        assert time_to_seconds("02:15:30") == 2 * 3600 + 15 * 60 + 30  # 2h 15m 30s
+        assert time_to_seconds("1-12:00:00") == 86400 + 12 * 3600  # 1 day 12 hours
+        assert time_to_seconds("7-00:00:00") == 7 * 86400  # 1 week
+
+    def test_empty_and_invalid_inputs(self):
+        """Test empty, None, and invalid inputs."""
+        assert time_to_seconds("") == 0
+        assert time_to_seconds("   ") == 0
+        assert time_to_seconds(None) == 0
+        assert time_to_seconds(pd.NA) == 0
+        assert time_to_seconds("invalid") == 0
+        assert time_to_seconds("1:2:3:4") == 0  # Too many colons
+        assert time_to_seconds("abc:def") == 0
+        assert time_to_seconds("-1:00:00") == 0  # Negative values
+
+    def test_whitespace_handling(self):
+        """Test that whitespace is properly handled."""
+        assert time_to_seconds(" 30 ") == 30
+        assert time_to_seconds("  1-02:30:45  ") == 86400 + 2 * 3600 + 30 * 60 + 45
+        assert time_to_seconds("\t12:30:45\n") == 12 * 3600 + 30 * 60 + 45
+
+    def test_pandas_na_values(self):
+        """Test pandas NA and NaN values."""
+        assert time_to_seconds(pd.NA) == 0
+        assert (
+            time_to_seconds(pd.NaType()) == 0 if hasattr(pd, "NaType") else True
+        )  # Skip if not available
+
+    def test_edge_case_values(self):
+        """Test edge case values that might appear in SLURM output."""
+        # Zero padding variations (should work with datetime parsing)
+        assert time_to_seconds("01:02:03") == 1 * 3600 + 2 * 60 + 3
+        assert time_to_seconds("1:2:3") == 1 * 3600 + 2 * 60 + 3
+
+        # Single digit values
+        assert time_to_seconds("5") == 5
+        assert time_to_seconds("1:5") == 1 * 60 + 5
+
+
 class TestEfficiencyReport(snakemake.common.tests.TestWorkflowsLocalStorageBase):
     __test__ = True