Page MenuHomePhabricator

InputDefinition.default_value
ClosedPublic

Authored by alangenfeld on Wed, Mar 25, 9:22 PM.

Details

Summary
  • adds the ability to set default values in InputDefinition
  • parses them when doing inference
  • fixes a py2 bug due to missing __ne__ on DagsterType

addresses #2316

Test Plan

good battery of unit tests

Diff Detail

Repository
R1 dagster
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

alangenfeld created this revision.Wed, Mar 25, 9:22 PM
alangenfeld edited the summary of this revision. (Show Details)Wed, Mar 25, 9:25 PM
schrockn requested changes to this revision.Wed, Mar 25, 9:29 PM

seems good overall. need to cover some edges cases (e.g. Nothing, where the default fails the type check). probably others i'm not thinking of right now.

Then also being able to defaults via python default params when we are computing things based on func sig, *or* explicitly disallow it

This revision now requires changes to proceed.Wed, Mar 25, 9:29 PM
alangenfeld retitled this revision from [RFC] InputDefinition.default_value to InputDefinition.default_value.Fri, Mar 27, 6:46 PM
alangenfeld retitled this revision from InputDefinition.default_value to [RFC] InputDefinition.default_value.

handle more cases add more tests

max added a comment.Sun, Mar 29, 1:13 AM

How do we feel about bundling this in with a crack at https://github.com/dagster-io/dagster/issues/1087

How do we feel about bundling this in with a crack at https://github.com/dagster-io/dagster/issues/1087

hesitant - mostly since i think that breaking change should happen at a major release not minor

agree with alex. also I still need to see a proposal that is definitively better to be worth the thrash.

schrockn requested changes to this revision.Mon, Mar 30, 3:36 PM

py3 type annotations? use native default values?

This revision now requires changes to proceed.Mon, Mar 30, 3:36 PM

use native default values?

what do you mean? this *?

python_modules/dagster/dagster_tests/core_tests/definitions_tests/test_decorators.py
543โ€“549

*

schrockn added inline comments.Mon, Mar 30, 3:42 PM
python_modules/dagster/dagster_tests/core_tests/definitions_tests/test_decorators.py
549

whoops missed that my bad

homerbush

Actually one more thing: Why don't we special case default checks for the scalar types? I think that would snag an entire class of errors.

Why don't we special case default checks for the scalar types? I think that would snag an entire class of errors.

could definitely do that, wasnt sure what to call the method on dagster_type, context_free_type_check ? simple_type_check ?

alangenfeld retitled this revision from [RFC] InputDefinition.default_value to InputDefinition.default_value.Tue, Mar 31, 11:50 PM

leverage BuiltinScalarDagsterType for early errors

nate added a comment.Wed, Apr 1, 3:28 AM

chefkiss

this is super nice and clean

python_modules/dagster/dagster/utils/test/__init__.py
234

I think parameterizes is the correct spelling, no?

python_modules/dagster/dagster_tests/core_tests/definitions_tests/test_input_defaults.py
96

maybe worth some tests with composites also? and fan-in?

schrockn accepted this revision.Wed, Apr 1, 12:02 PM

redfordnod

python_modules/dagster/dagster/core/definitions/input.py
28

๐Ÿ‘๐Ÿป

python_modules/dagster/dagster_tests/core_tests/definitions_tests/test_input_defaults.py
55

Why don't you make a task for for this. This is tractable with a little work and could be a good task for someone who wants to learn the type system

This revision is now accepted and ready to land.Wed, Apr 1, 12:02 PM
alangenfeld planned changes to this revision.Wed, Apr 1, 4:41 PM

woah good call almost whiffed on testing composite mappings which need much more care

This revision is now accepted and ready to land.Wed, Apr 1, 7:26 PM
alangenfeld updated this revision to Diff 11265.Wed, Apr 1, 8:41 PM

handle empty

alangenfeld updated this revision to Diff 11268.Wed, Apr 1, 8:46 PM

handle empty

3rd times the charm'

alangenfeld edited the summary of this revision. (Show Details)Wed, Apr 1, 11:57 PM
alangenfeld edited the test plan for this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.