Release timezone and calendar features in DatapointsAPI (beta) #1779

haakonvt · 2024-05-23T13:19:06Z

Builds upon #1774

[7.45.0] - 2024-05-28

Added

DatapointsAPI now support timezone and new calendar-based granularities like month, quarter and year.
These API features are in beta, and the SDK implementation in alpha, meaning breaking changes can
happen without warning. Set beta header to avoid warning. Users of retrieve_dataframe_in_tz should
consider upgrading as soon as possible.

codecov · 2024-05-23T13:25:57Z

Codecov Report

Attention: Patch coverage is 91.20879% with 24 lines in your changes are missing coverage. Please review.

Project coverage is 92.94%. Comparing base (00990e7) to head (756a4f6).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1779      +/-   ##
==========================================
- Coverage   92.95%   92.94%   -0.02%     
==========================================
  Files         121      121              
  Lines       17547    17673     +126     
==========================================
+ Hits        16311    16426     +115     
- Misses       1236     1247      +11

Files	Coverage Δ
cognite/client/_api/datapoints.py	`97.74% <100.00%> (-0.14%)`	⬇️
cognite/client/_version.py	`100.00% <100.00%> (ø)`
cognite/client/utils/__init__.py	`100.00% <ø> (+11.11%)`	⬆️
cognite/client/utils/_importing.py	`91.66% <ø> (ø)`
cognite/client/_api/datapoint_tasks.py	`95.21% <96.55%> (-0.36%)`	⬇️
cognite/client/utils/_pandas_helpers.py	`85.48% <85.00%> (-0.88%)`	⬇️
cognite/client/utils/_time.py	`94.86% <87.50%> (+0.05%)`	⬆️
cognite/client/data_classes/datapoints.py	`91.72% <88.00%> (+0.30%)`	⬆️

... and 3 files with indirect coverage changes

tests/tests_unit/test_base.py

doctrino

Main comment is that I vote for allowing string as input for input to API method.

Otherwise, there is some refactoring which seems unrelated to the new functionality (although I guess implicitly related) which makes it hard to get an overview. But the testing is good, so I trust the changes.

cognite/client/_api/datapoints.py

doctrino · 2024-05-29T06:47:44Z

