Page MenuHomePhabricator

handle differing arg and input def ordering
ClosedPublic

Authored by alangenfeld on Jan 16 2020, 6:33 PM.

Details

Summary

In order to use the fn argument order instead of the input def order we have to pass that information along to the solid definition. Here is the simplest appraoch.

resolves #2064

Test Plan

added test

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.Jan 16 2020, 6:33 PM
alangenfeld planned changes to this revision.Jan 16 2020, 6:50 PM
alangenfeld updated this revision to Diff 8749.Jan 16 2020, 9:47 PM

special case singurlar input_def

sashank added inline comments.Jan 17 2020, 7:59 PM
python_modules/dagster/dagster/core/decorator_utils.py
63

I know this was already there, but I think this should be error type extra, and the error below should be missing_name?

71

This should be missing_name instead of extra

max added inline comments.Jan 22 2020, 9:45 PM
python_modules/dagster/dagster_tests/core_tests/definitions_tests/test_pipeline.py
274

it's hard for me to see why this should raise but test_single_non_positional_input_use should pass...

fix old code and add comments to tests

revert change to other code

alangenfeld planned changes to this revision.Jan 23 2020, 8:39 PM

better handle single non positional case

schrockn accepted this revision.Fri, Jan 24, 4:41 PM

seems good. plz heed final comments

python_modules/dagster/dagster/core/definitions/composition.py
154–167

nit: seems like this block of code should be incorporated into input_name_at_position or a different method. function getting big. single function that returns input_name with the error check

156

position

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

why is this not hard error?

71–74

one liner?

names = [inp.name for inp in self_input_defs if inp.name not in self._positional_inptus]
This revision is now accepted and ready to land.Fri, Jan 24, 4:41 PM
This revision was automatically updated to reflect the committed changes.