Repository: Lightning-AI/pytorch-lightning
Pull request: Adds more robust timer duration parsing
## What does this PR do?

Failure to provide the timer duration string (e.g., via `--max_time`) in the appropriate format currently results in an uninformative IndexError. This uses a simple regex (and the built-in `re`) for more robust input validation and instead throws a misconfiguration exception.

I experimented with `datetime.strptime` for this, but it is not well-suited to the job because it is designed for absolute times, not deltas, and therefore does not support the idea of "0 days".

Fixes #19454.

I am submitting as is, but reviewers, feel free to recommend testing. 

<details>
  <summary><b>Before submitting</b></summary>

- Was this **discussed/agreed** via a GitHub issue? (not for typos and docs)
- [x] Did you read the [contributor guideline](https://github.com/Lightning-AI/lightning/blob/master/.github/CONTRIBUTING.md), **Pull Request** section?
- [x] Did you make sure your **PR does only one thing**, instead of bundling different changes together?
- Did you make sure to **update the documentation** with your changes? (if necessary)
- Did you write any **new necessary tests**? (not for typos and docs)
- [ ] Did you verify new and **existing tests pass** locally with your changes?
- Did you list all the **breaking changes** introduced by this pull request?
- Did you **update the CHANGELOG**? (not for typos, docs, test updates, or minor internal changes/refactors)

</details>

## PR review

Anyone in the community is welcome to review the PR.
Before you start reviewing, make sure you have read the [review guidelines](https://github.com/Lightning-AI/lightning/wiki/Review-guidelines). In short, see the following bullet-list:

<details>
  <summary>Reviewer checklist</summary>

- [ ] Is this pull request ready for review? (if not, please submit in draft mode)
- [ ] Check that all items from **Before submitting** are resolved
- [ ] Make sure the title is self-explanatory and the description concisely explains the PR
- [ ] Add labels and milestones (and optionally projects) to the PR so it can be classified

</details>

<!-- readthedocs-preview pytorch-lightning start -->
----
📚 Documentation preview 📚: https://pytorch-lightning--19513.org.readthedocs.build/en/19513/

<!-- readthedocs-preview pytorch-lightning end -->

Problem statement:
Input validation for Trainer's `min_time` and `max_time` when passed as string
### Description & Motivation

The argument to the `Trainer`s min_time is expected to be a string of the form /(\d\d):(\d\d):(\d\d):(\d\d)/. One way in which parsing can fail (simple example: `--min_time 60`) throws an `IndexError` and provides little context for what is wrong.

### Pitch

This is an uninformative error message, and managed to briefly stump me and a colleague. It should be caught and converted into an informative one that explains the expected format.

This should probably be done at the API level rather than becoming something specific to CLI parsing. That is, since one can provide a string to this API, it should catch this error and instead raise an error with a name like `TimeParsingError` or something of that nature.

### Alternatives

Use `datetime.timedelta.strptime` or other standard-library elements to do the parsing, and catch the `ValueError` and raise something more informative.

### Additional context

I am working on PyTorch-Lightning >=1.7.0,<2.0.0 because I haven't had the chance to migrate to `LightningCLI` and such, but a quick perusal suggests this is still an issue.

cc @borda @carmocca @awaelchli

Hints from issue discussion:
@kylebgorman Would you like to contribute this suggestion?
> @kylebgorman Would you like to contribute this suggestion?

Sure, it seems like a small issue. (I need to clear OSS patches with my employer, but I'll do that now.)

Commit message: precommit hooks

Patch to review:
diff --git a/src/lightning/pytorch/callbacks/timer.py b/src/lightning/pytorch/callbacks/timer.py
index 7aa84c0ab5d8c..21f503b3ae748 100644
--- a/src/lightning/pytorch/callbacks/timer.py
+++ b/src/lightning/pytorch/callbacks/timer.py
@@ -17,6 +17,7 @@
 """
 
 import logging
+import re
 import time
 from datetime import timedelta
 from typing import Any, Dict, Optional, Union
@@ -50,6 +51,8 @@ class Timer(Callback):
         verbose: Set this to ``False`` to suppress logging messages.
 
     Raises:
+        MisconfigurationException:
+            If ``duration`` is not in the expected format.
         MisconfigurationException:
             If ``interval`` is not one of the supported choices.
 
@@ -86,9 +89,17 @@ def __init__(
     ) -> None:
         super().__init__()
         if isinstance(duration, str):
-            dhms = duration.strip().split(":")
-            dhms = [int(i) for i in dhms]
-            duration = timedelta(days=dhms[0], hours=dhms[1], minutes=dhms[2], seconds=dhms[3])
+            datematch = re.fullmatch(r"(\d+):(\d\d):(\d\d):(\d\d)", duration.strip())
+            if not datematch:
+                raise MisconfigurationException(
+                    f"Parameter value {interval!r} cannot be convert into duration. Expected DD:HH:MM:SS format."
+                )
+            duration = timedelta(
+                days=int(datematch.group(1)),
+                hours=int(datematch.group(2)),
+                minutes=int(datematch.group(3)),
+                seconds=int(datematch.group(4)),
+            )
         if isinstance(duration, dict):
             duration = timedelta(**duration)
         if interval not in set(Interval):


Code review comments:
diff_hunk: @@ -86,9 +89,17 @@ def __init__(
     ) -> None:
         super().__init__()
         if isinstance(duration, str):
-            dhms = duration.strip().split(":")
-            dhms = [int(i) for i in dhms]
-            duration = timedelta(days=dhms[0], hours=dhms[1], minutes=dhms[2], seconds=dhms[3])
+            datematch = re.fullmatch(r"(\d+):(\d\d):(\d\d):(\d\d)", duration.strip())
+            if not datematch:
+                raise MisconfigurationException(
+                    f"Parameter value {interval!r} cannot be convert into duration. Expected DD:HH:MM:SS format."
path: src/lightning/pytorch/callbacks/timer.py
text: @user1:
Just a small wording suggestion to be more consistent with the rest of Lightning's messaging:
```suggestion
                    f"`Timer(duration={interval!r})` is not a valid duration. Expected a string in the format DD:HH:MM:SS."
```

diff_hunk: @@ -86,9 +89,17 @@ def __init__(
     ) -> None:
         super().__init__()
         if isinstance(duration, str):
-            dhms = duration.strip().split(":")
-            dhms = [int(i) for i in dhms]
-            duration = timedelta(days=dhms[0], hours=dhms[1], minutes=dhms[2], seconds=dhms[3])
+            datematch = re.fullmatch(r"(\d+):(\d\d):(\d\d):(\d\d)", duration.strip())
path: src/lightning/pytorch/callbacks/timer.py
text: @user1:
```suggestion
            duration_match = re.fullmatch(r"(\d+):(\d\d):(\d\d):(\d\d)", duration.strip())
```