Page MenuHomePhabricator

Remove execution_params from ScheduleDefinition
Changes PlannedPublic

Authored by sashank on Dec 10 2019, 2:10 AM.

Details

Reviewers
prha
alangenfeld
Summary

execution_params really didn't make sense on ScheduleDefinition, so taking it out. Helps clean up code in the schedule mutation and schedule definition resolver too.

Additional benefit is that we can create ExecutionParams (and therefore a PipelineRun) in other places without a Schedule and just a ScheduleDefinition.

Test Plan

unit

Diff Detail

Repository
R1 dagster
Branch
schedule-def-refactor
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

sashank created this revision.Dec 10 2019, 2:10 AM
sashank edited the summary of this revision. (Show Details)Dec 10 2019, 2:11 AM
sashank added a reviewer: alangenfeld.
sashank edited the summary of this revision. (Show Details)
sashank added inline comments.Dec 10 2019, 2:16 AM
python_modules/dagster/dagster/core/definitions/schedule.py
117

Debating on whether to just replace pipeline_name with selector, which would let us pass in a solid_subset as well.

sashank added inline comments.Dec 10 2019, 10:17 AM
python_modules/dagster-graphql/dagster_graphql/implementation/fetch_schedules.py
39–41

If we got rid of the schedule arg here, then we could have this as a property on ScheduleDefinition. The only reason we need schedule is to add a single tag.

Then, in start_scheduled_execution, we could just add the one tag onto ExecutionParams manually.

The only question is whether it makes sense to have an execution_params property on ScheduleDefinition.

alangenfeld added inline comments.Dec 10 2019, 4:46 PM
python_modules/dagster-graphql/dagster_graphql/implementation/fetch_schedules.py
39–41

I've already forgotten the difference between Schedule and ScheduleDefinition - having those names so close is confusing

python_modules/dagster/dagster/core/definitions/schedule.py
117

seems good though be mindful of breaking change sequencing

alangenfeld resigned from this revision.Dec 10 2019, 8:54 PM
prha added inline comments.Dec 10 2019, 9:18 PM
python_modules/dagster-graphql/dagster_graphql/implementation/fetch_schedules.py
39–41

yeah, maybe something like ScheduleInvocation?

What's the argument against having ExecutionParams on ScheduleDefinition?

sashank added inline comments.Dec 10 2019, 9:54 PM
python_modules/dagster-graphql/dagster_graphql/implementation/fetch_schedules.py
39–41

Good idea - a rename would be helpful here.

The argument against having ExecutionParams on ScheduleDefintion is because ExecutionParams is part of dagster_graphql. There isn't any other reference to ExecutionParams in dagster except for the current one on ScheduleDefinition.

sashank planned changes to this revision.Dec 11 2019, 12:29 AM
prha added inline comments.Dec 11 2019, 12:31 AM
python_modules/dagster-graphql/dagster_graphql/implementation/fetch_schedules.py
39–41

Ah, yeah... we probably do not want execution params on ScheduleDefinition. I like the idea of keeping all the core definitions having similar arguments (config_fn, tags_fn, etc)

sashank updated this revision to Diff 8331.Jan 3 2020, 12:13 AM
sashank edited the summary of this revision. (Show Details)

rebase

sashank planned changes to this revision.Jan 3 2020, 12:13 AM