👽 Implement `tzinfo.fromutc` for `TzInfo` #864

lig · 2023-08-07T12:06:55Z

Change Summary

Implement tzinfo.fromutc method in TzInfo as default behavior (see: https://docs.python.org/3/library/datetime.html#datetime.tzinfo.fromutc) isn't compatible with TzInfo implementation.

Related issue number

See pydantic/pydantic#6735

Checklist

Unit tests for the changes exist
Documentation reflects the changes where applicable
Pydantic tests pass with this pydantic-core (except for expected changes)
My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Selected Reviewer: @adriangb

codecov · 2023-08-07T12:12:53Z

Codecov Report

Merging #864 (4c8c7ba) into main (7f7d03d) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #864      +/-   ##
==========================================
+ Coverage   93.78%   93.79%   +0.01%     
==========================================
  Files         105      105              
  Lines       15331    15362      +31     
  Branches       25       25              
==========================================
+ Hits        14378    14409      +31     
  Misses        947      947              
  Partials        6        6

Files Changed	Coverage Δ
python/pydantic_core/__init__.py	`92.59% <ø> (ø)`
src/input/datetime.rs	`98.77% <100.00%> (+0.10%)`	⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f7d03d...4c8c7ba. Read the comment docs.

lig · 2023-08-07T12:17:36Z

please review

codspeed-hq · 2023-08-07T12:18:19Z

CodSpeed Performance Report

Merging #864 will not alter performance

_{Comparing lig/6735-tzinfo-type-doesnt-provide-proper-dst (4c8c7ba) with main (7f7d03d)}

Summary

✅ 138 untouched benchmarks

src/input/datetime.rs

samuelcolvin

please can we test fromutc directly, and also check if there are some nice tests for it in cpython and copy them?

I have no idea if the current implementation is correct.

samuelcolvin · 2023-08-08T13:55:47Z

src/input/datetime.rs

@@ -496,6 +496,11 @@ impl TzInfo {
        None
    }

+    fn fromutc<'p>(&self, py: Python<'p>, dt: &'p PyDateTime) -> PyResult<&'p PyDateTime> {


Suggested change

fn fromutc<'p>(&self, py: Python<'p>, dt: &'p PyDateTime) -> PyResult<&'p PyDateTime> {

fn fromutc<'py>(&self, py: Python<'py>, dt: &'py PyDateTime) -> PyResult<&'py PyDateTime> {

use use py everywhere.

lig · 2023-08-10T08:56:37Z

please can we test fromutc directly, and also check if there are some nice tests for it in cpython and copy them?

I have no idea if the current implementation is correct.

Well, the current implementation of utcoffset is a shortcut as well and doesn't take into account dst or zone info data for the specific point of time of the argument. So, the whole class is pretty naive implementation. I have strong doubts it can pass all CPython tests.

davidhewitt

Quoting the Python docs:

Most implementations of dst() will probably look like one of these two:
def dst(self, dt):
    # a fixed-offset class:  doesn't account for DST
    return timedelta(0)
...

... if our TzInfo type represents a fixed time zone, maybe we want to implement dst like that too?

davidhewitt · 2023-08-15T20:47:14Z

src/input/datetime.rs

+    fn fromutc<'p>(&self, py: Python<'p>, dt: &'p PyDateTime) -> PyResult<&'p PyDateTime> {
+        dt.call_method1("__add__", (self.utcoffset(py, py.None().as_ref(py))?,))?
+            .extract()
+    }


Being unable to resist a little golfing here:

No need to take py separately as you can get it from dt.py()

I don't think there's value in casting the return value to datetime, that's just spending CPU cycles

Suggested change

fn fromutc<'p>(&self, py: Python<'p>, dt: &'p PyDateTime) -> PyResult<&'p PyDateTime> {

dt.call_method1("__add__", (self.utcoffset(py, py.None().as_ref(py))?,))?

.extract()

}

fn fromutc<'py>(&self, dt: &'py PyDateTime) -> PyResult<&'py PyAny> {

let py = dt.py();

dt.call_method1("__add__", (self.utcoffset(py, py.None().as_ref(py))?,))

}

(self.utcoffset() could actually get similar treatment)

What is the benefit of taking py from dt? I thought the preferred way is requesting it with the function argument.

Just that it's all the same py so no need to have a separate argument for it. It's a bit like the following would be redundant:

fn f(x: i32, (_, y): (i32, i32)) { ... } f(x, (x, y)) // passed x twice, ignored in second case

Requesting it with the function argument is better than Python::with_gil if you don't already have it for sure.

lig · 2023-08-17T12:41:44Z

@davidhewitt

if our TzInfo type represents a fixed time zone, maybe we want to implement dst like that too?

Returning 0 from dst explicitly means that there is no day light saving time in effect. However, there could be but we just do not know it in our implementation. So, having it return None in such a case seems fair.

davidhewitt · 2023-08-17T12:52:57Z

However, there could be but we just do not know it in our implementation.

Is this the case? If we parse 2032-01-01T01:01:00+02:00 I would be ok with treating that as a fixed offset which we can assume to be +02:00 with dst offset of 0?

Looks like https://datatracker.ietf.org/doc/draft-ietf-sedate-datetime-extended/ is needed for RFC3339 datetimes to support zones with dst, in which case it's passed explicitly.

lig · 2023-08-21T08:28:03Z

Is this the case? If we parse 2032-01-01T01:01:00+02:00 I would be ok with treating that as a fixed offset which we can assume to be +02:00 with dst offset of 0?

We cannot assume that. For example, CEST is exactly +02:00 but it has dst offset of +01:00 now included in that +02:00. 2032-01-01T01:01:00+02:00 could be CEST with dst+1h or some +02:00 timezone that doesn't use DST and thus has dst+0h now.

davidhewitt · 2023-08-21T08:45:40Z

Returning 0 from dst explicitly means that there is no day light saving time in effect. However, there could be but we just do not know it in our implementation. So, having it return None in such a case seems fair.

Discussed offline - datetime.timezone always returns None, so we'll do the same.

lig · 2023-08-21T09:12:31Z

@samuelcolvin shall we bring some of those tests in?
https://github.com/python/cpython/blob/main/Lib/test/datetimetester.py#L256

lig · 2023-08-21T09:13:14Z

please review

samuelcolvin

otherwise LGTM.

samuelcolvin · 2023-08-22T12:23:57Z

src/input/datetime.rs

@@ -508,11 +512,18 @@ pub struct TzInfo {
 #[pymethods]
 impl TzInfo {
    #[new]
-    fn new(seconds: i32) -> Self {
-        Self { seconds }
+    fn py_new(seconds: f32) -> PyResult<Self> {


Better to add

impl TryFrom<i32> for TzInfo { type Error = PyErr; fn try_from(seconds: i32) -> PyResult<Self> { if seconds.abs() >= 86400 { Err(PyValueError::new_err(format!( "TzInfo offset must be strictly between -86400 and 86400 (24 hours) seconds, got {seconds}" ))) } else { Ok(Self { seconds }) } } }

Then use (seconds.trunc() as i32).try_into() here, then you can use let tz_info: TzInfo = offset.try_into()?; above and avoid going i32 -> f32 -> i32.

samuelcolvin · 2023-08-22T12:24:50Z

tests/test_tzinfo.py

+DSTEND = datetime(1, 10, 25, 1)
+
+
+class TestTzInfo(unittest.TestCase):


assuming this is copied from cpython somewhere, please include a link to the original in a docstring.

samuelcolvin · 2023-08-22T12:26:29Z

tests/test_tzinfo.py

+        self.EST = TzInfo(-timedelta(hours=5).total_seconds())
+        self.DT = datetime(2010, 1, 1)
+
+    def test_str(self):


you can probably use parameterize for these and thereby make any error easier to read.

Ideally these should be rewritten in pytest style, it shouldn't take very long, but I know you've already spend lots of time on this, so don't worry if you can't do it quickly.

I kinda like the idea of keeping them as unittest. @lig can you add a link / note to permalink to the standard library for future selfs?

src/input/datetime.rs

adriangb

This looks good, I like that we are trying to match str() and such as closely as possible to the standard library 👍🏻

lig mentioned this pull request Aug 7, 2023

✅ Implement tzinfo.fromutc method for TzInfo in pydantic-core pydantic/pydantic#7019

Merged

5 tasks

pydantic-hooky bot added the ready for review label Aug 7, 2023

pydantic-hooky bot assigned adriangb Aug 7, 2023

lig commented Aug 7, 2023

View reviewed changes

src/input/datetime.rs Outdated Show resolved Hide resolved

samuelcolvin requested changes Aug 8, 2023

View reviewed changes

pydantic-hooky bot added awaiting author revision and removed ready for review labels Aug 8, 2023

pydantic-hooky bot assigned lig and unassigned adriangb Aug 8, 2023

davidhewitt reviewed Aug 15, 2023

View reviewed changes

lig force-pushed the lig/6735-tzinfo-type-doesnt-provide-proper-dst branch from 19858e9 to 9aa86b3 Compare August 17, 2023 12:34

lig requested review from samuelcolvin and davidhewitt August 17, 2023 12:45

lig assigned davidhewitt and samuelcolvin and unassigned lig Aug 17, 2023

pydantic-hooky bot added ready for review and removed awaiting author revision labels Aug 21, 2023

lig requested a review from adriangb August 21, 2023 16:11

lig force-pushed the lig/6735-tzinfo-type-doesnt-provide-proper-dst branch from 94b1b36 to 605a8f9 Compare August 21, 2023 16:27

samuelcolvin requested changes Aug 22, 2023

View reviewed changes

pydantic-hooky bot added awaiting author revision and removed ready for review labels Aug 22, 2023

pydantic-hooky bot assigned lig and unassigned adriangb, davidhewitt and samuelcolvin Aug 22, 2023

adriangb approved these changes Aug 22, 2023

View reviewed changes

lig added 7 commits August 22, 2023 23:54

👽 Implement tzinfo.fromutc for TzInfo

db7e7cc

👌 Use 'py instead of 'p

6d04f1c

👌 Do not extract TzInfo.fromutc result

b61221e

👌 Use dt.py()

2120256

✅ Adapt some timezone CPython tests for TzInfo

ed6fdb5

💡 Add link to original CPython timezone tests

037ccf1

👌 Implement TryFrom<i32> for TzInfo

4c8c7ba

lig force-pushed the lig/6735-tzinfo-type-doesnt-provide-proper-dst branch from 605a8f9 to 4c8c7ba Compare August 22, 2023 22:20

lig enabled auto-merge (squash) August 22, 2023 22:21

lig requested a review from samuelcolvin August 22, 2023 22:32

lig disabled auto-merge August 22, 2023 22:32

lig added ready for review and removed awaiting author revision labels Aug 22, 2023

lig assigned samuelcolvin and unassigned lig Aug 22, 2023

samuelcolvin approved these changes Aug 23, 2023

View reviewed changes

samuelcolvin merged commit 8477825 into main Aug 23, 2023
30 checks passed

samuelcolvin deleted the lig/6735-tzinfo-type-doesnt-provide-proper-dst branch August 23, 2023 12:52

renovate bot mentioned this pull request Apr 11, 2024

chore(deps): update dependency pydantic_core to v2.18.2 aaditkamat/QMB6945-RMS-Project#166

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

👽 Implement `tzinfo.fromutc` for `TzInfo` #864

👽 Implement `tzinfo.fromutc` for `TzInfo` #864

lig commented Aug 7, 2023 •

edited by pydantic-hooky bot

codecov bot commented Aug 7, 2023 •

edited

lig commented Aug 7, 2023

codspeed-hq bot commented Aug 7, 2023 •

edited

samuelcolvin left a comment

samuelcolvin Aug 8, 2023

lig commented Aug 10, 2023

davidhewitt left a comment

davidhewitt Aug 15, 2023

lig Aug 17, 2023

davidhewitt Aug 17, 2023

davidhewitt Aug 17, 2023

lig commented Aug 17, 2023

davidhewitt commented Aug 17, 2023

lig commented Aug 21, 2023

davidhewitt commented Aug 21, 2023

lig commented Aug 21, 2023

lig commented Aug 21, 2023

samuelcolvin left a comment

samuelcolvin Aug 22, 2023

samuelcolvin Aug 22, 2023

samuelcolvin Aug 22, 2023

adriangb Aug 22, 2023

adriangb left a comment

	fn fromutc<'p>(&self, py: Python<'p>, dt: &'p PyDateTime) -> PyResult<&'p PyDateTime> {
	fn fromutc<'py>(&self, py: Python<'py>, dt: &'py PyDateTime) -> PyResult<&'py PyDateTime> {

		DSTEND = datetime(1, 10, 25, 1)


		class TestTzInfo(unittest.TestCase):

👽 Implement tzinfo.fromutc for TzInfo #864

👽 Implement tzinfo.fromutc for TzInfo #864

Conversation

lig commented Aug 7, 2023 • edited by pydantic-hooky bot

Change Summary

Related issue number

Checklist

codecov bot commented Aug 7, 2023 • edited

Codecov Report

lig commented Aug 7, 2023

codspeed-hq bot commented Aug 7, 2023 • edited

Merging #864 will not alter performance

Summary

samuelcolvin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lig commented Aug 10, 2023

davidhewitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lig commented Aug 17, 2023

davidhewitt commented Aug 17, 2023

lig commented Aug 21, 2023

davidhewitt commented Aug 21, 2023

lig commented Aug 21, 2023

lig commented Aug 21, 2023

samuelcolvin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adriangb left a comment

Choose a reason for hiding this comment

👽 Implement `tzinfo.fromutc` for `TzInfo` #864

👽 Implement `tzinfo.fromutc` for `TzInfo` #864

lig commented Aug 7, 2023 •

edited by pydantic-hooky bot

codecov bot commented Aug 7, 2023 •

edited

codspeed-hq bot commented Aug 7, 2023 •

edited