Speed up `PandasDataset` for long dataframes #2435

lostella · 2022-11-11T12:29:52Z

Issue #, if available: #2363

Description of changes: This PR is similar in spirit to #2377, but without adding a dedicated class. Also strengthened tests a bit.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Please tag this pr with at least one of these labels to make our release process faster: BREAKING, new feature, bug fix, other change, dev setup

lostella · 2022-11-11T12:38:33Z

I'm using the following example to evaluate speed (iterating twice since index checks are disabled after the first iteration)

from pathlib import Path
from time import time
import pandas as pd
from tqdm import tqdm

from gluonts.dataset.pandas import PandasDataset

df = pd.read_parquet(Path(__file__).resolve().parent / "long_df_sample.parquet")

t0 = time()
ds = PandasDataset.from_long_dataframe(
    dataframe=df,
    item_id="item_id",
    timestamp="timestamp",
    freq="M",
)
t1 = time()
print(f"construction time: {t1 - t0}")

N = 2

t0 = time()
for _ in range(N):
    for entry in tqdm(ds):
        pass
t1 = time()
print(f"average iteration time: {(t1 - t0)/N}")

Before this PR:

construction time: 32.1102340221405
100%|███████████████████████████████████████████████████████████████████████████| 25000/25000 [00:04<00:00, 6237.54it/s]
100%|██████████████████████████████████████████████████████████████████████████| 25000/25000 [00:01<00:00, 17872.74it/s]

After this PR:

construction time: 0.44616174697875977
100%|███████████████████████████████████████████████████████████████████████████| 25000/25000 [00:11<00:00, 2164.93it/s]
100%|███████████████████████████████████████████████████████████████████████████| 25000/25000 [00:08<00:00, 2803.81it/s]

So construction is much faster (not as fast as in #2377, since there the groupby operation is delayed), but iteration is slower. This is because the original code essentially pre-processes and caches all dataframes at construction time: with this PR, one can get the same by simply caching the dataset explicitly using list, in case dataset iteration is a bottleneck.

lostella · 2022-11-11T12:42:00Z

If I cache the dataset at construction time with ds = list(ds), with the PR code I get

construction + caching time: 11.770990133285522
100%|████████████████████████████████████████████████████████████████████████| 25000/25000 [00:00<00:00, 2887525.47it/s]
100%|████████████████████████████████████████████████████████████████████████| 25000/25000 [00:00<00:00, 2800833.38it/s]

which is anyway much faster than the original code.

jaheba · 2022-11-11T12:59:28Z

src/gluonts/dataset/pandas.py

+    if isinstance(data, dict):
+        return data.items()
+    if isinstance(data, (Iterable, Collection)):
+        first_element = first(data)


You might want to use toolz.peek here.

isn't it pretty much the same for collections or iterables?

I guess we rely on the re-iterable aspect?

jaheba · 2022-11-11T13:47:25Z

src/gluonts/dataset/pandas.py

-            self._dataframes = [(None, df) for df in self.dataframes]
-        else:  # case single dataframe
-            self._dataframes = [(None, self.dataframes)]
+            self._pairs: Any = self.dataframes.items()


Anything that is assigned to should be listed as a definition.

not sure I get it

There should be a type annotation on the class level

jaheba · 2022-11-11T13:49:41Z

src/gluonts/dataset/pandas.py

-            series.index, DatetimeIndexOpsMixin
-        ), "series index has to be a DatetimeIndex."
-        return series.to_frame(name="target")
+def pair_with_item_id(obj):


Type annotation is missing.

I think this should use a NamedTuple. I think it's generally bad style to rely on some implied meaning through ordering.

jaheba · 2022-11-11T15:02:15Z

src/gluonts/dataset/pandas.py

-            if self.timestamp:
-                df = df.set_index(keys=self.timestamp)
+        if isinstance(self.dataframes, (pd.Series, pd.DataFrame)):
+            self._dataframes = [self.dataframes]


Why have one with and one without underscore?

Long story short: some test (test_hts_to_dataset) is accessing the dataframes attribute and was breaking with my change, and I'm trying to avoid touching tests at all (in the sense of modifying existing assertions). But I can also do that.

jaheba · 2022-11-11T15:55:49Z

