Alternative to D5613, this demonstrates a map method for unpacking dynamic outputs in the DSL.
Details
Details
added and updated tests
Diff Detail
Diff Detail
- Repository
- R1 dagster
- Lint
Lint Passed - Unit
No Test Coverage
Event Timeline
Comment Actions
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–46 | [1] | |
| 99–106 | [1] | |
Comment Actions
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
Comment Actions
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
Comment Actions
What if we added a map method to InvokedSolidOutputHandle that raises a NotImplementedError?
Comment Actions
q mgmt.
is sandy's approach viable?
would really really like to find a way to avoid having to deal with these pylint directives
Comment Actions
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
| python_modules/dagster/dagster/utils/linter.py | ||
|---|---|---|
| 82–118 ↗ | (On Diff #29175) | 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 ↗ | (On Diff #29175) | just us by default, users who are using pylint can import and use this if they want |
Comment Actions
excited to see this land. I will let @prha look at the lint ish as I have little context on that
| python_modules/dagster/dagster/utils/linter.py | ||
|---|---|---|
| 98–105 ↗ | (On Diff #29334) | 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? |
Comment Actions
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 ↗ | (On Diff #29334) | 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 |