fix: time_to_seconds to include day format D-HH:MM:SS

[Cheshakoneko]

This PR extends the time_to_seconds function to correctly parse the D-HH:MM:SS time format, which SLURM can also return. The function splits the time string if a hyphen - is present and multiplies the number of days by 86400 seconds.

Summary by CodeRabbit

• New Features

  • Expanded time parsing to support SLURM formats: D-HH:MM:SS, HH:MM:SS, MM:SS, and S, including optional days. Outputs consistent numeric seconds.

• Bug Fixes

  • Improved robustness: trims whitespace, normalizes non-string inputs, and handles empty/NA values as 0 to prevent errors during parsing.

[coderabbitai]

Walkthrough

Enhanced time_to_seconds in efficiency_report.py to robustly parse SLURM time formats (D-HH:MM:SS, HH:MM:SS, MM:SS, S), handle optional days, trim/normalize inputs, treat empty/NA as 0, and return consistent numeric types. Updated docstring and improved input preprocessing and string handling.

Changes

Cohort / File(s)	Summary
Time parsing enhancements snakemake_executor_plugin_slurm/efficiency_report.py	Extended time_to_seconds to support multiple SLURM formats, accumulate days, normalize inputs (strip/convert), handle empty/NA as 0, and ensure consistent int/float returns; updated docstring and parsing flow.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

I nibble ticks and tocks with glee,
From days to seconds—1, 2, 3!
SLURM clocks cleaned, whitespace shorn,
NA snores, to zero born.
Hop, hop—parsing’s crisp and tight,
Time made tidy, day and night. ⏱️🐇

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title succinctly identifies the specific fix to the time_to_seconds function by noting inclusion of the D-HH:MM:SS format, clearly reflecting the primary change in concise language.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

snakemake_executor_plugin_slurm/efficiency_report.py (2)

13-16: Consider optimizing duplicate string conversion.

Lines 13 and 16 both call str(time_str).strip(), which is redundant. Consider restructuring for clarity and efficiency:

-    if pd.isna(time_str) or str(time_str).strip() == "":
+    if pd.isna(time_str):
         return 0
-
-    time_str = str(time_str).strip()
+    time_str = str(time_str).strip()
+    if time_str == "":
+        return 0

---

18-21: Consider defensive handling for malformed day format.

If the input contains a hyphen but has an empty day or time component (e.g., "-", "5-", or "-12:34"), the function will raise a ValueError. While SLURM should only return valid formats, adding validation would make the function more robust.

 days = 0
 if "-" in time_str:  # Format D-HH:MM:SS
     d, time_str = time_str.split("-", 1)
+    if not d or not time_str:
+        return 0
     days = int(d) * 86400

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 104d0f7 and 1f99ac6.

📒 Files selected for processing (1)

• snakemake_executor_plugin_slurm/efficiency_report.py (1 hunks)

🔇 Additional comments (2)

snakemake_executor_plugin_slurm/efficiency_report.py (2)

24-34: LGTM! Good enhancement for handling days and fractional seconds.

The logic correctly:

• Adds the days offset in all three time format branches
• Uses float() for seconds to handle fractional values (e.g., from SLURM's microsecond precision)
• Maintains the existing fallback to 0 for unrecognized formats

---

11-34: Add comprehensive tests for SLURM time parsing
Include cases for:

• D-HH:MM:SS formats (e.g. “1-12:34:56”, “0-00:05:30”)
• HH:MM:SS (“12:34:56”)
• MM:SS (“34:56”)
• S (“56”)
• Fractional seconds (“2-00:00:30.5”)
• Edge cases: empty/None, zero values, large day counts

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Clarify or remove the temporary import.

The import os is marked as "only temporarily needed for print debugging" but is actively used on line 228. Either remove the import and the os.listdir() call if they're truly temporary, or remove the misleading comment.

🤖 Prompt for AI Agents

In snakemake_executor_plugin_slurm/efficiency_report.py around line 7 (and usage
at line 228), the import os is marked as "only temporarily needed for print
debugging" but is actually used via os.listdir() on line 228; either remove the
import and the os.listdir() call (and any debug prints relying on it) if they
were temporary, or remove/adjust the misleading comment and keep the import and
call if they are required for functionality; update or remove related debug
prints and ensure imports and code reflect the chosen approach so linter/tests
don't complain.

[cmeesters]

@Cheshakoneko thank you for this PR immediately fixing, what I - apparently - overlooked!

[cmeesters]

I really like your PR - it just turned out, that test cases were missing, and we also need to add checks for fractional seconds (which I think we can totally discard, but the HPC world is crazy about precision, so ...)

Well, I cloned your PR, but I cannot push to your fork, so #362 might replace your PR - but you are still a contributor. And a jolly good one, I might say!