Page MenuHomePhabricator

Support python dictionaries as inputs and outputs
AbandonedPublic

Authored by schrockn on Tue, Sep 3, 10:10 PM.

Details

Reviewers
alangenfeld
Group Reviewers
Restricted Project
Summary

This is a huge oversight in our API. Support for passing dictionaries in betweens solids. Also need to do sets.

Test Plan

Buildkite

Diff Detail

Repository
R1 dagster
Branch
support-dict-1
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

schrockn created this revision.Tue, Sep 3, 10:10 PM
schrockn updated this revision to Diff 4189.Tue, Sep 3, 10:42 PM
schrockn added a reviewer: Restricted Project.

up

Harbormaster completed remote builds in B3354: Diff 4194.
sashank added inline comments.
python_modules/dagster/dagster/core/types/__init__.py
27

Would it be possible to rename the existing config field Dict to something else, so that we can name this Dict instead of PythonDict? I could see users expecting the following to work:

from dagster import Dict

OutputDefinition(Dict)

since the following work:

from dagster import Int, Bool, String, Float

OutputDefinition(Int)
OutputDefinition(Bool)
OutputDefinition(String)
OutputDefinition(Float)

and because Int, Bool, String, and Float aren't called PythonInt, etc.

Yeah I agree that is where this should end up, but it will be tricksy. (See the issue https://github.com/dagster-io/dagster/issues/1710 where this awkwardness is noted) Notably the semantics of Dict are going to be different in the config and runtime cases. In addition Dict in the config context takes required fields at this point.

Right now the signature of Dict is:

Dict(fields)

And is only valid in the config system.

So we would probably, initially, want to support:

Runtime:

Dict

Config:

Dict
Dict(fields)

Then we can figure out what to do in the runtime case later.

I'm going to update this to remove PythonDict from the top-level include. Will demonstrate what it will take to use Dict in both contexts in a follow-on.

schrockn updated this revision to Diff 4262.Thu, Sep 5, 1:29 PM

remove from top-level include

alangenfeld requested changes to this revision.Fri, Sep 6, 3:39 PM
alangenfeld added a subscriber: alangenfeld.

I think we want Dict from typing for this, and we should potentially validate key and value types ie Dict[str, int] to be consistent with List

This revision now requires changes to proceed.Fri, Sep 6, 3:39 PM
alangenfeld accepted this revision.Thu, Sep 12, 5:10 PM
alangenfeld added inline comments.
python_modules/dagster/dagster/core/types/mapping.py
21–22

add list as well while youre here?

This revision is now accepted and ready to land.Thu, Sep 12, 5:10 PM
schrockn abandoned this revision.Fri, Sep 13, 1:56 PM