Page MenuHomePhabricator

mypy
ClosedPublic

Authored by sandyryza on Dec 26 2020, 7:51 PM.

Details

Summary

This diff does two things:

  • Sets up buildkite to run a mypy step for each package
  • Fixes (or ignores) all mypy issues in dagster core

All packages except dagster core are currently excluded. The idea is that we can incrementally fix issues and include the other packages.

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

Harbormaster returned this revision to the author for changes because remote builds failed.Dec 26 2020, 8:08 PM
Harbormaster failed remote builds in B23427: Diff 28501!
Harbormaster returned this revision to the author for changes because remote builds failed.Dec 27 2020, 2:28 AM
Harbormaster failed remote builds in B23451: Diff 28525!
Harbormaster returned this revision to the author for changes because remote builds failed.Dec 27 2020, 4:30 AM
Harbormaster failed remote builds in B23452: Diff 28526!
Harbormaster returned this revision to the author for changes because remote builds failed.Dec 27 2020, 4:34 AM
Harbormaster failed remote builds in B23453: Diff 28527!
Harbormaster returned this revision to the author for changes because remote builds failed.Dec 27 2020, 9:56 PM
Harbormaster failed remote builds in B23460: Diff 28535!
Harbormaster returned this revision to the author for changes because remote builds failed.Dec 28 2020, 1:43 AM
Harbormaster failed remote builds in B23477: Diff 28552!
python_modules/dagster/dagster/core/snap/solid.py
179

whats the error here

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

this thing haunts my dreams

python_modules/dagster/dagster/utils/test/schedule_storage.py
41

whats up with these?

python_modules/dagster/dagster_tests/core_tests/test_composites.py
35–38

we should figure out a way not to require this or have a comment

python_modules/dagster/dagster/core/snap/solid.py
179

The issue is that the set of attrs is "computed" - i.e. SOLID_DEF_HEADER_PROPS is defined elsewhere and then referenced here. Mypy is smart enough to infer the set of attributes for a namedtuple if they're hardcoded where it's defined, but not smart enough to run the code that computes them.

So we could also fix this by just inlining the value of SOLID_DEF_HEADER_PROPS.

python_modules/dagster/dagster/utils/test/schedule_storage.py
41

mypy was complaining about that this sets an attr that doesn't exist: https://github.com/dagster-io/dagster/blob/master/python_modules/dagster/dagster_tests/core_tests/storage_tests/test_schedule_storage.py#L15

we can probably remove the `TestScheduleStorage.test = False" now that we have this. I'll do that,

python_modules/dagster/dagster_tests/core_tests/test_composites.py
35–38

Definitely agree that we should figure out a way to not require this. Are you saying that we should do that in this diff or down the road?

You are probably already aware of this, but this is following what we recommend users do: https://docs.dagster.io/tutorial/types#mypy-compliance.

python_modules/dagster/dagster/core/snap/solid.py
179

let's just inline them then

python_modules/dagster/dagster_tests/core_tests/test_composites.py
37

Yeah I feel increasingly bad about recommending that. IMO, you want to use Dagster types alongside mypy we should just be using InputDefinition and OutputDefinition everywhere (with the possible exception of built-ins), but I think I might be in the minority there.

will let @max resolve this and D5764

schrockn requested changes to this revision.Mon, Jan 4, 4:47 PM

So before you and @max start to do this I would like to see 1) a plan for adopting this and goals around it and 2) language that we will announce to the team around expectations here. E.g. is all new code typed? who is responsible for migrating? what is the overall plan?

This revision now requires changes to proceed.Mon, Jan 4, 4:47 PM
This revision was not accepted when it landed; it landed in state Needs Review.Wed, Jan 6, 8:55 PM
Closed by commit R1:5502b4bf4210: mypy (authored by sandyryza). · Explain Why
This revision was automatically updated to reflect the committed changes.