Page MenuHomeElementl

monitor sensor 2/ allow nullable pipeline_name in sensor def

Authored by yuhan on May 12 2021, 4:28 AM.



allow pipeline_name=None in SensorDefinition

Test Plan

unit + local dagit

image.png (650×2 px, 95 KB)

Diff Detail

R1 dagster
Lint Not Applicable
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.May 12 2021, 4:49 AM
Harbormaster failed remote builds in B30456: Diff 37436!
yuhan requested review of this revision.May 12 2021, 6:12 AM

do we want to use name prefixes to distinguish rather than subclassing and having some sort of flag on the external sensor?


the name prefix was the simplest approach. open to alternatives - a flag on SensorDefinition and ExternalSensorData might be cleaner

This revision is now accepted and ready to land.May 14 2021, 8:26 PM

monitor sensor -> pipeline sensor


I suspect that future readers of this code might find it confusing that sensors with _is_pipeline_sensor=True are sensors that do not target a pipeline. It might make sense to give it a name like _is_no_target_sensor? Alternatively, would it make sense to omit it and instead use sensor.pipeline_name is None in the situation where we would otherwise use it?

is_pipeline_sensor -> has_no_target


  • make target nullable (target.pipeline_name is still not nullable)
  • has_no_target -> no_target

I'm a bit skeptical on even having this no_target bool, its effectively just re-encoding state derivable from target being absent. We can fire an experimental warning if neither pipeline_name or job is passed in, but im not sure persisting a bool makes sense

yuhan requested review of this revision.Thu, Jun 3, 6:12 PM

@alangenfeld I was thinking of using no_target as the source of truth so we can error the case of neither pipeline_name or job. But you are right that persisting a bool may be too early. Firing an experimental warning sounds good. Updated.

This revision is now accepted and ready to land.Thu, Jun 3, 8:23 PM