Page MenuHomePhabricator

warn on None returned for conditional output
ClosedPublic

Authored by alangenfeld on Wed, Nov 18, 10:35 PM.

Details

Summary

Pretty easy pit to fall in to when trying out conditional outputs for the first time - warn to guide users in the right direction.

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

python_modules/dagster/dagster/core/definitions/decorators/solid.py
245–254

we could also interpret None here as a desire to skip if the output is not required and log an info/warn to point at yield Output(None) if they wanted to pass None downstream

This seems reasonable/helpful to me. My gut is that this is better than the alternative you suggested of skipping on None. I can definitely imagine that there are some edge cases where someone wants to be able to pass a None through an optional input. And generally seems less difficult to debug something not executing when you expect it to than executing when you don't expect it to.

I haven't thought through all the edge cases though, so someone else might have more perspective.

This revision is now accepted and ready to land.Thu, Nov 19, 10:33 PM