Page MenuHomePhabricator

(python-typing-to-inference-1) Top level not typing
ClosedPublic

Authored by schrockn on Dec 6 2019, 11:26 PM.

Details

Summary

This is part of a series of diffs in order to take more control over our
type system. In particular we are going push the use of python.Typing
up to inference layer only and only allow them where the runtime types
are used. This commingling of types has long been an issue (and will
remain a bit of one).

It is the work here that really exposed the issue of not
controlling the API for our types as we were passing instances
of ConfigType to the typing objects. https://dagster.phacility.com/D1564

This would be a breaking change for folks who have imported typing
and used it in the context of config.

Had to add support for untyped List in config to bring up to parity
with Set and Tuple.

Test Plan

BK

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

schrockn created this revision.Dec 6 2019, 11:26 PM
schrockn added inline comments.Dec 6 2019, 11:27 PM
python_modules/dagster/dagster/core/types/typing_api.py
17 ↗(On Diff #7221)

i'm going to push the check for DagsterList up the stack

schrockn updated this revision to Diff 7226.Dec 6 2019, 11:45 PM
schrockn retitled this revision from RFC: Top level not typing to Top level not typing.
schrockn edited the summary of this revision. (Show Details)

upmessage

schrockn updated this revision to Diff 7236.Dec 7 2019, 3:38 AM
schrockn retitled this revision from Top level not typing to (python-typing-to-inference-1) Top level not typing.
schrockn edited the summary of this revision. (Show Details)

upmessage

schrockn updated this revision to Diff 7240.Dec 7 2019, 4:46 AM
schrockn edited the summary of this revision. (Show Details)

upmessage

alangenfeld accepted this revision.Dec 9 2019, 11:40 PM
alangenfeld added a subscriber: themissinghlink.

As discussed in person - this is a short term side step to get the rest of the system changes to the config system through.

From our discussions I believe we need to get back to a state where dagster.List *is* typing.List (same for others). This could be by:

  • dunder call or meta-class shenanigans to allow typing.List to power config type instances
  • make the config API types separate (Array, Struct, etc). Bonus points if this "shape" validation system could be built in such a way that it is reusable for other purposes as @themissinghlink alluded to.
  • other better ideas
python_modules/dagster/dagster/core/types/wrapping.py
77–97

nit: whys optional different

This revision is now accepted and ready to land.Dec 9 2019, 11:40 PM
schrockn updated this revision to Diff 7354.Dec 9 2019, 11:55 PM

feedback

This revision was landed with ongoing or failed builds.Dec 10 2019, 12:05 AM
This revision was automatically updated to reflect the committed changes.
schrockn marked an inline comment as done.