Page MenuHomePhabricator
Differential D5843

root input manager
ClosedPublic
Actions

Authored by sandyryza on Wed, Jan 6, 5:45 PM.

Details

Reviewers
alangenfeld
yuhan
schrockn
Commits
R1:ae09fcdb1a03: root input manager
Summary

This diff does two related things makes it so that object managers (soon to be IO managers) are responsible for all loading of inputs that have upstream outputs.

Outstanding questions: https://elementl.quip.com/kkLXAoLIhqlX/Root-input-manager-type-loader-progress-doc

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

There are a very large number of changes, so older changes are hidden. Show Older Changes
sandyryza added a comment.Wed, Jan 6, 9:23 PM

up

Harbormaster returned this revision to the author for changes because remote builds failed.Wed, Jan 6, 9:40 PM
Harbormaster failed remote builds in B23773: Diff 28910!
sandyryza updated this revision to Diff 28922.Wed, Jan 6, 10:18 PM

up

Harbormaster returned this revision to the author for changes because remote builds failed.Wed, Jan 6, 10:33 PM
Harbormaster failed remote builds in B23782: Diff 28922!
sandyryza updated this revision to Diff 28928.Wed, Jan 6, 11:15 PM

up

Harbormaster returned this revision to the author for changes because remote builds failed.Wed, Jan 6, 11:31 PM
Harbormaster failed remote builds in B23787: Diff 28928!
sandyryza updated this revision to Diff 28930.Thu, Jan 7, 12:01 AM

up

Harbormaster returned this revision to the author for changes because remote builds failed.Thu, Jan 7, 12:19 AM
Harbormaster failed remote builds in B23789: Diff 28930!
sandyryza updated this revision to Diff 28938.Thu, Jan 7, 4:18 AM

up

Harbormaster returned this revision to the author for changes because remote builds failed.Thu, Jan 7, 4:34 AM
Harbormaster failed remote builds in B23800: Diff 28938!
sandyryza updated this revision to Diff 28940.Thu, Jan 7, 4:35 AM

up

sandyryza updated this revision to Diff 28944.Thu, Jan 7, 4:46 AM

up

Harbormaster failed remote builds in B23802: Diff 28940!Thu, Jan 7, 4:50 AM
Harbormaster returned this revision to the author for changes because remote builds failed.Thu, Jan 7, 5:02 AM
Harbormaster failed remote builds in B23805: Diff 28944!
sandyryza updated this revision to Diff 28945.Thu, Jan 7, 5:37 AM

up

Harbormaster returned this revision to the author for changes because remote builds failed.Thu, Jan 7, 5:55 AM
Harbormaster failed remote builds in B23806: Diff 28945!
sandyryza mentioned this in D5744: RFC: handle type-based loading with input managers.Thu, Jan 7, 6:56 PM
sandyryza updated this revision to Diff 28972.Thu, Jan 7, 6:58 PM

up

sandyryza updated this revision to Diff 28976.Thu, Jan 7, 7:17 PM

up

