-
Notifications
You must be signed in to change notification settings - Fork 3.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds more robust timer duration parsing #19513
Conversation
Failure to provide the timer duration string (e.g., via --max_time) in the sensible format currently results in an uninformative IndexError. This uses a simple regex 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 not designed for absolute times, not deltas, and therefore does not support the idea of "0 days". Closes Lightning-AI#19454.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kylebgorman This is already great!
Would you like to try adding a unit test for this?
It would go into the file tests/tests_pytorch/callbacks/test_timer.py
. You can use
with pytest.raises(Misconfiguration, match="Expected DD:HH:MM:SS format"):
Timer(...)
to test your code.
Otherwise I'm happy to help or do it.
Co-authored-by: awaelchli <[email protected]>
Co-authored-by: awaelchli <[email protected]>
I greatly appreciate the suggestions, particularly about wording of the exception. I'll add a unit test shortly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's great, thanks a lot for adding the test!
Feel free to also add an entry in the "Added" section of the changelog in src/lightning/pytorch/CHANGELOG.md
Done. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #19513 +/- ##
==========================================
- Coverage 84% 48% -35%
==========================================
Files 450 442 -8
Lines 38264 38113 -151
==========================================
- Hits 31996 18444 -13552
- Misses 6268 19669 +13401 |
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-inre
) 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.
Before submitting
PR review
Anyone in the community is welcome to review the PR.
Before you start reviewing, make sure you have read the review guidelines. In short, see the following bullet-list:
Reviewer checklist
📚 Documentation preview 📚: https://pytorch-lightning--19513.org.readthedocs.build/en/19513/