https://dagster.io/ logo
Powered by
k

Kaushik Visvanathan

07/22/2020, 7:02 PM
Hello! We have a composite solid containing multiple solids. These solids all return one optional output (
Outputdefinition(Optional[float], is_required=False
) based on a condition. As i understand it - if the condition fails, all downstream solids dependent on this solids input are skipped and the pipeline runs to completion. The issue arises when calling the 
output_values
on the enclosing composite's execution result - it seems to throw an error complaining that output does not exist for some of these conditional solids, even if the outputs are marked as optional. Is this expected behavior? The outputs of the composite are marked as optional too
a

alex

07/22/2020, 7:03 PM
sounds likely to be a bug in the implementation of 
output_values
- can you share some more detail such as the full error output
k

Kaushik Visvanathan

07/22/2020, 8:01 PM
Yes, we have some wrappers in play so here's a mock example as close as I can give to our original:
Copy code
@solid(
    output_defs=[
        OutputDefinition(Optional[float], name="foo_output", is_required=False),
    ]
)
def foo_solid(
    context,
    condition,
):
    if condition:
        yield Output(1.0, 'foo_output')

@composite_solid(
    output_defs=[
        OutputDefinition(Optional[float], name="foo_output", is_required=False),
    ]
)
def foo_composite():
    foo = foo_solid()
    return {"foo_output": foo}
Copy code
result = execute_solid(foo_composite) # simplified to exclude mode and env dict which we have built

result.output_values # error is here
Exception :
Copy code
dagster/core/execution/results.py in output_values(self)

    256         return {
    257             output_name: self.output_value(output_name)
--> 258             for output_name in self.solid.definition.output_dict
    259         }
    260 

dagster/core/execution/results.py in <dictcomp>(.0)
    256         return {
    257             output_name: self.output_value(output_name)
--> 258             for output_name in self.solid.definition.output_dict
    259         }
    260 

dagster/core/execution/results.py in output_value(self, output_name)
    283             self.solid.definition.solid_named(output_mapping.solid_name),
    284             '.'.join([self.handle, output_mapping.solid_name]),
--> 285         ).output_value(output_mapping.output_name)
    286 
    287 

dagster/core/execution/results.py in output_value(self, output_name)
    496                     'Did not find result {output_name} in solid {self.solid.name} '
    497                     'execution result'
--> 498                 ).format(output_name=output_name, self=self)
    499             )
    500         else:

DagsterInvariantViolationError: Did not find result foo_output in solid foo_solid execution result
To follow up on this I think there's a missing check within the 
SolidExecutionResult
class's 
output_value
method to see if a given output is required or not. Is this something I could make a PR for? Would that be the fastest way to resolve this issue?
a

alex

07/23/2020, 6:45 PM
PRs welcome but I have a few minutes so let me see if i can take care of it quick
should go out in the release today
k

Kaushik Visvanathan

07/23/2020, 7:21 PM
Awesome, thank you!
Hello, just following up on this since it may not have fully addressed our bug: In line 217 of the execution file, shown below:
Copy code
@property
    def output_values(self):
        values = {}

        for output_name in self.solid.definition.output_dict:
            output_mapping = self.solid.definition.get_output_mapping(output_name)

            inner_solid_values = self._result_for_handle(
                self.solid.definition.solid_named(output_mapping.solid_name),
                SolidHandle(output_mapping.solid_name, None),
            ).output_values

            if output_mapping.output_name in inner_solid_values: # This line
                values[output_name] = inner_solid_values[output_mapping.output_name]
Shouldn't there be a check to make sure 
inner_solid_values
is not 
None
before it starts iterating?
Otherwise we get:
Copy code
dagster/core/execution/results.py in output_values(self)
    215             ).output_values
    216 
--> 217             if output_mapping.output_name in inner_solid_values:
    218                 values[output_name] = inner_solid_values[output_mapping.output_name]
    219 
TypeError: argument of type 'NoneType' is not iterable
Not sure if this touches anything else downstream also
j

Jamie

07/28/2020, 7:41 PM
to add some more info, this is a unit test we have that fails with the NoneType error
Copy code
@solid(
        output_defs=[
            OutputDefinition(Int, name='result', is_required=False),
        ]
    )
    def optional_run(context, should_run: Bool):
        if should_run:
            yield Output(1, 'result')
    @solid(
        output_defs=[
            OutputDefinition(Bool, name='result', is_required=False),
        ]
    )
    def dependent_solid(context, x: Int):
        yield Output(x == 1, 'result')
    @composite_solid(
        output_defs=[
            OutputDefinition(Bool, name='result', is_required=False)
        ]
    )
    def optional_outputs_composite(should_run: Bool):
        res = dependent_solid(optional_run(should_run))
        return {
            'result': res
        }
result = execute_solid(optional_outputs_composite(False))
result.output_values
a

alex

07/28/2020, 7:44 PM
Jamie are you on 
0.8.9
?
j

Jamie

07/28/2020, 7:45 PM
yes, I had the error Kaushik mentioned prior to 0.8.9 and then got the NoneType error after upgrading to 0.8.9
a

alex

07/28/2020, 7:49 PM
oh i missed those other new messages above
ah shoot, ya this is my bad
should have fix in this weeks release
thankyou 1
Powered by