Page MenuHomePhabricator

[sensors-8] file toy sensor
ClosedPublic

Authored by prha on Fri, Nov 20, 2:52 AM.

Details

Summary

add a toy sensor that fires a pipeline execution every time a file in given directory
changes

Test Plan

none

Diff Detail

Repository
R1 dagster
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Fri, Nov 20, 3:07 AM
Harbormaster failed remote builds in B21467: Diff 26046!
prha requested review of this revision.Fri, Nov 20, 3:48 PM
python_modules/dagster-test/dagster_test/toys/sensors.py
8–11

I think it would be good to structure this a function that returns a sensor and that takes a folder argument to make it more generic, since this is an example

21

error message not accurate"??

21

also for our code examples would strongly prefer limiting exception-as-control-flow as much as possible. Can we just check up front if the directory exists? Or contain the try catch to a more tightly scoped function?

47–49

This error message looks inaccurate

54

hmm. does this object need to be specialized to sensors?

60

it is the execution key that make this idempotent correct?

dgibson added inline comments.
python_modules/dagster-test/dagster_test/toys/sensors.py
60

yeah. One thing i've been debating is if we should make execution_key required (but maybe none-able for some of the non-partition-y cases @sandyryza presented). Seems like you should opt-out of idempotency/run-exactly-once rather than opt-in

python_modules/dagster-test/dagster_test/toys/sensors.py
60

This is kind of why I have these two sensor implementations, showing the two approaches:

The first relies on the user doing the bookkeeping, by using the last evaluation time, to find the delta.
The second relies on the job tick table to enforce idempotency using execution key. Potentially we should pass the execution key in as a run tag and use run storage instead of the tick table for these checks.

python_modules/dagster-test/dagster_test/toys/sensors.py
60

I don't think the second is necessarily totally incompatible with the first? we could still have a last_successful_execution_time or cursor that you pass in, so that you don't have to return the full set of SensorRunParams every time the sensor runs - it would just be important that that time be the checkpoint time after which we know every run fired

I like it!

python_modules/dagster-test/dagster_test/toys/sensors.py
60

Can we use f-strings to make this Python3-ic?

prha marked 8 inline comments as done.

fstrings

this seems good to get in and iterate on, we should definitely bring back the execution key for our example though?

python_modules/dagster-test/dagster_test/toys/sensors.py
12–21

this comment seems not right?

85–91

where did the execution_key go :(

To keep harping on this, I think execution_key should be required (but Noneable)

This revision is now accepted and ready to land.Tue, Nov 24, 5:05 PM
This revision was automatically updated to reflect the committed changes.