Page MenuHomePhabricator

[mypy][core] execution/plan
ClosedPublic

Authored by alangenfeld on Jan 20 2021, 11:11 PM.

Details

Summary

Trying to do a whole directory in a diff , I can break this up if desired.

Test Plan

mypy, 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

python_modules/dagster/dagster/check/__init__.py
102

Implicit return in function which does not return [misc]

seems mypy doesnt know raise_with_traceback is NoReturn

python_modules/dagster/dagster/core/execution/context/system.py
370–379 ↗(On Diff #29977)

pretty sure this is right - fixed typing issues exposed while working

Harbormaster returned this revision to the author for changes because remote builds failed.Jan 20 2021, 11:30 PM
Harbormaster failed remote builds in B24616: Diff 29977!
Harbormaster returned this revision to the author for changes because remote builds failed.Jan 21 2021, 7:51 PM
Harbormaster failed remote builds in B24655: Diff 30024!

It would be nice to have a better story for these named tuples. Unfortunately, it might require mucking around with metaclasses to support overriding init if we want to do runtime type checking.

python_modules/dagster/dagster/check/__init__.py
102

do we need raise_with_traceback now that we're on py3?

python_modules/dagster/dagster/core/execution/plan/compute.py
23

I wonder if its worth defining a superclass here that users can use to annotate the return type of their solid functions?

python_modules/dagster/dagster/core/execution/plan/inputs.py
42–47

Worth considering:

class StepInput(typing.NamedTuple):
    name: str
    dagster_type: DagsterType
    source: StepInputSource
This revision is now accepted and ready to land.Jan 22 2021, 5:19 PM
python_modules/dagster/dagster/core/execution/plan/plan.py
717

Should this be NoReturn?

It would be nice to have a better story for these named tuples. Unfortunately, it might require mucking around with metaclasses to support overriding init if we want to do runtime type checking.

What i am thinking is that once we trust our internal code more - we can just change serdes to go through a static constructor method with check and drop the overload of __new__. The NamedTuple syntax here is more cumbersome but mypy does parse and understand the types when used this way.

python_modules/dagster/dagster/core/execution/plan/inputs.py
42–47

for sure will happen in future diff

python_modules/dagster/dagster/core/execution/plan/plan.py
717

it doesn't always throw - so the implicit None gets returned

This revision was automatically updated to reflect the committed changes.