Page MenuHomeElementl

allow partial InputDefinition list
ClosedPublic

Authored by alangenfeld on Fri, Apr 16, 11:00 PM.

Details

Summary

Our current behavior is that if input_defs is provided, we use that otherwise we infer input_defs from the type signature.

The proposed behavior in this diff is that we infer the InputDefinition for all arguments in the type signature that do not have an entry in input_defs

The main benefit here is that if a user needs to add a property to one input, they no longer need to provide InputDefinition for all, they can just add the one.

Test Plan

added test, will add some more if we like this

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.Fri, Apr 16, 11:18 PM
Harbormaster failed remote builds in B29031: Diff 35630!

One reason I like the named "InputProps" is that it felt additive, rather than the single source of truth for definitions. My concern here is that input defs would no longer be a single source of truth. The current state of things is definitely annoying so this is not a veto or anything. Just raising the concern.

Overall, I think this is a positive change. I've personally run into this edge before, and it is frustrating to have to put such a long definition in for an input that has no special properties, simply because a different parameter does.

The error out behavior that we have in status quo doesn't really protect the user from anything imo. If someone removes an input definition in the input_defs list and forgets to remove it from the function signature (probably one of the more common reasons why someone would encounter this error), they'll still get an error, just from the pipeline definition (unsatisfied input, technically it'll be a DagsterInvalidConfigError). You could argue that the error you get in that case is less descriptive than the one we currently have, but I think the ergonomics are more important here.

I still think it would be nice to have an even more slimmed-down option, but this definitely feels like a positive move, independent of work in that realm.

My concern here is that input defs would no longer be a single source of truth

yea i think thats the clearest con / cost here. Its not clear to me this "impurity" would be perceptible by users though, but it will effect those working on the system (or those who dig in to understand more deeply how it works)

I still think it would be nice to have an even more slimmed-down option

Is there a version that you can think of that feels worth the user education / breaking change cycle / 2 ways of doing things? The InputProps version didn't feel quite good enough in my opinion

per-offline discussion agree with decision that InputProps or similar doesn't clear the cost-benefit bar right now

I am in favor of this change

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

Why not nullable? Was passing [None] previously supported? Would it break compatibility to stop supporting it?

317

Nit, feel free to ignore: is including "checked" in this function name important?

324

Short docstring could be helpful to explain what this is for.

python_modules/dagster/dagster_tests/core_tests/definitions_tests/test_solid_io.py
8

assert that the solid def ends up with two inputs?

roger other inlines - will get this cleaned up

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

the difference between opt_list and opt_nullable_list is whether its cast to [] or left as None

we used the version that left as None to distinguish between explicitly no inputs and not-set

317

it does validation is all that communicates is the idea

alangenfeld retitled this revision from [RFC] allow partial InputDefinition list to allow partial InputDefinition list.Tue, Apr 20, 10:32 PM
This revision is now accepted and ready to land.Wed, Apr 21, 3:50 PM
This revision was automatically updated to reflect the committed changes.