Skip to content
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

Avoid parse errors of datetime.isoformat strings #4025

Merged

Conversation

toshihikoyanase
Copy link
Member

@toshihikoyanase toshihikoyanase commented Sep 29, 2022

Motivation

@nzw0301 found that CI failed due to the error of JournalStorage. They said the dumped strings of datetime_start (or datetime_complete) could lack microseconds but datetime_from_isoformat function always expects them.

import datetime
datetime.datetime(2022, 9, 29, 2, 27, 32, 0).isoformat()
# '2022-09-29T02:27:32'

def datetime_from_isoformat(datetime_str: str) -> datetime.datetime:
# TODO(wattlebirdaz): Use datetime.fromisoformat after dropped Python 3.6 support.
return datetime.datetime.strptime(datetime_str, "%Y-%m-%dT%H:%M:%S.%f")

@hvy suggested workarounds in https://bugs.python.org/issue40076, and this PR employs the timespec argument of datetime.isoformat to always show microseconds.

Description of the changes

  • Specify timespec="microseconds" to datetime.isoformat()
  • Add test case to handle template trials with datetime_start/datetime_complete whose microseconds values are zero.

@toshihikoyanase toshihikoyanase added the bug Issue/PR about behavior that is broken. Not for typos/examples/CI/test but for Optuna itself. label Sep 29, 2022
@github-actions github-actions bot added the optuna.storages Related to the `optuna.storages` submodule. This is automatically labeled by github-actions. label Sep 29, 2022
@toshihikoyanase
Copy link
Member Author

@wattlebirdaz @c-bata Could you review this PR as authors of JournalStorage, please?

@codecov-commenter
Copy link

Codecov Report

Merging #4025 (cee4443) into master (31da4d0) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #4025   +/-   ##
=======================================
  Coverage   90.19%   90.19%           
=======================================
  Files         160      160           
  Lines       12580    12580           
=======================================
  Hits        11347    11347           
  Misses       1233     1233           
Impacted Files Coverage Δ
optuna/storages/_journal/storage.py 96.50% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@rotaki
Copy link
Collaborator

rotaki commented Sep 29, 2022

Thank you for finding out the bug and fixing it. LGTM.

Copy link
Member

@c-bata c-bata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your fix! LGTM.

@c-bata c-bata added this to the v3.1.0 milestone Sep 29, 2022
@c-bata c-bata merged commit e8abeec into optuna:master Sep 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue/PR about behavior that is broken. Not for typos/examples/CI/test but for Optuna itself. optuna.storages Related to the `optuna.storages` submodule. This is automatically labeled by github-actions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants