[1/n subset refactor] Split TimeWindowPartitionsSubset #17684

clairelin135 · 2023-11-03T17:21:08Z

TimeWindowPartitionsSubset has lots of performance improvement code that relies on representing partitions as keys, but relies on converting partition keys to time windows in order to serialize. In preparation for serializing the time window version, this PR splits the implementation:

BaseTimePartitionsSubset: a time partitions subset superclass that defines abstract methods, and contains helper functions
TimePartitionKeyPartitionsSubset: the "partition key" representation
TimeWindowPartitionsSubset: the "time window" representation

By default, when creating partitions subsets via partitions_def.empty_subset().with_partition_keys(...), a TimePartitionKeyPartitionsSubset will be created. This class contains a method to convert to a TimeWindowPartitionsSubset when it needs to be serialized.

In future PRs, time partitions subsets can be directly deserialized as TimeWindowPartitionsSubsets.

clairelin135 · 2023-11-03T18:47:40Z

Current dependencies on/for this PR:

master
- PR [1/n subset refactor] Split TimeWindowPartitionsSubset #17684 👈
  - PR [2/n subset refactor] Make TimeWindowPartitionsDefinition serializable #17660
    - PR [3/n subset refactor] Add whitelist_for_serdes to TimeWindowPartitionsSubset #17702
      - PR [4/n subset refactor] Add whitelist_for_serdes to DefaultPartitionsSubset #17703
        
        PR [5/n subset refactor] [serdes] Enable serializing mappings with non-scalar keys #18057
        
        PR [6/n subset refactor] Serialize AssetGraphSubset and AssetBackfillData with whitelist_for_serdes #17844
        
        PR [7/n subset refactor] Use new asset backfill data serialization format #17929

This stack of pull requests is managed by Graphite.

sryza · 2023-11-06T16:21:26Z

python_modules/dagster/dagster/_core/definitions/time_window_partitions.py

+        )
+
+    @property
+    @cached_method


Because it takes no arguments, you can use @cached_property from functools here

sryza · 2023-11-06T16:25:56Z

python_modules/dagster/dagster/_core/definitions/time_window_partitions.py

