Page MenuHomeElementl

grokkability improvements for partitions
ClosedPublic

Authored by sandyryza on Jun 30 2021, 3:25 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Jul 1, 11:40 AM
Unknown Object (File)
Fri, Jul 1, 10:38 AM
Unknown Object (File)
Thu, Jun 30, 5:43 PM
Unknown Object (File)
Thu, Jun 30, 2:08 PM
Unknown Object (File)
Tue, Jun 28, 9:22 AM
Unknown Object (File)
Tue, Jun 28, 9:19 AM
Unknown Object (File)
Mon, Jun 27, 5:15 AM
Unknown Object (File)
Mon, Jun 27, 1:08 AM
Subscribers
None

Details

Summary
  • Added some mypy types, including making Partitions generic.
  • schedule_partition_range now returns a list of partitions instead of a function that returns a list of partitions.
  • PartitionParams -> Partitions. I don't 100% love Partitions as a replacement, but IMO it gets the meaning across better. PartitionParams makes me expect a dumb struct of parameters to some fancy function.
Test Plan

bk

Diff Detail

Repository
R1 dagster
Branch
partition-grok (branched from master)
Lint
Lint Passed
Unit
No Test Coverage

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Jun 30 2021, 3:50 AM
Harbormaster failed remote builds in B32882: Diff 40520!
Harbormaster returned this revision to the author for changes because remote builds failed.Jun 30 2021, 4:29 AM
Harbormaster failed remote builds in B32884: Diff 40522!
Harbormaster returned this revision to the author for changes because remote builds failed.Jun 30 2021, 4:03 PM
Harbormaster failed remote builds in B32894: Diff 40538!
Harbormaster returned this revision to the author for changes because remote builds failed.Jun 30 2021, 7:11 PM
Harbormaster failed remote builds in B32914: Diff 40567!
Harbormaster returned this revision to the author for changes because remote builds failed.Jun 30 2021, 10:26 PM
Harbormaster failed remote builds in B32947: Diff 40605!

I prefer partition_params to just partitions, but not in a die-on-the-hill fashion

python_modules/dagster/dagster/core/definitions/partition.py
129

PartitionsFactory? :)

get_partitions() on Partitions feels weird

290–301

we could probably do a better job explaining here the invariant below that one of 'partition_fn' or 'partitions' must be supplied

This revision is now accepted and ready to land.Jul 1 2021, 1:45 PM
rexledesma added inline comments.
python_modules/dagster/dagster/core/definitions/partition.py
137

would probably be nice to have a check here

@rexledesma @dgibson I added one more change, which is to rename TimeBasedPartitions to ScheduleTimeBasedPartitions - this will help disambiguate with some of the things I'm adding for the crag partitions revamp. Also added the check you requested @rexledesma. Would PartitionsDefinition be better than Partitions?

this looks good to me, one question inline

python_modules/dagster/dagster/core/definitions/partition.py
445

is this line right? I thought it was still either returning a single thing or a list of things, but the thing in both was the same

python_modules/dagster/dagster/core/definitions/partition.py
445

codemod error. will fix