Assorted type annotations #8356

smackesey · 2022-06-13T16:40:37Z

Summary & Motivation

This is a large PR with a lot of type annotation improvements and a few other minor changes. Unfortunately it is difficult to break up into anything smaller because typing changes cause cascading mypy errors-- adding annotations in one place causes errors to surface in another place, which then needs type annotations added, etc. However review shouldn't be too difficult since most of the changes are pretty trivial:

Added declarations of instance attributes to several classes. In some cases corresponding type declarations at the point of assignment of instance variables were removed.
Many type annotations using Dict/List/Set replaced with Mapping/Sequence/AbstractSet respectively. Sometimes
corresponding runtime type checks were also swapped (e.g. check.list_param > check.sequence_param)
Added many missing argument and return type annotations
Factored out a few complex Callable or Union types into more readable type aliases
Fixed various incorrect annotations
Added some type-ignores where added annotations triggered mypy bugs

In several other places check.not_none was inserted where there was an implicit assumption that a value was non-null, but this wasn't inferrable by the type-checker.

There are also a few minor runtime changes:

dagster.config.traversal_context: config schema args should be non-optional
dagster.core.definitions.configurable: use isinstance (legible to type-checkers) instead of custom method that calls
isinstance

Finally, internal requires updates to match these, here's the sister PR: https://github.com/dagster-io/internal/pull/2498. That should be merged after this, which will need to be merged with the internal compatibility check still red.

How I Tested These Changes

Existing test suite.

vercel · 2022-06-13T16:40:40Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

3 Ignored Deployments

Name	Status	Updated
dagit-storybook	⬜️ Ignored (Inspect)	Jul 8, 2022 at 8:24PM (UTC)
dagster	⬜️ Ignored (Inspect)	Jul 8, 2022 at 8:24PM (UTC)
dagster-oss-cloud-consolidated	⬜️ Ignored (Inspect)	Jul 8, 2022 at 8:24PM (UTC)

smackesey · 2022-06-13T16:40:51Z

Current dependencies on/for this PR:

master
- PR Type annotations for IOManager #8308
  - PR Assorted type annotations #8356 👈
    - PR more types #8733

This comment was auto-generated by Graphite.

sryza · 2022-07-05T16:24:34Z

Is this meant to be marked as a draft?

smackesey · 2022-07-05T18:23:00Z

Is this meant to be marked as a draft?

Whoops forgot to unmark it

alangenfeld · 2022-07-08T13:58:34Z

python_modules/dagster/dagster/core/asset_defs/assets.py

+    _node_def: NodeDefinition
+    _keys_by_input_name: Mapping[str, AssetKey]
+    _keys_by_output_name: Mapping[str, AssetKey]
+    _partitions_def: Optional[PartitionsDefinition]
+    _partition_mappings: Mapping[AssetKey, PartitionMapping]
+    _asset_deps: Mapping[AssetKey, AbstractSet[AssetKey]]
+    _resource_defs: Mapping[str, ResourceDefinition]
+    _group_names_by_key: Mapping[AssetKey, str]
+    _selected_asset_keys: AbstractSet[AssetKey]
+    _can_subset: bool
+    _metadata_by_asset_key: Mapping[AssetKey, MetadataUserInput]
+


seeing this sent me on a journey trying to better understand class attributes vs instance attributes - I'm surprised the types didnt propagate from __init__ correctly though, i guess issues witch check calls?

alangenfeld · 2022-07-08T14:00:33Z

python_modules/dagster/dagster/core/definitions/asset_layer.py

+    _asset_keys_by_node_input_handle: Mapping[NodeInputHandle, AssetKey]
+    _asset_info_by_node_output_handle: Mapping[NodeOutputHandle, AssetOutputInfo]
+    _asset_deps: Mapping[AssetKey, AbstractSet[AssetKey]]
+    _dependency_node_handles_by_asset_key: Mapping[AssetKey, Set[NodeHandle]]
+    _source_assets_by_key: Mapping[AssetKey, "SourceAsset"]
+    _asset_defs_by_key: Mapping[AssetKey, "AssetsDefinition"]
+    _asset_defs_by_node_handle: Mapping[NodeHandle, Set["AssetsDefinition"]]
+    _io_manager_keys_by_asset_key: Mapping[AssetKey, str]


alright seeing this repeated im definitely curious to hear about why you took this approach

A few reasons:

This is the standard way to annotate instance attrs in Python-- see PEP 526. There is a special ClassVar annotation you use for class variables.

