Page MenuHomePhabricator

(type-system-changes-8) eliminate as_dagster_type
ClosedPublic

Authored by schrockn on Tue, Jan 28, 5:03 PM.

Details

Summary

This is being eliminated because as_dagster_type
actually adds and registers the type in the global registry,
where the semantics seem like they should be the exact
same as a PythonObjectDagsterType.

If one *wants* to do mapping the user can do it
via more explicit registration API.

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.Tue, Jan 28, 5:03 PM
schrockn added inline comments.Wed, Jan 29, 11:58 PM
python_modules/libraries/dagster-pyspark/dagster_pyspark/__init__.py
79

this is what is required to make this not breaking

schrockn updated this revision to Diff 9148.Wed, Jan 29, 11:58 PM
schrockn retitled this revision from (type-system-changes-8) as_dagster_type to (type-system-changes-8) RFC: elimlinate as_dagster_type.
schrockn edited the summary of this revision. (Show Details)
schrockn edited the test plan for this revision. (Show Details)
schrockn added reviewers: max, alangenfeld.

upmessage

examples/dagster_examples/airline_demo/solids.py
7

so this is what is required to ensure that the library is included and get the underlying symbol in one go. The other option is to name that dagster type DagsterDataFrame or some such

schrockn updated this revision to Diff 9159.Thu, Jan 30, 9:57 PM
schrockn retitled this revision from (type-system-changes-8) RFC: elimlinate as_dagster_type to (type-system-changes-8) RFC: eliminate as_dagster_type.

upmessage

schrockn updated this revision to Diff 9169.Thu, Jan 30, 11:05 PM

upmessage

schrockn updated this revision to Diff 9171.Thu, Jan 30, 11:10 PM

upmessage

schrockn updated this revision to Diff 9177.Thu, Jan 30, 11:38 PM

upmessage

max added inline comments.Tue, Feb 4, 11:19 PM
examples/dagster_examples/intro_tutorial/multiple_outputs.py
15–20

wherever we land, I don't think we can have this in tutorial code

schrockn added inline comments.Tue, Feb 4, 11:21 PM
examples/dagster_examples/intro_tutorial/multiple_outputs.py
15–20

I agree, except for perhaps a "mypy compliance" section

max added inline comments.Thu, Feb 6, 9:41 PM
python_modules/libraries/dagster-pyspark/dagster_pyspark/__init__.py
79

if python_type=RDD is already set in define_python_dagster_type, why do we need to set it again in map_python_type_to_dagster_type?

schrockn added inline comments.Thu, Feb 6, 9:43 PM
python_modules/libraries/dagster-pyspark/dagster_pyspark/__init__.py
79

define_python_dagster_type does not map the type, it just implements the type check for you

79

this preserves the existing behavior. I'm not sure libraries should be mapping types (i propose that in the guide) anyways

max requested changes to this revision.Thu, Feb 6, 9:51 PM
max added inline comments.
examples/dagster_examples/airline_demo/solids.py
7

i'm confused by what you mean here

examples/dagster_examples/intro_tutorial/composite_solids.py
19–24

i really think this shouldn't be in the tutorial code no matter where we land -- we can push it to another section about playing nicely with mypy

python_modules/dagster/dagster/core/types/dagster_type.py
370

this is rough, i think we need to put some thought into error messages, and also make sure that behavior is the same between python versions:

python 3.7

>>> from typing import List
>>> check.type_param(List, 'list')
dagster.check.ParameterCheckError: Param "list" was supposed to be a type. Got typing.List instead of type <class 'typing._GenericAlias'>

>>> from dagster import List
>>> check.type_param(List, 'list')
dagster.check.ParameterCheckError: Param "list" was supposed to be a type. Got <dagster.core.types.dagster_type.DagsterListApi object at 0x1073932b0> instead of type <class 'dagster.core.types.dagster_type.DagsterListApi'>

python 2.7

>>> from typing import List
>>> check.type_param(List, 'list')
typing.List

>>> from dagster import List
>>> check.type_param(List, 'list')
dagster.check.ParameterCheckError: Param "list" was supposed to be a type. Got <dagster.core.types.dagster_type.DagsterListApi object at 0x1073932b0> instead of type <class 'dagster.core.types.dagster_type.DagsterListApi'>
>>> check.type_param(List, 'list')
dagster.check.ParameterCheckError: Param "list" was supposed to be a type. Got <dagster.core.types.dagster_type.DagsterListApi instance at 0x109377d40> instead of type <type 'instance'>

it's not clear in either case how to proceed

This revision now requires changes to proceed.Thu, Feb 6, 9:51 PM
alangenfeld added inline comments.Thu, Feb 6, 11:49 PM
examples/dagster_examples/intro_tutorial/reusable_solids.py
21

rm these all these # type: Any comments

alangenfeld accepted this revision.Thu, Feb 6, 11:56 PM

im fine with this but lets not lose track on the follow up to restrict the api a bit

schrockn retitled this revision from (type-system-changes-8) RFC: eliminate as_dagster_type to (type-system-changes-8) eliminate as_dagster_type.Fri, Feb 7, 6:07 PM
schrockn edited the summary of this revision. (Show Details)
This revision was not accepted when it landed; it landed in state Needs Review.Fri, Feb 7, 6:25 PM
This revision was automatically updated to reflect the committed changes.

note: alex had accepted. I undid that state through update of commit message