Harbormaster failed remote builds in B23831: Diff 28972!Thu, Jan 7, 7:17 PM
Harbormaster returned this revision to the author for changes because remote builds failed.Thu, Jan 7, 7:33 PM
Harbormaster failed remote builds in B23834: Diff 28976!
sandyryza requested review of this revision.Thu, Jan 7, 7:55 PM
sandyryza added inline comments.
python_modules/dagster/dagster/core/definitions/mode.py
60 ↗(On Diff #28972)

Note: we are introducing a new reserved resource key, "root_input_manager".

python_modules/dagster/dagster/core/definitions/pipeline.py
780

We are now delegating the question "can load the input definition?" to the root input manager.

python_modules/dagster/dagster/core/execution/plan/inputs.py
123

FromStepOutput now always uses the upstream object manager.

python_modules/dagster/dagster/core/storage/input_manager.py
14 ↗(On Diff #28976)

[1] This is kind of a hack to get tests working and merits broader discussion. It relates to two slightly bigger questions:

  • How does the root input manager communicate to the framework that it doesn't know how to handle a particular input type?
  • How does falling back to default values work?
55 ↗(On Diff #28976)

See [1]

96 ↗(On Diff #28976)

TODO: consolidate these into a single argument

278 ↗(On Diff #28976)

See [1]

sandyryza added reviewers: alangenfeld, yuhan, schrockn.Thu, Jan 7, 7:55 PM
sandyryza retitled this revision from root input manager to RFC: root input manager.Thu, Jan 7, 8:00 PM
sandyryza edited the summary of this revision. (Show Details)
sandyryza updated this revision to Diff 29006.Thu, Jan 7, 9:25 PM

up

Harbormaster failed remote builds in B23858: Diff 29006!Thu, Jan 7, 9:43 PM
schrockn added a comment.Fri, Jan 8, 12:45 PM

How fixed are we on the term "root"? I'm trying to recall other words that we have used or would use to describe inputs that have no upstream. I just want to make sure this is the best we have come up with.

SourceInputManager
StandaloneInputManager
UnattachedInputManager
OriginatingInputManager
SeedInputManager

My other comment is here is that we are kind of retreating from notion of being able to use solids you don't control or don't want to alter upstream of a solid you are working on. I was quite fond of that when we got there but it certainly introduced confusion on other dimensions. I just want to be explicit about the tradeoff we are making and to be sure we are comfortable with it. Namely, you cannot specify input loading behavior independent of the IOManager attached to the upstream output.

schrockn added inline comments.Fri, Jan 8, 12:55 PM
python_modules/dagster/dagster/core/storage/input_manager.py
17 ↗(On Diff #29006)

minor nit. i don't think one approach is better or worse but most of our sentinels are just classes

schrockn requested changes to this revision.Fri, Jan 8, 12:55 PM

just doing q mgmt. any other alternative names?

This revision now requires changes to proceed.Fri, Jan 8, 12:55 PM
sandyryza added a comment.EditedFri, Jan 8, 6:33 PM

How fixed are we on the term "root"? I'm trying to recall other words that we have used or would use to describe inputs that have no upstream. I just want to make sure this is the best we have come up with.

Definitely not fixed. SourceInputManager sounds at least as good to me. Some more ideas:

  • UnconnectedInputManager
  • NoDependencyInputManager

My other comment is here is that we are kind of retreating from notion of being able to use solids you don't control or don't want to alter upstream of a solid you are working on. I was quite fond of that when we got there but it certainly introduced confusion on other dimensions. I just want to be explicit about the tradeoff we are making and to be sure we are comfortable with it. Namely, you cannot specify input loading behavior independent of the IOManager attached to the upstream output.

I think "you cannot specify input loading behavior independent of the IOManager attached to the upstream output" is definitely accurate. I do think there is a difference between "you cannot specify input loading behavior independent of the IOManager attached to the upstream output" and "retreating from notion of being able to use solids you don't control". Solids only specify their manager keys, not their manager implementations. The pipeline definer ultimately decides how intermediates are stored/loaded.

sandyryza updated this revision to Diff 29097.Fri, Jan 8, 6:34 PM

up

sandyryza retitled this revision from RFC: root input manager to root input manager.Fri, Jan 8, 6:36 PM
sandyryza edited the summary of this revision. (Show Details)
Harbormaster failed remote builds in B23933: Diff 29097!Fri, Jan 8, 6:49 PM
sandyryza updated this revision to Diff 29099.Fri, Jan 8, 6:53 PM

up

sandyryza updated this revision to Diff 29100.Fri, Jan 8, 6:57 PM

up

Harbormaster failed remote builds in B23935: Diff 29099!Fri, Jan 8, 7:09 PM
Harbormaster failed remote builds in B23936: Diff 29100!Fri, Jan 8, 7:13 PM
sandyryza updated this revision to Diff 29123.Fri, Jan 8, 9:01 PM

up

Harbormaster completed remote builds in B23958: Diff 29123.Fri, Jan 8, 9:19 PM
alangenfeld requested changes to this revision.Fri, Jan 8, 10:45 PM

based on our earlier conversation it seems we may punt on this for release due to the complexity around:

  • type based required resource keys
  • interactions with default value loading

leading us to believe we may not have this totally dialed in yet

This revision now requires changes to proceed.Fri, Jan 8, 10:45 PM
sandyryza added a child revision: D5894: move dagster type loader under root input managers.Sat, Jan 9, 2:00 AM
sandyryza updated this revision to Diff 29251.Sun, Jan 10, 4:38 AM

up

Harbormaster completed remote builds in B24058: Diff 29251.Sun, Jan 10, 4:56 AM
alangenfeld accepted this revision.Mon, Jan 11, 10:40 PM

finallydone

sandyryza updated this revision to Diff 29366.Tue, Jan 12, 3:57 PM

up

sandyryza added a child revision: D5924: unexpose output managers.Tue, Jan 12, 4:03 PM
Harbormaster failed remote builds in B24148: Diff 29366!Tue, Jan 12, 4:17 PM
sandyryza added a child revision: D5925: InputManager -> RootInputManager.Tue, Jan 12, 4:21 PM
sandyryza updated this revision to Diff 29372.Tue, Jan 12, 4:40 PM

up

Harbormaster failed remote builds in B24153: Diff 29372!Tue, Jan 12, 4:57 PM
sandyryza updated this revision to Diff 29377.Tue, Jan 12, 5:18 PM

up

Harbormaster completed remote builds in B24155: Diff 29377.Tue, Jan 12, 5:37 PM
This revision was not accepted when it landed; it landed in state Needs Review.Tue, Jan 12, 6:03 PM
Closed by commit R1:ae09fcdb1a03: root input manager (authored by sandyryza). · Explain Why
This revision was automatically updated to reflect the committed changes.
sandyryza added a commit: R1:ae09fcdb1a03: root input manager.

Revision Contents
Changeset List

PathSize
docs/
next/
src/
pages/
overview/
io-managers/
23 lines
examples/
docs_snippets/
docs_snippets/
overview/
io_managers/
2 lines
2 lines
29 lines
python_modules/
dagster/
dagster/
core/
definitions/
23 lines
5 lines
12 lines
40 lines
execution/
plan/
32 lines
2 lines
dagster_tests/
core_tests/
storage_tests/
130 lines
4 lines
61 lines

Diff 29384

View Options

docs/next/src/pages/overview/io-managers/io-managers.mdx

Loading...
View Options

examples/docs_snippets/docs_snippets/overview/io_managers/config_input_manager.py

Loading...
View Options

examples/docs_snippets/docs_snippets/overview/io_managers/root_input_manager.py

Loading...
View Options

examples/docs_snippets/docs_snippets/overview/io_managers/subselection.py

Loading...
View Options

python_modules/dagster/dagster/core/definitions/environment_configs.py

Loading...
View Options

python_modules/dagster/dagster/core/definitions/i_solid_definition.py

Loading...
View Options

python_modules/dagster/dagster/core/definitions/input.py

Loading...
View Options

python_modules/dagster/dagster/core/definitions/pipeline.py

Loading...
View Options

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

Loading...
View Options

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

Loading...
View Options

python_modules/dagster/dagster_tests/core_tests/storage_tests/test_input_manager.py

Loading...
View Options

python_modules/dagster/dagster_tests/core_tests/storage_tests/test_io_manager.py

Loading...
View Options

python_modules/dagster/dagster_tests/core_tests/storage_tests/test_output_manager.py

Loading...