Page MenuHomePhabricator

[core] add context to type_checks
ClosedPublic

Authored by alangenfeld on Jan 2 2020, 10:44 PM.

Details

Summary

This diff makes context available to type_checks in a non-breaking way.

  • uses get_args to determine whether to pass through context - enforce 1 arg or 2 args with context first rules.
  • adds a TypeCheckContext that is created for type checks.
  • adds required_resource_keys to DagsterType
Test Plan

added tests

Diff Detail

Repository
R1 dagster
Branch
type-check (branched from master)
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

alangenfeld created this revision.Jan 2 2020, 10:44 PM

one of concerns with this direction is that providing this context to the type checks makes it possible to do "call site specific" shenanigans on a "type" which feels like a dangerous place for coupling

brainstorming:

  • add a function separate from type check that gets context - maybe formalize the idea of precondition checks. Maybe do something do prevent them from happening repeatedly on output and again on ever input use
  • overhaul the type check from just a function to something richer that can support these varying constraints more gracefully - akin to inpuy_hydration_config & co
  • don't make any changes and revisit the motivating examples to consider alternative solutions to the broader problem

"one of concerns with this direction is that providing this context to the type checks makes it possible to do "call site specific" shenanigans on a "type" which feels like a dangerous place for coupling"

I'm not following this concern.

Pretty clear to me that types should be able to declare required resources.

Re: API. I think this is part of the broader discussion of how many API changes to make in type system and whether or not they should be soft- or hard-breaking. Seems like either we temporarily do something like this and warn, and then break in 0.8.0. Or we just hard break now.

Overall I think this is pretty reasonable

python_modules/dagster/dagster/core/engine/engine_inprocess.py
437

how does this relate to the logic in dagster_event_sequence_for_step

python_modules/dagster/dagster/core/types/runtime/runtime_type.py
44 ↗(On Diff #8327)

cxt not cnt. levenstein distance from naughty word should be > 1

schrockn requested changes to this revision.Jan 7 2020, 5:15 PM

q mgmt

This revision now requires changes to proceed.Jan 7 2020, 5:15 PM

Definitely want to discuss this tmrw. I think this should go in

alangenfeld updated this revision to Diff 9506.Feb 11 2020, 5:49 PM

The Return

alangenfeld retitled this revision from [RFC] add context to type_checks to [core] add context to type_checks.Feb 11 2020, 5:55 PM
alangenfeld edited the summary of this revision. (Show Details)
alangenfeld edited the test plan for this revision. (Show Details)
alangenfeld added a reviewer: prha.
alangenfeld updated this revision to Diff 9515.Feb 11 2020, 7:02 PM
alangenfeld edited the summary of this revision. (Show Details)

py2 and docs

alangenfeld updated this revision to Diff 9517.Feb 11 2020, 7:16 PM

rebase + fix pandas

prha accepted this revision.Feb 11 2020, 9:07 PM

yeah, this makes sense to me.

max added a comment.Feb 11 2020, 9:24 PM

I think we should hard break this now so that the changes are bundled with the other custom type changes

I think we should hard break this now so that the changes are bundled with the other custom type changes

I'm not against it - where does everyone else stand

schrockn requested changes to this revision.Feb 11 2020, 9:38 PM

q mgmt. good call @max

This revision now requires changes to proceed.Feb 11 2020, 9:38 PM
alangenfeld updated this revision to Diff 9542.Feb 11 2020, 9:52 PM

hard break

alangenfeld added inline comments.Feb 11 2020, 10:19 PM
examples/dagster_examples/intro_tutorial/custom_types_test.py
100–112

[1]

python_modules/dagster/dagster/utils/test/__init__.py
310–313

[1] test in the tutorial motivated this change to catch Failure

schrockn accepted this revision.Feb 11 2020, 10:39 PM

looks good

python_modules/dagster/dagster_tests/core_tests/runtime_types_tests/test_types.py
69–89

per in-person comment, file issue to refactor this and mark as good first issue

This revision is now accepted and ready to land.Feb 11 2020, 10:39 PM

that other test

leave issue # arround

more tutorial fixes

This revision was automatically updated to reflect the committed changes.