cognite/client/_api/datapoints.py

                >>> df = client.time_series.data.retrieve_dataframe_in_tz(
                ...     external_id=["foo", "bar"],
                ...     aggregates=["sum", "continuous_variance"],
                ...     granularity="1quarter",
                ...     start=datetime(2020, 1, 1, tzinfo=ZoneInfo("America/New_York")),
                ...     end=datetime(2022, 12, 31, tzinfo=ZoneInfo("America/New_York")))
        """
+        warnings.warn(


Great to see that this is no longer necessary :)

Does it make sense to warn when this is still in beta? I'm not sure 🤔

doctrino · 2024-05-29T06:54:24Z

cognite/client/data_classes/datapoints.py

@@ -295,7 +316,8 @@ def to_payload_item(self) -> _DatapointsPayloadItem:
            payload["ignoreBadDataPoints"] = self.ignore_bad_datapoints
        if self.treat_uncertain_as_bad is False:
            payload["treatUncertainAsBad"] = self.treat_uncertain_as_bad
-
+        if self.timezone:
+            payload["timeZone"] = self.timezone


Is this correct? My IDE complains that self.timezone is an object, while payload["timeZone"] expects str or None

I could have made an api_friendly_timezone parameter instead of overwriting the existing one, but then the fetching logic must use that one instead of the logical one (timezone), so I vouch to keep it. Pleeeenty of tests that cover all possible inputs.

cognite/client/data_classes/datapoints.py

matiascognite

Lots of code here, but looks great to me. That said, I'm not into python, and I haven't run any of these, so my approval is partially a rubber stamp. Some nitty comments though, may act on them, or not :)

matiascognite · 2024-05-30T10:55:34Z

cognite/client/_api/datapoints.py

            aggregates (Aggregate | str | list[Aggregate | str] | None): Single aggregate or list of aggregates to retrieve. Default: None (raw datapoints returned)
-            granularity (str | None): The granularity to fetch aggregates at. e.g. '15s', '2h', '10d'. Default: None.
+            granularity (str | None): The granularity to fetch aggregates at. e.g. '15s', '2h', '10d'. The unit can be spelled out for clarity, e.g., second(s), minute(s), hour(s), day(s), week(s), month(s), quarter(s), or year(s). Default: None.


nit: you write the full name, minute(s) and month(s), and you imply that there is an abbreviation. The docs doesn't say what these are though (in particular m/mo as abbreviations for minute/month). Maybe it's easier if you add examples between s and h, and after d?

Agreed! I'll add a separate section on available granularities, then keep the docstring "short and simple"!

Changed to:

granularity (str | None): The granularity to fetch aggregates at. Can be given as an abbreviation or spelled out for clarity: s/second(s), m/minute(s), h/hour(s), d/day(s), w/week(s), mo/month(s),
q/quarter(s), or y/year(s). Examples: 30s, 5m, 1day, 2weeks. Default: None.

matiascognite · 2024-05-30T11:05:28Z

cognite/client/_api/datapoints.py

@@ -1015,34 +1045,35 @@ def retrieve_dataframe(

        if not uniform_index:
            return fetcher.fetch_all_datapoints_numpy().to_pandas(
-                column_names, include_aggregate_name, include_granularity_name
+                column_names, include_aggregate_name, include_granularity_name, include_status=True


just to be on the safe side, is it intentional to set include_status=True? Not include_status=include_status?

This is just the conversion step to pandas which only converts when status codes are available. That said, I agree we can just pass include_status=include_status instead

matiascognite · 2024-05-30T12:56:03Z

cognite/client/utils/_time.py

+    "sec": "s",
+    "second": "s",
+    "seconds": "s",
+    "t": "m",


Out of curiosity, what is the "t" granularity stand for?

In "pandas world", T has always been minutes (as m was month)

matiascognite · 2024-05-30T13:05:52Z

cognite/client/utils/_time.py

+        return timezone.utc
+    elif re.match(r"^(-|\+)\d\d?$", tz) and abs(hours_offset := int(tz)) <= 18:
+        return timezone(timedelta(hours=hours_offset))
+    return cast(timezone, datetime.strptime(tz, "%z").tzinfo)


Can't run this code in my head, so just want to check, we do support UTC+05:30 and similar? (Is that what datetime.strptime does?)

I assume this is only called when we couldn't parse time zones like "Europe/Oslo", so that we don't have to handle those.

Yup, "%z" parses any UTC offset, so I'm relying as much as possible on built-in datetime parsing

matiascognite · 2024-05-30T13:18:43Z

tests/tests_unit/test_utils/test_time.py

+        ("", timedelta(0)),
+        ("01:15", timedelta(seconds=4500)),
+        ("01:15:12", timedelta(seconds=4512)),
+        ("23:59", timedelta(seconds=86340)),


We have disallowed (400 error) fixed offsets that are not multiple of 15 minutes. Not sure if this matters to you.

We parse it and just pass it on to the API (and let it decide/raise) - this way the SDK is future-proof in case you ever decide to be more lenient with offsets 😄

…t just converted ms)

This reverts commit 67baf98.

This reverts commit 7f37fd3.

tests/tests_unit/test_utils/test_time.py

haakonvt force-pushed the swap-dps-tz-to-api branch 2 times, most recently from 8908885 to a7286f4 Compare May 24, 2024 23:47

haakonvt added the do-not-merge label May 26, 2024

haakonvt force-pushed the swap-dps-tz-to-api branch 2 times, most recently from dc1c766 to 3afe935 Compare May 28, 2024 10:42

haakonvt changed the title ~~[DONT MERGE] Release timezone and calendar features in DatapointsAPI~~ Release timezone and calendar features in DatapointsAPI (beta) May 28, 2024

haakonvt marked this pull request as ready for review May 28, 2024 10:43

haakonvt requested review from a team as code owners May 28, 2024 10:43

haakonvt requested review from mattmurph9 and removed request for a team May 28, 2024 10:43

haakonvt removed the do-not-merge label May 28, 2024

haakonvt commented May 28, 2024

View reviewed changes

tests/tests_unit/test_base.py Outdated Show resolved Hide resolved

doctrino reviewed May 29, 2024

View reviewed changes

doctrino self-requested a review May 29, 2024 09:59

doctrino approved these changes May 29, 2024

View reviewed changes

haakonvt force-pushed the swap-dps-tz-to-api branch from a337bd9 to a5498ff Compare May 30, 2024 07:25

matiascognite reviewed May 30, 2024

View reviewed changes

haakonvt added 11 commits May 30, 2024 22:07

add warning to ret..in_tz

9f3ed12

cleanup of zoneinfo: type annots. and importing

fa358c4

change datetime imports to avoid timezone ambiguity

55fc0c8

force serial fetching for tz + calendar queries

a942ea9

add cursor support for tz+calender queries

c5a7e33

fix docstring test that failed

b2f613d

several minor bugfixes. in chunking mode, use first cursor

bf05a64

add timezone for Datapoint

251c33f

add tests for timezone for Datapoint

9da1c3a

do not allow str timezone (we cant display it)

a95c19f

make lint CI build job have all deps for mypy inferrence

df0fa79

haakonvt added 16 commits May 30, 2024 22:07

never mutate user dps query

f339c26

fix timezone not being passed on in retrieve_dataframe

3f3eb13

add integ.test test_timezone_raw_query_dst_transitions

82d3b56

add integ. test test_timezone_agg_query_dst_transitions

3eccb20

add test for calendar granularities

0d03514

improve error message on _verify_time_range to include user given (no…

41b6466

…t just converted ms)

handle beta header for user with warning

00b333e

changelog, version

996ce6d

make backports-zoneinfo required on 3.8

07ad634

update zoneinfo imports after required change

04bf8e4

support string timezone

ecbc535

reorder tests to their appropriate classes

3eb4d61

documentation changes after adding str + tz/month example code

89d7d5b

fix DatapointsArray.load for timezone

b6de2d7

add test for parse_str_timezone

58f65a9

move retrieve_dataframe_in_tz "to use API"

7f37fd3

haakonvt force-pushed the swap-dps-tz-to-api branch from a5498ff to 7f37fd3 Compare May 31, 2024 07:29

haakonvt added 8 commits May 31, 2024 09:35

update test using DayAligner

67baf98

doc improvements re pr feedback

d4f6184

change dt.foo to datetime.foo

a292704

Revert "update test using DayAligner"

0aef557

This reverts commit 67baf98.

Revert "move retrieve_dataframe_in_tz "to use API""

e5f04f6

This reverts commit 7f37fd3.

more dps doc improvements

b5389c4

dont warn when still beta

41fa87a

deactivate groups create tests due to max API limit

44f54e8

haakonvt commented May 31, 2024

View reviewed changes

tests/tests_unit/test_utils/test_time.py Outdated Show resolved Hide resolved

last round of fixes (DatapointsList.__str__)

756a4f6

haakonvt enabled auto-merge (squash) May 31, 2024 13:14

haakonvt merged commit bb76e95 into master May 31, 2024
12 checks passed

haakonvt deleted the swap-dps-tz-to-api branch May 31, 2024 13:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Release timezone and calendar features in DatapointsAPI (beta) #1779

Release timezone and calendar features in DatapointsAPI (beta) #1779

haakonvt commented May 23, 2024 •

edited

Loading

codecov bot commented May 23, 2024 •

edited

Loading

doctrino left a comment

doctrino May 29, 2024

haakonvt May 29, 2024

doctrino May 29, 2024

haakonvt May 29, 2024 •

edited

Loading

matiascognite left a comment

matiascognite May 30, 2024

haakonvt May 30, 2024

haakonvt May 31, 2024 •

edited

Loading

matiascognite May 30, 2024

haakonvt May 31, 2024

matiascognite May 30, 2024

haakonvt May 30, 2024

matiascognite May 30, 2024

matiascognite May 30, 2024

haakonvt May 30, 2024

matiascognite May 30, 2024

haakonvt May 30, 2024

Release timezone and calendar features in DatapointsAPI (beta) #1779

Release timezone and calendar features in DatapointsAPI (beta) #1779

Conversation

haakonvt commented May 23, 2024 • edited Loading

[7.45.0] - 2024-05-28

Added

codecov bot commented May 23, 2024 • edited Loading

Codecov Report

doctrino 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

haakonvt May 29, 2024 • edited Loading

Choose a reason for hiding this comment

matiascognite left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haakonvt May 31, 2024 • edited Loading

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

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

haakonvt commented May 23, 2024 •

edited

Loading

codecov bot commented May 23, 2024 •

edited

Loading

haakonvt May 29, 2024 •

edited

Loading

haakonvt May 31, 2024 •

edited

Loading