Page MenuHomePhabricator

RFC: Use Config Type instances instead of classes; kill inst()
AbandonedPublic

Authored by schrockn on Dec 6 2019, 9:39 PM.

Details

Summary

This is risky and I think it should probably be chunked up,
but this changes the config type system to only operate on instances
rather than classes. I think this is huge leap forward in terms of
being able to understand and deal with this. Creating objects is
just way more intuitive than creating classes.

Note that this is an RFC. We can chat about it, but it is probably good
to chop this up as there are a couple questionable things in here.

Notably passing config type instances to python.typing is not supported.
I had to add a fake call method to configtype in order to trick python.typing
into allowing the object. I assume this would break mypy as well, but who knows.

This also eliminates the invariant that two different type instances with the
same name is not allows. We used to enforce that there is only one instance. As
we move away from named types this will not be a concern as the hashing will
take care of this.

Test Plan

BK

Diff Detail

Repository
R1 dagster
Branch
config-sin-eating-real-rfc
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

schrockn created this revision.Dec 6 2019, 9:39 PM
schrockn retitled this revision from RFCL Use Config Type instances instead of classes; kill inst() te to RFC: Use Config Type instances instead of classes; kill inst().Dec 6 2019, 9:40 PM
nate added inline comments.Dec 6 2019, 9:56 PM
python_modules/dagster/dagster/core/types/field.py
61

nope is extraneous?

Harbormaster failed remote builds in B5805: Diff 7202!
schrockn updated this revision to Diff 7205.Dec 6 2019, 10:02 PM

up

python_modules/dagster/dagster/core/types/field.py
61

yeah this is me just ensuring that this codepath is never hit. note that this is an RFC. I plan on chopping this up into probably 10-15 lower risk diffs given how subtle this is.

schrockn abandoned this revision.Dec 9 2019, 9:57 PM

stacked now