Page MenuHomeElementl

Replace namedtuple with typing.NamedTuple in outputs.py
Needs RevisionPublic

Authored by sandyryza on Dec 28 2020, 5:55 PM.

Details

Summary

This is to prove out the pattern.

Why not use dataclasses?

  • dataclasses require a dependency for Python 3.6, but not for 3.7+, which complicates our packaging. There's surely a way around this, but could be a pain.
  • dataclasses are mutable by default. Though that's not really a big deal because it's also easy to just set frozen=True.
  • NamedTuples can be unpacked, dataclasses can not. Users probably shouldn't be unpacking our NamedTuples, but, if they are, we'd break them.

Why might we eventually want to move to dataclasses?

  • NamedTuples and inheritance don't mix well. There are a bunch of places in the codebase where we try to get them to mix, which causes awkwardness.

If we want to switch to dataclasses in the future, it would not be difficult.

Depends on D5768.

Test Plan

bk

Diff Detail

Repository
R1 dagster
Branch
typing-namedtuple (branched from master)
Lint
Lint Passed
Unit
No Test Coverage

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Dec 28 2020, 6:08 PM
Harbormaster failed remote builds in B23500: Diff 28575!
sandyryza retitled this revision from Replace namedtuple with typing.NamedTuple for StepOutputHandle to Replace namedtuple with typing.NamedTuple for in outputs.py.Dec 29 2020, 5:17 PM
sandyryza retitled this revision from Replace namedtuple with typing.NamedTuple for in outputs.py to Replace namedtuple with typing.NamedTuple in outputs.py.Dec 29 2020, 5:23 PM
sandyryza edited the summary of this revision. (Show Details)
Harbormaster returned this revision to the author for changes because remote builds failed.Dec 29 2020, 6:22 PM
Harbormaster failed remote builds in B23518: Diff 28594!
Harbormaster returned this revision to the author for changes because remote builds failed.Dec 29 2020, 6:47 PM
Harbormaster failed remote builds in B23522: Diff 28598!

from what i can tell there are no differences to this approach besides the additional typing information on __annotations__
https://docs.python.org/3/library/typing.html

this seems very uncontroversial to me

absolute-win

This revision is now accepted and ready to land.Jan 4 2021, 5:06 PM
schrockn requested changes to this revision.Jan 4 2021, 5:12 PM

I don't think we should eliminate the check calls until we turn on mypy typing

This revision now requires changes to proceed.Jan 4 2021, 5:12 PM

to be clear i love the new syntax.