Page MenuHomePhabricator

Create resource for partitions
AbandonedPublic

Authored by sashank on Dec 9 2019, 10:27 PM.

Details

Summary

This diff introduces a partition_resource, which grabs partition_set_name and partition_name off of the pipeline run tags and provides the correct Partition object as a resource.

Tradeoff of this approach is that we can't execute pipelines that use the partition resource, since they won't have the correct tags on the pipeline run. We'll need to build a UI in the config editor that let's us edit tags, and also automatically fill out config and tags by selecting a partition.

Test Plan

unit

Diff Detail

Repository
R1 dagster
Branch
partition-resource
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

sashank updated this revision to Diff 7338.Dec 9 2019, 10:27 PM
sashank created this revision.

Formatting

sashank edited the summary of this revision. (Show Details)Dec 9 2019, 10:28 PM
sashank edited the summary of this revision. (Show Details)
sashank edited the summary of this revision. (Show Details)
sashank added inline comments.Dec 9 2019, 10:32 PM
python_modules/dagster/dagster/core/partition/__init__.py
13

This works when executing a pipeline through dagit, but get_handle returns (None, None) when running the pipeline using execute_pipeline.

13

So, will need to figure out another way to thread the execution_target_handle through to here to get the partition_handle

sashank added a reviewer: max.Dec 9 2019, 10:33 PM
sashank updated this revision to Diff 7340.Dec 9 2019, 10:35 PM

Fix imports

sashank planned changes to this revision.EditedDec 9 2019, 11:10 PM

After discussion with @max: Instead of passing in the partition_set_definition_name and partition_name through resource config, we can grab these two things off tags. We already have the dagster/partition_name tag, so we should add the dagster/partition_set_definition_name tag.

This approach would come at the cost of not having a way to execute pipeline runs for a partition through dagit. But we could soon have a Partition selector (just like we have a preset selector) that would add the appropriate tags and pre-populate the config for the partition.

sashank planned changes to this revision.Dec 10 2019, 7:18 PM
sashank updated this revision to Diff 7525.Dec 10 2019, 11:05 PM

Update tests, use tags to get partition in partition_resource

sashank edited the summary of this revision. (Show Details)Dec 10 2019, 11:16 PM
sashank updated this revision to Diff 7531.Dec 10 2019, 11:21 PM
sashank edited the summary of this revision. (Show Details)

Up

sashank updated this revision to Diff 7533.Dec 10 2019, 11:25 PM

formatting

sashank updated this revision to Diff 7537.Dec 10 2019, 11:33 PM

Add missing files + lint

sashank added inline comments.Dec 10 2019, 11:39 PM
python_modules/dagster/dagster/core/partition/__init__.py
18

Not sure if this should return None or error out

sashank updated this revision to Diff 7549.Dec 11 2019, 12:12 AM

rerun tests

prha added inline comments.Dec 11 2019, 12:18 AM
python_modules/dagster/dagster/core/partition/__init__.py
18

Yeah, I think we should probably raise for all of these.

33

This should probably raise?

prha requested changes to this revision.Dec 11 2019, 12:24 AM

returning to your queue

This revision now requires changes to proceed.Dec 11 2019, 12:24 AM
sashank updated this revision to Diff 7557.Dec 11 2019, 12:28 AM

raise errors when partition_resource cannot be initialized

max resigned from this revision.Dec 11 2019, 10:02 PM

i think i've said my piece on this being useful in the edge case / for introspection -- let's not foreground use of this resource in ordinary examples / usage patterns, we want people to be able to slice config space independently of writing solid logic

alangenfeld added inline comments.Dec 12 2019, 4:22 PM
python_modules/dagster/dagster_tests/core_tests/partition_tests/test_partitions.py
20–25

why are the PartitionSelectors classes?

prha added inline comments.Dec 16 2019, 4:25 PM
python_modules/dagster/dagster_tests/core_tests/partition_tests/test_partitions.py
20–25

Yeah, I envisioned this being more complicated than it actually is... they should actually just be functions.

alangenfeld accepted this revision.Dec 16 2019, 4:31 PM

I think this seems fine to exist

sashank removed a reviewer: prha.Dec 17 2019, 12:22 AM
This revision is now accepted and ready to land.Dec 17 2019, 12:22 AM
prha accepted this revision.Dec 17 2019, 12:31 AM
sashank added a subscriber: max.Fri, Jan 24, 8:40 PM

After working more with partitions, I think introducing this would encourage implementing partitioned-based schedules using the anti-pattern @max mentioned. We can hold off on it until we see a clear need for a partition resource.

sashank abandoned this revision.Fri, Jan 24, 8:40 PM