Page MenuHomePhabricator

Remove execution_params from ScheduleDefinition
ClosedPublic

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

Details

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.

Now that we have functions that run to resolve the environment dict and tags, any error in either function would cause the scheduler page to error.

With this diff, we resolve the pipeline name and mode directly off the schedule definition (instead of getting them off execution params on the client side) and don't fail loading the page on any user errors in the schedule definition.

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
127

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
11–13

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
11–13

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
127

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
11–13

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
11–13

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
11–13

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
sashank edited the summary of this revision. (Show Details)Mar 10 2020, 7:20 PM
sashank edited the test plan for this revision. (Show Details)
sashank updated this revision to Diff 10409.Mar 10 2020, 7:21 PM
sashank edited the test plan for this revision. (Show Details)

Up

sashank updated this revision to Diff 10412.Mar 10 2020, 7:31 PM

Add test

Harbormaster failed remote builds in B8412: Diff 10411!
sashank updated this revision to Diff 10415.Mar 10 2020, 8:09 PM

Update snapshots

sashank updated this revision to Diff 10418.Mar 10 2020, 9:06 PM

make gql

prha requested changes to this revision.Mar 10 2020, 10:29 PM
prha added inline comments.
js_modules/dagit/src/schedules/ScheduleRow.tsx
298 ↗(On Diff #10418)

this seems weird, to populate the config editor with an error message...

Ideal scenario is that the config editor is blank with a standard error message showing up somewhere (in a toast?)?

This revision now requires changes to proceed.Mar 10 2020, 10:29 PM
sashank updated this revision to Diff 10422.Mar 10 2020, 10:49 PM

Show disabled button if environmentConfigYaml is null

sashank updated this revision to Diff 10423.Mar 10 2020, 10:50 PM

Null check instead of simple presence check

Harbormaster completed remote builds in B8420: Diff 10423.
prha accepted this revision.Mar 11 2020, 1:02 AM
This revision is now accepted and ready to land.Mar 11 2020, 1:02 AM
This revision was automatically updated to reflect the committed changes.