fix(task): if finish task output is None, it should not be processed #155
Conversation
draco/core/task.py
Outdated
| @@ -364,6 +369,10 @@ def finish(self): | |||
|
|
|||
| def _process_output(self, output, input_tag=None): | |||
|
|
|||
| # when output is None, there is nothing to write or check | |||
| if output is None: | |||
| return None | |||
There was a problem hiding this comment.
I think something more dramatic should happen here. This routine should only ever be called with a valid memh5 container, and so it's probably more appropriate to raise a specific exception than silently return None. Probably this test should be something more specific like not isinstance(output, memh5.MemDiskGroup) (I am not entirely sure that's the appropriate class to test for).
The same is true of _nan_process_output.
There was a problem hiding this comment.
I used memh5.MemDiskGroup and memh5.MemGroup, but got this exception with the tests:
if not isinstance(output, memh5.MemDiskGroup) or not isinstance(
output, memh5.MemGroup
):
raise pipeline.PipelineRuntimeError(
> f"Task must output a valid memh5 container; given {type(output)}"
)
E caput.pipeline.PipelineRuntimeError: Task must output a valid memh5 container; given <class 'draco.core.containers.SiderealStream'>
Is the test realistically depicting the standard pipeline by returning a SiderealStream container?
There was a problem hiding this comment.
Is the test realistically depicting the standard pipeline by returning a
SiderealStreamcontainer?
Yes.
The bug is in the boolean logic. You almost certainly want not (isinstance(output, memh5.MemDiskGroup) or isinstance(output, memh5.MemGroup) or the more succinct not isinstance(output, (memh5.MemDiskgroup, memh5.MemGroup)).
What you've written is equivalent to not (isinstance(output, MemDiskgroup) and isinstance(output, MemGroup))
There was a problem hiding this comment.
Man, you are totally right, what was I doing, I was thinking in English.
There was a problem hiding this comment.
TBH you probably don't want to check for MemGroup at least within the branch that deals with saving. Ultimately it calls output.save(...) (within BasicContMixin.write_output) and that only works on subclasses of MemDiskGroup , but will fail if given a MemGroup as it doesn't guarantee a save method exists.
anjakefala
force-pushed
the
handle-None
branch
3 times, most recently
from
e753419
to
5938eb8
Compare
- fixes bug where pipeline exceptions, if a task returns a None as a result to `.finish`
result to
.finishI saw at least a few places where the pipeline would exception, if output was a
None. In another part oftask.py, theoutputcheck is done before it is passed to_process_output. I repeated that forfinish(), but it also did not make sense to me for_process_outputto generally exception on aNone. So I added the check there too.A
Noneshould be returned in these cases, because then_check_task_outputdoes the right thing: