Page MenuHomePhabricator

RFC: Rationalize type check system

Authored by max on Nov 8 2019, 5:58 PM.


Group Reviewers
Restricted Project
R1:b043308d8b85: RFC: Rationalize type check system

In the status quo ante, evaluating a type check may return None or yield an instance of TypeCheck (=passed) or else raise Failure (=failed). This seems like a bad interface for a couple of reasons.

  • First, it's quite natural for users to return True/False to indicate success/failure in the ordinary case, especially as they ramp up (and let the Dagster machinery generate events as needed).
  • Second, the interaction of the typecheck_metadata_fn and type_check is obscure. Why not simply allow a TypeCheck to be returned from the type_check when a user wants to include additional metadata?
  • Third, when a type check fails, why shouldn't we be able to return metadata about the failure? i.e., by returning a TypeCheck(success=False)

Stacked on D1389

Test Plan


Diff Detail

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

Event Timeline

max created this revision.Nov 8 2019, 5:58 PM
max edited the summary of this revision. (Show Details)Nov 8 2019, 6:05 PM

on first pass pretty sure I like this - I think there is an interesting option we open up being able to opt in to failed TypeChecks not halting pipeline execution vs. raised Failure always halting. I'll take some more time to think and let others comment

schrockn accepted this revision.Nov 11 2019, 6:10 PM

very nice. thanks for looking at this.


yeah this is much better


So i think this might be a regression in the API. Using an exception does make composite types easier not to screw up.

This revision is now accepted and ready to land.Nov 11 2019, 6:10 PM
max updated this revision to Diff 6441.Nov 11 2019, 8:56 PM


max updated this revision to Diff 6444.Nov 11 2019, 10:27 PM

Simplify further

This revision was automatically updated to reflect the committed changes.