Page MenuHomeElementl

Make solid invocation directly call the underlying decorated fxn
ClosedPublic

Authored by cdecarolis on Mon, Jun 7, 8:20 PM.

Details

Summary

This diff makes direct invocation call the underlying python function instead of the compute_fn.

Test Plan

Re-adjusted tests to account for new state of the world. Some tests exist to document that we don't catch incorrect behavior.

Diff Detail

Repository
R1 dagster
Branch
solid_invocation_direct
Lint
Lint Passed
Unit
No Test Coverage

Event Timeline

its a bit complex but we *could* do output validation if we wanted

do we want to?

its a bit complex but we *could* do output validation if we wanted

do we want to?

I guess if the decorated fxn isn't a generator we can validate, and then if it is a generator, we wrap it in another generator that performs the validation?

alangenfeld added a subscriber: owen.

I guess if the decorated fxn isn't a generator we can validate, and then if it is a generator, we wrap it in another generator that performs the validation?

and if its async you wrap it an async generator that does the validation i think

@owen has D8225 which is worth being aware of

I *think* validating outputs is a behavior we want, I may have my_solid() == my_val correct in test but if my typehint or OutputDefinition is wrong the test will no longer catch that

python_modules/dagster/dagster_tests/core_tests/test_solid_invocation.py
480

is there a wrong_type test somewhere here I am missing?

python_modules/dagster/dagster/core/definitions/decorators/solid.py
77–78

maybe @owen has an opinion from working on this recently, but would it make sense to just not do this wrapping here and instead allow compute_fn to simply be the decorated_fn and move the coercion code from decorator business to runtime?

python_modules/dagster/dagster/core/definitions/solid.py
91–95

should we drop passing positional_inputs and context_arg_provided and just resolve them from the decorated_fn if we are passing it here now?

python_modules/dagster/dagster/core/definitions/decorators/solid.py
77–78

assuming the wrapped compute_fn thing isn't used in any context other than the runtime pipeline execution, I didn't see a real reason to do it beforehand.

python_modules/dagster/dagster/core/definitions/decorators/solid.py
77–78

Will do that

python_modules/dagster/dagster/core/definitions/solid.py
91–95

good point

python_modules/dagster/dagster_tests/core_tests/test_solid_invocation.py
480

Might be missing that - will add one

What's the verdict on output validation? Is that something we want to pursue? cc @alangenfeld @sandyryza

I think yes, we are still validating inputs right?

I think yes, we are still validating inputs right?

yup. will add that fxnality then.

This revision now requires changes to proceed.Wed, Jun 9, 4:10 PM

Add output type checking and validation

python_modules/dagster/dagster/core/definitions/solid_invocation.py
131–195

The code duplication here makes me uneasy... at least some of this can probably be consolidated.

welldone

python_modules/dagster/dagster/core/definitions/solid_invocation.py
131–195

the nature of dealing with the different fundamental function type is code like this - you can tidy it up but i think its inevitable

i think it feels worse that it is burdensome in practice

This revision is now accepted and ready to land.Thu, Jun 10, 4:04 PM
python_modules/dagster/dagster/core/definitions/solid_invocation.py
131–195

i think it feels worse that it is burdensome in practice

Do you mean that something about this feels burdensome in practice?

Add solid compute wrapper override flag

Remove compute wrapper override, add DecoratedSolidFunction class to determine whether solid fxn came from decorator

Rearrange implementation around flag for decorators requiring context arguments, fix tests

alangenfeld added inline comments.
python_modules/dagster/dagster/core/definitions/composition.py
286–288

nit: "directly invoked"

python_modules/dagster/dagster/core/definitions/decorators/solid.py
26

took me way too much to figure out whats up with this, add a comment or maybe make a DecoratedSolidFunction subclass DecoratedLambdaSolidFunction that overrides to False

python_modules/dagster/dagster/core/definitions/solid.py
91

positional_inputs should get cleaned up the same way

129–131

nit: direct

python_modules/dagster/dagster/core/definitions/solid_invocation.py
128–129

mypy

197–198

mypy

python_modules/dagster/dagster/core/execution/plan/execute_step.py
299–302

comments

python_modules/dagster/dagster_tests/core_tests/definitions_tests/test_decorators.py
192–193 ↗(On Diff #39567)

😔

This revision now requires changes to proceed.Tue, Jun 15, 7:25 PM
cdecarolis added inline comments.
python_modules/dagster/dagster/core/definitions/solid.py
91

in what way exactly

python_modules/dagster/dagster/core/definitions/solid.py
91

should move to a property of DecoratedSolidFunction the same way presence of the context is

Add positional inputs to the decorated function wrapper

alangenfeld added inline comments.
python_modules/dagster/dagster/core/definitions/solid.py
63–64

rm

108–115

did you look in to removing positional_inputs from NodeDefinition too? Do we actually set that property in the composite case? I thought we didn't but i could have misremembered

This revision is now accepted and ready to land.Fri, Jun 18, 2:56 PM
python_modules/dagster/dagster/core/definitions/solid.py
108–115

yea we use it for composite solids from what I can see (and also apparently pipeline def)?