IMO it is a lot more readable than annotations in `init.

I'm surprised the types didnt propagate from init correctly though, i guess issues witch check calls?

Yes, the types are often not inferred correctly in __init__. I think it would work the same as the above style if we explicitly annotated the type for every assignment in __init__, but I'm not 100% sure. Regardless, it's a lot harder to read/find the declared type when its embedded in often lengthy __init__ logic.

The above style matches the style used by dataclasses and pydantic, which I still hope to transition to

alangenfeld · 2022-07-08T15:01:05Z

python_modules/dagster/dagster_tests/core_tests/definitions_tests/test_schedule_invocation.py

-def test_schedule_context_backcompat():
-    # If an instance of ScheduleEvaluationContext is a ScheduleExecutionContext, then annotating as
-    # ScheduleExecutionContext and passing in a ScheduleEvaluationContext should pass mypy
-    assert isinstance(ScheduleEvaluationContext(None, None), ScheduleExecutionContext)


it would be surprising to accidentally delete since theres a comment next to it - but you could preserve a test here that just annotates some functions as am additional guard

I actually realize now that I made a mistake in making ScheduleExecutionContext a type alias, because it's a literal alias of ScheduleEvaluationContext that's exported top-level. I guess we'll remove with 1.0 but for now I'll restore it (and the test), though the comment I think is wrong, we're not testing if mypy passes here but rather python isinstance

smackesey mentioned this pull request Jun 13, 2022

Type annotations for IOManager #8308

Merged

smackesey force-pushed the sean/type-annotations branch 2 times, most recently from 08db1ba to 951bcab Compare June 13, 2022 20:49

smackesey force-pushed the sean/io-manager-types branch from bd60c90 to 653a40a Compare June 14, 2022 00:39

smackesey force-pushed the sean/type-annotations branch from 951bcab to d109928 Compare June 14, 2022 00:39

Base automatically changed from sean/io-manager-types to master June 14, 2022 01:12

smackesey force-pushed the sean/type-annotations branch 2 times, most recently from 021bb43 to ce5eed9 Compare June 20, 2022 19:12

vercel bot deployed to Preview – dagster June 20, 2022 19:15 View deployment

smackesey force-pushed the sean/type-annotations branch 2 times, most recently from fc851e3 to 42d50ba Compare July 1, 2022 14:32

vercel bot deployed to Preview – dagster July 1, 2022 14:42 View deployment

smackesey mentioned this pull request Jul 4, 2022

more types #8733

Merged

smackesey force-pushed the sean/type-annotations branch 2 times, most recently from 7701e08 to ddf970b Compare July 5, 2022 13:20

smackesey requested review from dpeng817 and sryza July 5, 2022 16:21

smackesey marked this pull request as ready for review July 5, 2022 18:22

smackesey requested a review from alangenfeld July 7, 2022 01:31

alangenfeld approved these changes Jul 8, 2022

View reviewed changes

smackesey force-pushed the sean/type-annotations branch from ddf970b to d739da8 Compare July 8, 2022 17:38

smackesey added 5 commits July 8, 2022 16:22

types

52b7ca5

fixes

00813f8

fix

0d4613c

temp commit

602a802

more

c17e320

smackesey added 13 commits July 8, 2022 16:22

fixes

09bcb34

fix

7bcf377

revert accidental doc change

8394ff9

fix

4618f89

get_args

9b19405

fix

1b880ed

fixes

e583ca4

fixes

5a18ce1

fixes

bed4534

bump typing-extensions

e58f6c6

revert ScheduleExecutionContext changes

a0513d6

rebase/fixes

450dbfe

rebase

efc4198

smackesey force-pushed the sean/type-annotations branch from d739da8 to efc4198 Compare July 8, 2022 20:24

smackesey merged commit 2d35b84 into master Jul 8, 2022

smackesey deleted the sean/type-annotations branch July 8, 2022 21:30

Jiafi pushed a commit to Jiafi/dagster that referenced this pull request Jul 11, 2022

Assorted type annotations (dagster-io#8356)

dbbb534

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Assorted type annotations #8356

Assorted type annotations #8356

smackesey commented Jun 13, 2022 •

edited

vercel bot commented Jun 13, 2022 •

edited

smackesey commented Jun 13, 2022 •

edited

sryza commented Jul 5, 2022

smackesey commented Jul 5, 2022 •

edited

alangenfeld Jul 8, 2022

alangenfeld Jul 8, 2022

smackesey Jul 8, 2022

alangenfeld Jul 8, 2022

smackesey Jul 8, 2022

Assorted type annotations #8356

Assorted type annotations #8356

Conversation

smackesey commented Jun 13, 2022 • edited

Summary & Motivation

How I Tested These Changes

vercel bot commented Jun 13, 2022 • edited

smackesey commented Jun 13, 2022 • edited

sryza commented Jul 5, 2022

smackesey commented Jul 5, 2022 • edited

alangenfeld Jul 8, 2022

Choose a reason for hiding this comment

alangenfeld Jul 8, 2022

Choose a reason for hiding this comment

smackesey Jul 8, 2022

Choose a reason for hiding this comment

alangenfeld Jul 8, 2022

Choose a reason for hiding this comment

smackesey Jul 8, 2022

Choose a reason for hiding this comment

smackesey commented Jun 13, 2022 •

edited

vercel bot commented Jun 13, 2022 •

edited

smackesey commented Jun 13, 2022 •

edited

smackesey commented Jul 5, 2022 •

edited