Skip to content

changed localtime to gmtime to fix daylight savings issues #3805

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.

Sign up for GitHub

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

Closed
JamesUnicomb wants to merge 2 commits into from
Closed

changed localtime to gmtime to fix daylight savings issues #3805

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d8a5a5d
changed localtime to gmtime to fix daylight savings issues
Mar 16, 2022
505c63b
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Mar 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 13 additions & 13 deletions include/pybind11/chrono.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,15 +99,15 @@ class duration_caster {
PYBIND11_TYPE_CASTER(type, const_name("datetime.timedelta"));
};

inline std::tm *localtime_thread_safe(const std::time_t *time, std::tm *buf) {
inline std::tm *gmtime_thread_safe(const std::time_t *time, std::tm *buf) {
#if (defined(__STDC_LIB_EXT1__) && defined(__STDC_WANT_LIB_EXT1__)) || defined(_MSC_VER)
if (localtime_s(buf, time))

@rwgk rwgk Mar 16, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

All the Windows tests are failing. Do you already have a plan for that? Could we simply remove the #if and use the same code, with mutex, everywhere?

@Skylion007 Skylion007 Mar 17, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This function needs to be swapped out too to stop the tests from failing

return nullptr;
return buf;
#else
static std::mutex mtx;
std::lock_guard<std::mutex> lock(mtx);
std::tm *tm_ptr = std::localtime(time);
std::tm *tm_ptr = std::gmtime(time);
if (tm_ptr != nullptr) {
*buf = *tm_ptr;
}
Expand Down Expand Up @@ -166,7 +166,7 @@ class type_caster<std::chrono::time_point<std::chrono::system_clock, Duration>>
return false;
}

value = time_point_cast<Duration>(system_clock::from_time_t(std::mktime(&cal)) + msecs);
value = time_point_cast<Duration>(system_clock::from_time_t(timegm(&cal)) + msecs);

@CrustyAuklet CrustyAuklet Mar 28, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

timegm isn't available on windows, so this line needs to change to something like:

#if defined(_MSC_VER)
    time_t tt = _mkgmtime(&cal);
#else
    time_t tt = timegm(&cal);
#endif

    value = time_point_cast<Duration>(system_clock::from_time_t(tt) + msecs);

@rwgk rwgk Mar 28, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

As @Skylion007 pointed out before, we don't have to go to UTC to solve the roundtrip problem: #3805 (comment)
I think it's better to keep using the local time zone, but make proper use of the fold feature.

@CrustyAuklet CrustyAuklet Mar 28, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

with that solution would std::system_clock time points go into python as time aware (UTC) datetime objects, and python datetime objects go into C++ as UTC?

C++ system_clock is Unix Time (so UTC modulo leap seconds and such), so the time zone conversions are a little surprising.

@Skylion007 Skylion007 Apr 4, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I believe python datetime objects should be in local time in both? I am not sure we are actually attaching the time zone to the Python objects (we should but it doesn't seem straight forward using the raw C - API).

If it's too tricky, we might want to call the Python datetime function directly if we need to.

return true;
}

Expand Down Expand Up @@ -194,17 +194,17 @@ class type_caster<std::chrono::time_point<std::chrono::system_clock, Duration>>
std::time_t tt
= system_clock::to_time_t(time_point_cast<system_clock::duration>(src - us));

std::tm localtime;
std::tm *localtime_ptr = localtime_thread_safe(&tt, &localtime);
if (!localtime_ptr) {
throw cast_error("Unable to represent system_clock in local time");
std::tm gmtime;
std::tm *gmtime_ptr = gmtime_thread_safe(&tt, &gmtime);
if (!gmtime_ptr) {
throw cast_error("Unable to represent system_clock in GMT time");
}
return PyDateTime_FromDateAndTime(localtime.tm_year + 1900,
localtime.tm_mon + 1,
localtime.tm_mday,
localtime.tm_hour,
localtime.tm_min,
localtime.tm_sec,
return PyDateTime_FromDateAndTime(gmtime.tm_year + 1900,

@rwgk rwgk Mar 16, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Doesn't this change the behavior?
Before: local timezone
After: actually UTC but (incorrectly?) appearing as local timezone?

Same question from a different angle: as-is this PR fixes the roundtrip issue, i.e. fixes an internal consistency issue, but is it still externally inconsistent?

Do we have to tell PyDateTime_FromDateAndTime() somehow that the passed time is UTC?

I just looked a bit, seeing this warning 3 times under https://docs.python.org/3/library/datetime.html:

Warning Because naive datetime objects are treated by many datetime methods as local times, it is preferred to use aware datetimes to represent times in UTC. As such, the recommended way to create an object representing a specific timestamp in UTC is by calling datetime.fromtimestamp(timestamp, tz=timezone.utc).

How can handle this correctly from the C API?

@Skylion007 Skylion007 Mar 18, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@JamesUnicomb I am not sure about the UTC issue, but the datetime roundtrip issue can be easily resolved. We just need to use PyDateTime_FromDateAndTimeAndFold instead of PyDateTime_FromDateAndTime and make sure to properly set the is_fold argument.

gmtime.tm_mon + 1,
gmtime.tm_mday,
gmtime.tm_hour,
gmtime.tm_min,
gmtime.tm_sec,
us.count());
}
PYBIND11_TYPE_CASTER(type, const_name("datetime.datetime"));
Expand Down
10 changes: 10 additions & 0 deletions tests/test_chrono.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,16 @@ def test_chrono_system_clock_roundtrip_date():
assert time2.microsecond == 0


def test_chrono_system_clock_roundtrip_daylight_savings():
# naive datetime - AEDST clock will change
datetime1 = datetime.datetime(2021, 10, 3, 2, 18, 46, 677734)

# Roundtrip the time
datetime2 = m.test_chrono2(datetime1)

assert datetime2.hour == datetime1.hour


SKIP_TZ_ENV_ON_WIN = pytest.mark.skipif(
"env.WIN", reason="TZ environment variable only supported on POSIX"
)
Expand Down
Loading