Page MenuHomeElementl

add evaluate_sensor util function for testing sensors
Needs RevisionPublic

Authored by prha on Feb 26 2021, 5:49 PM.

Details

Summary

will resolve the sensor into run requests, but will not actually execute the run

Test Plan

add bk test

Diff Detail

Repository
R1 dagster
Branch
prha/test_sensor
Lint
Lint Passed
Unit
No Test Coverage

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Feb 26 2021, 6:04 PM
Harbormaster failed remote builds in B26544: Diff 32429!
prha requested review of this revision.Feb 26 2021, 7:35 PM

I've noticed we've been getting a lot of questions about this.

Does this add a lot on top of sensor.get_execution_data(SensorExecutionContext())?

I wonder if the solution just requires:

  • Making it possible to construct construct a SensorExecutionContext without a DagsterInstance
  • Documentation

I agree it doesn't add a lot on top of it, but I don't think instance should be optional on the context.

I don't think instance should be optional on the context.

Why not? I suspect <50% of implementations will use it.

FWIW for IOManagers we've purposely made most of the fields on the context optional so that users can test them with less boilerplate.

Definitely a little hand-wavy, so not a hard opinion here, but constructing a context feels like a leaked implementation abstraction. In my head, a context is an object constructed by the framework that makes things available to the solid during normal execution. Having an abstraction layer purely for testing seems reasonable.

Is the concern here just code duplication?

I think this is good. My final question is what we should default the instance to

python_modules/dagster/dagster/utils/test/__init__.py
361

default to this or to ephemeral?

This revision now requires changes to proceed.Mar 17 2021, 6:43 PM

Sorry for the delay here. My main concern here is that evaluate_sensor is another API that users will need to look up and remember. If someone is writing a sensor, they're already dealing with the ScheduleExecutionContext API and SensorDefinition APIs.

My other concern is about consistency with other testing APIs. E.g. for IOManagers, our current recommended practice is to construct an OutputContext or InputContext and pass it in to the IOManager. Not necessarily saying that's the write way, but I'm concerned about introducing a new API that entrenches the gap.

Yeah I think the core point of contention is whether or not the constructor for contexts should be considered private. I think they should. We could even go the point where we change *Context classes to be abstract classes only.

I think the context construction pathways are so intricate and tied to internal implementations that they should be considered private

Making the context construction private sounds reasonable to me.

In my mind, the question is between these two:

def my_test():
    context = SensorExecutionContext.create_for_test(instance=..., last_run_key=..., last_completion_time=...)
    run_requests = my_sensor.get_execution_data(context)
    ...

(maybe get_execution_data would read better as evaluate or get_run_requests?)

def my_test():
    run_requests = evaluate_sensor(my_sensor, instance=..., last_run_key=..., last_completion_time=...)
    ...

ah ok. curious to hear @prha's thoughts on the diff btwn those two

I don't feel super strongly about either direction, but I do think we should shoot for consistency in our testing APIs.

I'm completely fine with the static method approach.