Page MenuHomePhabricator

RFC: Rationalize type check system
ClosedPublic

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

Details

Reviewers
alangenfeld
schrockn
prha
Group Reviewers
Restricted Project
Commits
R1:b043308d8b85: RFC: Rationalize type check system
Summary

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

Unit

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

max created this revision.Fri, Nov 8, 5:58 PM
max edited the summary of this revision. (Show Details)Fri, Nov 8, 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.Mon, Nov 11, 6:10 PM

very nice. thanks for looking at this.

python_modules/dagster/dagster/core/definitions/events.py
280

yeah this is much better

python_modules/dagster/dagster/core/types/python_dict.py
46

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.Mon, Nov 11, 6:10 PM
max updated this revision to Diff 6441.Mon, Nov 11, 8:56 PM

Rebase

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

Simplify further