Page MenuHomeElementl

Make solid invocation directly call the underlying decorated fxn
ClosedPublic

Authored by cdecarolis on Jun 7 2021, 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
93–94

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
107–110

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
93–94

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
93–94

Will do that

python_modules/dagster/dagster/core/definitions/solid.py
107–110

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.Jun 9 2021, 4:10 PM

Add output type checking and validation

python_modules/dagster/dagster/core/definitions/solid_invocation.py
127–191

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
127–191

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.Jun 10 2021, 4:04 PM
python_modules/dagster/dagster/core/definitions/solid_invocation.py
127–191

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
284–286

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
107

positional_inputs should get cleaned up the same way

146–148

nit: direct

python_modules/dagster/dagster/core/definitions/solid_invocation.py
124–125

mypy

193–194

mypy

python_modules/dagster/dagster/core/execution/plan/execute_step.py
300–303

comments

python_modules/dagster/dagster_tests/core_tests/definitions_tests/test_decorators.py
192–193

😔

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

in what way exactly

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

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
79–80

rm

126–133

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.Jun 18 2021, 2:56 PM
python_modules/dagster/dagster/core/definitions/solid.py
126–133

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