Page MenuHomeElementl

rename SensorExecutionContext to SensorEvaluationContext
ClosedPublic

Authored by cdecarolis on Jun 14 2021, 6:27 PM.

Details

Summary

In the recent orchestration / execution context split, the word execution has taken on more meaning. While this has not been codified anywhere yet, the current thinking is that an ExecutionContext is a context involved in the core computation of the plan; having access to user code, resources, etc.

The SensorExecutionContext does not fit this bill, since it is used to evaluate config, which is still technically during the "orchestration" phase (constructing the plan, routing the steps of the plan, evaluation of config, etc).

One downside is that SensorContext is a bit of a vague name. Perhaps we could still include another word to more accurately reflect its purpose; SensorTickContext is one idea.

Test Plan

Unit and test docs build

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

+1 on moving way from ExecutionContext.

I think SensorContext is fine... did you also consider SensorEvaluationContext/ ScheduleEvaluationContext? I think we've been pretty good in the docs about "evaluating" sensors & schedules and "executing" pipelines.

In D8362#218480, @prha wrote:

+1 on moving way from ExecutionContext.

I think SensorContext is fine... did you also consider SensorEvaluationContext/ ScheduleEvaluationContext? I think we've been pretty good in the docs about "evaluating" sensors & schedules and "executing" pipelines.

Evaluate also seems pretty natural to me. I'd also do an accompanying docs pass to make sure we're communicating consistently if we decided to go that route.

I like SensorEvaluationContext and ScheduleEvaluationContext too.

requesting changes for q management

This revision now requires changes to proceed.Jun 15 2021, 4:11 PM

Change to SensorEvaluationContext

cdecarolis retitled this revision from rename SensorExecutionContext to SensorContext to rename SensorExecutionContext to SensorEvaluationContext.Jun 15 2021, 5:46 PM

I think this looks good, but now just concerned about whether we can land this without announcing a breaking change. It's a pretty trivial transform (codemod), but we will break folks who are using typing for their evaluation function signatures.

Interim change I guess is to implement some sort of abstract interface with the old name, let people switch to the new one, and then remove the old one with 0.12.0?

We're also going to change the Schedule context at the same time, yes?

In D8362#218962, @prha wrote:

I think this looks good, but now just concerned about whether we can land this without announcing a breaking change. It's a pretty trivial transform (codemod), but we will break folks who are using typing for their evaluation function signatures.

Interim change I guess is to implement some sort of abstract interface with the old name, let people switch to the new one, and then remove the old one with 0.12.0?

We're also going to change the Schedule context at the same time, yes?

Old name interface makes sense to me - maybe with a small deprecation warning at init.

I agree we should change the schedule context at the same time. Wasn't sure if we wanted to put in the same diff or not, but I'll pull that in.

prha requested changes to this revision.Jun 17 2021, 5:31 PM

returning for queue management

This revision now requires changes to proceed.Jun 17 2021, 5:31 PM
prha requested changes to this revision.Jun 25 2021, 6:59 PM

Do we think that people are actually constructing SensorExecutionContext and ScheduleExecutionContext? I was thinking that they were just using them as mypy types and that we just needed to export an interface that ScheduleEvaluationContext/SensorEvaluationContext implemented (to pass the isinstance checks).

Either way, we should probably export them from the main dagster module.

This revision now requires changes to proceed.Jun 25 2021, 6:59 PM
In D8362#222701, @prha wrote:

Do we think that people are actually constructing SensorExecutionContext and ScheduleExecutionContext? I was thinking that they were just using them as mypy types and that we just needed to export an interface that ScheduleEvaluationContext/SensorEvaluationContext implemented (to pass the isinstance checks).

Either way, we should probably export them from the main dagster module.

Given that they were already a top-level export, I think that it's possible people were using them for real. Agreed about the top-level import though.

SensorExecutionContext and ScheduleExecutionContext stay top level exports

Make old contexts type aliases of new contexts

the build errors for docs snapshots look real, but o.w. lgtm

This revision is now accepted and ready to land.Jun 29 2021, 4:12 PM