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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

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
447

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
447

codemod error. will fix

This revision was automatically updated to reflect the committed changes.