+        Args:
+            dt_cron_schedule (str): A cron schedule that dt is on one of the ticks of.
+        """
+        if self._included_time_windows is not None:


It would never be not None, right?

sryza · 2023-11-06T16:27:41Z

python_modules/dagster/dagster/_core/definitions/time_window_partitions.py

+    def __init__(
+        self,
+        partitions_def: TimeWindowPartitionsDefinition,
+        num_partitions: Optional[int] = None,


I'd lean towards not providing default values for these arguments and instead requiring the caller to to think about them.

sounds good

sryza · 2023-11-06T16:28:45Z

python_modules/dagster/dagster/_core/definitions/time_window_partitions.py

+        self._num_partitions = (
+            num_partitions
+            if num_partitions
+            else self._num_partitions_from_time_windows(partitions_def, included_time_windows)


This can be an expensive operation. If it wasn't previously part of the constructor, I'd be nervous that adding it could cause perf issues.

got it -- I amended this to instead just be optional, and to only calculate on demand

python_modules/dagster/dagster/_core/definitions/time_window_partitions.py

sryza · 2023-11-06T16:29:54Z

python_modules/dagster/dagster/_core/definitions/time_window_partitions.py

+            included_time_windows, "included_time_windows", of_type=TimeWindow
+        )
+
+    def get_included_time_windows(self) -> Sequence[TimeWindow]:


Why not just call this included_time_windows?

ah yeah we don't need this (probably accidentally added this)

sryza · 2023-11-06T16:30:50Z

python_modules/dagster/dagster/_core/definitions/time_window_partitions.py

            included_partition_keys=self._included_partition_keys,
        )

+    def resolve(self) -> "TimeWindowPartitionsSubset":


Is this needed? I don't see it called anywhere.

it's not used in this PR, but I was planning on using it in later parts of this stack to enable serializing a TimePartitionKeyPartitionsSubset

I think it would be clearer to add it in the later part that uses it

sounds good

sryza

This looks solid. Last thing: I'm wondering if there's a way to name these classes that makes their inheritance relationship clear. E.g. if I saw BaseTimeWindowPartitionsSubset and TimePartitionKeyPartitionsSubset in the wild, it wouldn't be obvious that the latter is a superclass of the former.

A convention-al way of naming these would be something like:

TimeWindowPartitionsSubset - abstract base class for subsets of a TimeWindowPartitionsDefinition
PartitionKeysTimeWindowPartitionsSubset - a TimeWindowPartitionsSubset that's represented by a set of partition keys.
TimeWindowsTimeWindowPartitionsSubset - a TimeWindowPartitionsSubset that's represented by a set of time windows.

TimeWindowsTimeWindowPartitionsSubset is a bit of a gnarly name of course, so definitely not a slam dunk.

sryza · 2023-11-07T23:41:17Z

python_modules/dagster/dagster/_core/definitions/time_window_partitions.py

            included_partition_keys=self._included_partition_keys,
        )

+
+class TimeWindowPartitionsSubset(BaseTimeWindowPartitionsSubset):


This could benefit from class-level docstring. Maybe "A PartitionsSubset for a TimeWindowPartitionsDefinition, which internally represents the included partitions using TimeWindow".

clairelin135 · 2023-11-08T20:17:09Z

What do you think of this:

BaseTimeWindowPartitionsSubset -> TimePartitionsSubset
TimePartitionKeyPartitionsSubset stays as is
TimeWindowPartitionsSubset stays as is

sryza · 2023-11-08T21:02:39Z

@clairelin135 how would you describe each of those?

clairelin135 · 2023-11-08T21:59:27Z

@sryza

TimePartitionsSubset: Base class for time partitions subsets that contains shared logic (i.e. building time windows from partition keys, checking equality/existence, etc.)
TimePartitionKeyPartitionsSubset: TimePartitionsSubset where included partitions are internally represented as strings. Primarily used in in-memory contexts to more performantly add/check partitions.
TimeWindowPartitionsSubset: TimePartitionsSubset where included partitions are internally represented as time windows. Primarily used for serialization to compress targeted windows.

sryza · 2023-11-08T23:31:56Z

We describe a TimeWindowPartitionsDefinition as "A set of partitions where each partitions corresponds to a time window." I'm concerned that TimePartitionsSubset might imply that there's a difference between a "time partition" and a "time window partition", especially if there's also a class called TimeWindowPartitionsSubset.

The reason I'm belaboring this is that some of these are going to end up in storage, so we won't be able to easily change them.

clairelin135 · 2023-11-09T01:04:10Z

@sryza that makes sense.

I'm hesitant about the naming of TimeWindowsTimeWindowPartitionsSubset because it feels redundant.

What if we kept BaseTimeWindowPartitionsSubset as is? In think "base" being in the name implies that it's a superclass.

Then we could rename TimePartitionKeyPartitionsSubset to PartitionKeysTimeWindowPartitionsSubset in order to clarify that it's a TimeWindowPartitionsSubset. I'm indifferent between PartitionKeysTimeWindowPartitionsSubset and TimeWindowPartitionKeysPartitionsSubset

sryza · 2023-11-09T15:48:41Z

@clairelin135 great, I like it. I vote for PartitionKeysTimeWindowPartitionsSubset, because it roughly (modulo "Base") follows the convention that a subclass of X that's specific in way Y should be named YX.

sryza

Last comment is that these classes we discussed could benefit from class-level docstring. Otherwise, this looks good to go.

clairelin135 changed the base branch from master to claire/new-subset-serialization November 3, 2023 17:21

clairelin135 force-pushed the 11-02-split_time_window_partitions_subset_implementation branch 2 times, most recently from 5bc3c4d to c9f78fc Compare November 3, 2023 18:47

clairelin135 changed the base branch from claire/new-subset-serialization to master November 3, 2023 18:47

clairelin135 force-pushed the 11-02-split_time_window_partitions_subset_implementation branch 4 times, most recently from a3c067a to cc35ffe Compare November 3, 2023 21:18

clairelin135 mentioned this pull request Nov 3, 2023

[2/n subset refactor] Make TimeWindowPartitionsDefinition serializable #17660

Merged

clairelin135 force-pushed the 11-02-split_time_window_partitions_subset_implementation branch from cc35ffe to 27d3b83 Compare November 3, 2023 21:52

clairelin135 changed the title ~~[wip] Split TimeWindowPartitionsSubset into partition key vs time window form~~ [1/n subset refactor] Split TimeWindowPartitionsSubset Nov 3, 2023

clairelin135 marked this pull request as ready for review November 3, 2023 23:24

clairelin135 requested review from sryza and OwenKephart November 3, 2023 23:25

This was referenced Nov 3, 2023

[3/n subset refactor] Add whitelist_for_serdes to TimeWindowPartitionsSubset #17702

Merged

[4/n subset refactor] Add whitelist_for_serdes to DefaultPartitionsSubset #17703

Merged

sryza reviewed Nov 6, 2023

View reviewed changes

clairelin135 force-pushed the 11-02-split_time_window_partitions_subset_implementation branch 2 times, most recently from d024b99 to e9dc40f Compare November 7, 2023 22:45

sryza reviewed Nov 7, 2023

View reviewed changes

clairelin135 mentioned this pull request Nov 8, 2023

[6/n subset refactor] Serialize AssetGraphSubset and AssetBackfillData with whitelist_for_serdes #17844

Merged

sryza reviewed Nov 9, 2023

View reviewed changes

clairelin135 force-pushed the 11-02-split_time_window_partitions_subset_implementation branch from 92c25eb to af983ea Compare November 9, 2023 18:16

clairelin135 added 7 commits November 9, 2023 10:25

split time window partitions subset implementation

a395b39

split time window partitions subset implementation

5a02929

first stab at fixing tests

514cdeb

restructure

11ce126

rename to time partition key partitions subset

1d5ff09

responding to feedback

09e676a

rename to PartitionKeysTimeWindowPartitionsSubset

ecc6f42

sryza approved these changes Nov 9, 2023

View reviewed changes

clairelin135 force-pushed the 11-02-split_time_window_partitions_subset_implementation branch from af983ea to ecc6f42 Compare November 10, 2023 00:23

clairelin135 merged commit 525c3cc into master Nov 10, 2023

clairelin135 deleted the 11-02-split_time_window_partitions_subset_implementation branch November 10, 2023 20:27

This was referenced Nov 11, 2023

[7/n subset refactor] Use new asset backfill data serialization format #17929

Merged

[5/n subset refactor] [serdes] Enable serializing mappings with non-scalar keys #18057

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1/n subset refactor] Split TimeWindowPartitionsSubset #17684

[1/n subset refactor] Split TimeWindowPartitionsSubset #17684

clairelin135 commented Nov 3, 2023 •

edited

Loading

clairelin135 commented Nov 3, 2023 •

edited

Loading

sryza Nov 6, 2023

sryza Nov 6, 2023

sryza Nov 6, 2023

clairelin135 Nov 7, 2023

sryza Nov 6, 2023

clairelin135 Nov 7, 2023

sryza Nov 6, 2023

clairelin135 Nov 7, 2023

sryza Nov 6, 2023

clairelin135 Nov 7, 2023 •

edited

Loading

sryza Nov 7, 2023

clairelin135 Nov 7, 2023

sryza left a comment

sryza Nov 7, 2023

clairelin135 commented Nov 8, 2023 •

edited

Loading

sryza commented Nov 8, 2023

clairelin135 commented Nov 8, 2023

sryza commented Nov 8, 2023 •

edited

Loading

clairelin135 commented Nov 9, 2023

sryza commented Nov 9, 2023

sryza left a comment

[1/n subset refactor] Split TimeWindowPartitionsSubset #17684

[1/n subset refactor] Split TimeWindowPartitionsSubset #17684

Conversation

clairelin135 commented Nov 3, 2023 • edited Loading

clairelin135 commented Nov 3, 2023 • 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

clairelin135 Nov 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sryza left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clairelin135 commented Nov 8, 2023 • edited Loading

sryza commented Nov 8, 2023

clairelin135 commented Nov 8, 2023

sryza commented Nov 8, 2023 • edited Loading

clairelin135 commented Nov 9, 2023

sryza commented Nov 9, 2023

sryza left a comment

Choose a reason for hiding this comment

clairelin135 commented Nov 3, 2023 •

edited

Loading

clairelin135 commented Nov 3, 2023 •

edited

Loading

clairelin135 Nov 7, 2023 •

edited

Loading

clairelin135 commented Nov 8, 2023 •

edited

Loading

sryza commented Nov 8, 2023 •

edited

Loading