src/gluonts/dataset/pandas.py

    """

    dataframes: Union[
        pd.DataFrame,
        pd.Series,
-        List[pd.DataFrame],


I think we can make use of TypeVar here: https://stackoverflow.com/questions/59933946/difference-between-typevart-a-b-and-typevart-bound-uniona-b

jaheba · 2022-11-16T10:04:38Z

Do we know what impact on memory consumption this change has?

lostella · 2022-11-16T12:26:45Z

Do we know what impact on memory consumption this change has?

I used scalene with Python 3.8.13, on dev vs this PR, using the data from #2363. Max memory consumption is as follows:

dev: 575.1 MB
this PR: 315.0 MB

* Fix rotbaum random seed and num_samples argument. (#2408) * Hierarchical: Make sure the input S matrix is of right dtype (#2415) * Speed up `PandasDataset` for long dataframes (#2435) * Fix frequency inference in `PandasDataset` (#2442) * Mypy fixes (#2427) * Fix new mypy complaints. * Also remove ininspection comments. * Tests: Change Python versions. (#2448) Remove Python 3.6, instead test up to 3.10. Co-authored-by: Sigrid Passano Hellan <[email protected]> Co-authored-by: Syama Sundar Rangapuram <[email protected]> Co-authored-by: Jasper <[email protected]>

lostella requested a review from jaheba November 11, 2022 12:29

jaheba reviewed Nov 11, 2022

View reviewed changes

lostella added bug fix (one of pr required labels) pending v0.11.x backport This contains a fix to be backported to the v0.11.x branch labels Nov 12, 2022

lostella added 15 commits November 15, 2022 09:56

speed up long dataframe case

df6f922

fixup

fd677f7

strengthen test

1731f36

clean up

ff8f711

fix style

a621789

fix import

7d23ba0

simplify

821d355

remove import

b17679b

fix type issues

3d61ad4

fix because of failing test

bdb8eff

simplify

787edcd

address comments

3ba6f9f

simplify

1a0759a

fixup

f3805cb

fix type annotations

247ee13

lostella force-pushed the fix-2363 branch from caf8653 to 247ee13 Compare November 15, 2022 11:20

fix bad if

e97acf6

jaheba approved these changes Nov 16, 2022

View reviewed changes

lostella merged commit d6aec0a into awslabs:dev Nov 16, 2022

lostella deleted the fix-2363 branch November 16, 2022 12:27

This was referenced Nov 16, 2022

Speed up PandasDataset further #2441

Merged

Fix frequency inference in PandasDataset #2442

Merged

lostella added a commit to lostella/gluonts that referenced this pull request Nov 21, 2022

Speed up PandasDataset for long dataframes (awslabs#2435)

b91f4da

lostella mentioned this pull request Nov 21, 2022

Backports for v0.11.2 #2449

Merged

lostella mentioned this pull request Nov 21, 2022

Best way to generate large Datasets #2363

Closed

lostella removed the pending v0.11.x backport This contains a fix to be backported to the v0.11.x branch label Nov 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Speed up `PandasDataset` for long dataframes #2435

Speed up `PandasDataset` for long dataframes #2435

lostella commented Nov 11, 2022 •

edited

Loading

lostella commented Nov 11, 2022 •

edited

Loading

lostella commented Nov 11, 2022 •

edited

Loading

jaheba Nov 11, 2022

lostella Nov 11, 2022

jaheba Nov 11, 2022

lostella Nov 11, 2022

jaheba Nov 11, 2022

lostella Nov 11, 2022

jaheba Nov 11, 2022

jaheba Nov 11, 2022

jaheba Nov 11, 2022

lostella Nov 11, 2022 •

edited

Loading

jaheba Nov 11, 2022

jaheba commented Nov 16, 2022

lostella commented Nov 16, 2022

Speed up PandasDataset for long dataframes #2435

Speed up PandasDataset for long dataframes #2435

Conversation

lostella commented Nov 11, 2022 • edited Loading

lostella commented Nov 11, 2022 • edited Loading

lostella commented Nov 11, 2022 • 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

lostella Nov 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaheba commented Nov 16, 2022

lostella commented Nov 16, 2022

Speed up `PandasDataset` for long dataframes #2435

Speed up `PandasDataset` for long dataframes #2435

lostella commented Nov 11, 2022 •

edited

Loading

lostella commented Nov 11, 2022 •

edited

Loading

lostella commented Nov 11, 2022 •

edited

Loading

lostella Nov 11, 2022 •

edited

Loading