One thing that I'm concerned about is communicating the failure modes of the resolution mechanism that we're doing here. It's a bit of magic, and there's a lot of ways each can fail.

Up

Removed comment, removed recorder

Added docstring, init_resources -> build_resources

up

Up

Addressing comments + propogating changes

Don't provide default values for resource initialization

Get rid of refactor artifact

Pull change into dagstermill

Up

Fixed arg on InitResourceContext

Up

Up

Refactor artifacts

Up

Split up diff

Fixed tests to account for new resource mechanism

Added ResourceConfig wrapper

Dagstermill up

Dagstermill up

Up

Up

Up

Major refactor, reuse most of core resource init path

Regarding resource initialization outside of execution; I do have a diff up moving in that direction (https://dagster.phacility.com/D6513). I do agree that it's probably better to default to requiring for now and let the dust settle on that, but my initial impression is that adding the instance would be kinda limiting. The failure cases on out of execution resource initialization are already kinda esoteric (ie my initialization requires run-specific information), and I don't feel like we fully understand that space yet.

UP

Fix run config passed in to init_resources from version resolution pathway

Got rid of InitResourceContext artifacts, accept run config instead of env config

black

Up

Up

Up

This is no longer needed.

Up

Up

Up

Added recursive output structure.

if we communicate it well in a release do you think that's enough to warrant breaking?

Up

Up

Maybe reconsider that one error message, but lgtm

Up

Up

gonna iterate on this - not requiring an execution plan or run id.

Up

In D6494#173264, @alangenfeld wrote:

execution plan and a run id in order to initialize resources

should you even require run_id and execution_plan? I would expect pipeline_run to just be an optional property

Up

In D6428#172943, @sandyryza wrote:

Ah, gotcha, that makes sense. Thoughts on simplifying this to just pass around output_dict? I'm worried that if we make this too flexible, this will end up converging towards a duplicate of IOManager.

I also think this should probably be turned on by default. If we're encouraging people to model their solids as pure logical functions, then there isn't much much point in executing unless you can get at the outputs.

In D6428#171778, @sandyryza wrote:

@cdecarolis do you mind adding a little context on the motivation for this? Are there particular situations where the reconstruct_context hack causes problems where this solution does not?

Up

Passed through callback fxn at top level instead of pure dict.

LGTM

Will take a closer look tomorrow, but the idea feels good to me. A good before-after would of course be the snowflake IO manager on internal.

Needs a little more juice to work for graphs, but the main idea is working for solids.

Thanks !

Looped in adding a property for better error state when solid/graph does not have output value.