Page MenuHomeElementl

Add solid as parameter to build_hook_context
ClosedPublic

Authored by cdecarolis on Thu, Jul 15, 1:27 AM.

Details

Summary

An issue was recently opened around this https://github.com/dagster-io/dagster/issues/4350, and this is a start to addressing it. We can, for now at least, create a solid from the definition.

Test Plan

added additional unit test

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Thu, Jul 15, 1:52 AM
Harbormaster failed remote builds in B33730: Diff 41648!
Harbormaster returned this revision to the author for changes because remote builds failed.Thu, Jul 15, 5:41 PM
Harbormaster failed remote builds in B33758: Diff 41682!
This revision is now accepted and ready to land.Fri, Jul 16, 12:48 AM
python_modules/dagster/dagster/core/definitions/dependency.py
96–125 ↗(On Diff #41682)

I clarify this a bit in D8715 - but the whole point of this class is binding an invocation in to a container - so its. a bit odd the container is optional

186–204 ↗(On Diff #41682)

i find this kind of stuff dubious since it makes assumptions about the outer context from within

python_modules/dagster/dagster/core/execution/context/hook.py
153–164

nit: i think when the inline if-else pile up like this it gets a bit hard to parse

159

re: above comments

I think making "hook_context_container" GraphDefinition instance here would work too

@alangenfeld how would you feel about requiring both solid and container to use the context.solid param? That way we could avoid a lot of the nastiness that results from making the container optional on Solid

I think an ephemeral graph isnt so bad - honestly could simplify how we make the Solid too

something like

@graph(name='hook_context')
def temp_graph():
   solid()

temp_graph.get_solids()[0]

Add graph container when constructing solid

python_modules/dagster/dagster/core/definitions/dependency.py
110 ↗(On Diff #41801)

rm

python_modules/dagster/dagster/core/execution/context/hook.py
161–172

I think you can replace this all with temp_graph.solids[0] (needs to get renamed to nodes but returns the Node)
maybe assert len(..) == 1