Page MenuHomeElementl

grokkability improvements for partitions
ClosedPublic

Authored by sandyryza on Jun 30 2021, 3:25 AM.

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
433

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
433

codemod error. will fix