Page MenuHomeElementl

DynamicOutput DSL map approach
ClosedPublic

Authored by alangenfeld on Dec 21 2020, 10:34 PM.

Details

Summary

Alternative to D5613, this demonstrates a map method for unpacking dynamic outputs in the DSL.

Test Plan

added and updated tests

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

interesting things

  • I opted away from map since I wasn't sure how to capture the return values and chaining given that a user could be trying to thread output nodes for different solid invocations. The end result is that the API feels more restrictive and you can't really make composites that have dynamic outputs as seen in [1]. This may be a nice thing for our starting position.
  • even though we mark the solid decorator as signature changing - it still sees the yield keyword, knows its a generator, and yells at you: Generator 'generator' has no 'forEach' member
python_modules/dagster/dagster_tests/core_tests/dynamic_tests/test_not_allowed.py
31–45

[1]

93–101

[1]

alangenfeld retitled this revision from RFC DynamicOutput DSL forEach approach to RFC DynamicOutput DSL map & forEach approach.Jan 5 2021, 10:04 PM
alangenfeld edited the summary of this revision. (Show Details)

If we have map, do we also need forEach?

If we have map, do we also need forEach?

don't need it but its cost is very low so i lean keep? not going to fight for it though

schrockn requested changes to this revision.Jan 7 2021, 9:36 PM

let's kill forEach

can we add some magic to help with the lint warnings?

This revision now requires changes to proceed.Jan 7 2021, 9:36 PM

lgtm. just want to know about the linting issue

This revision now requires changes to proceed.Jan 7 2021, 10:39 PM
alangenfeld retitled this revision from RFC DynamicOutput DSL map & forEach approach to RFC DynamicOutput DSL map approach.Jan 7 2021, 11:29 PM
alangenfeld edited the summary of this revision. (Show Details)
alangenfeld edited the summary of this revision. (Show Details)
alangenfeld requested review of this revision.EditedJan 7 2021, 11:46 PM

can we add some magic to help with the lint warnings?

The error is Generator 'generator' has no 'map' member so the __iter__ approach had the upside of not triggering this.

The only thing I could find that works is the pylint setting generated-members=map, but this appears to globally ignore any no-member error that is about a member prefixed with map

Thoughts?

edit: for clarification

x = 3
x.mapz() # no error
x.map() # no error
x.map_the_stuff() # no error
x.mauhhhh # error
x.mapuhhhh # no error
x.pam_map() # error
x.paz() # error

What if we added a map method to InvokedSolidOutputHandle that raises a NotImplementedError?

q mgmt.

is sandy's approach viable?

would really really like to find a way to avoid having to deal with these pylint directives

This revision now requires changes to proceed.Jan 8 2021, 12:50 PM

What if we added a map method to InvokedSolidOutputHandle that raises a NotImplementedError?

pylint sees that the function has yield in it so infers the return type to be Generator, which does not have map. Adding map to InvokedSolidOutputHandle does not remove the error.

I will spend some more time poking at pylint settings to see if i can find anything else

some fixes, still looking in to lint

python_modules/dagster/dagster/utils/linter.py
82–118

will this help our users avoid this problem, or just help us avoid it in our tests?

python_modules/dagster/dagster/utils/linter.py
82–118

just us by default, users who are using pylint can import and use this if they want

alangenfeld retitled this revision from RFC DynamicOutput DSL map approach to DynamicOutput DSL map approach.Jan 11 2021, 11:03 PM
alangenfeld added a subscriber: prha.

+ @prha for lint shenanigans

excited to see this land. I will let @prha look at the lint ish as I have little context on that

This revision is now accepted and ready to land.Jan 13 2021, 7:23 PM
python_modules/dagster/dagster/utils/linter.py
98–105

Having a hard time understanding what this is doing... This replaces the body of every solid with a call to unknown? Does this mean that we don't get any linting inside of a solid body?

Yeah, this seems fine if it's for internal use.

If we expect users to want to use this, we should add some documentation / disclaimers

python_modules/dagster/dagster/utils/linter.py
98–105

this goal is to override the *inference* engines understanding of whats happening in the solid body, so it lo longer uses that information to for example give errors about using the output of invoking a solid due to its inference about its return type. I don't believe this blanks out all linting, just the inference result, by means of this decorator @astroid.inference_tip

This revision was automatically updated to reflect the committed changes.