-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DM-44488: Add tests for the --raise-on-partial-outputs option to pipetask. #25
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the most concerning comment of the ones I've left: we specifically added error info to the task metadata, so I'm surprised it's not getting output.
data/SConscript
Outdated
# Configure one quantum to fail with AnnotatedPartialOutputsError. | ||
# By default this is treated as a qualified success, and downstream | ||
# quanta will be run (we expect them to them do NoWorkFound, which | ||
# just reflects the fact that it's a little weird to pick ISR as | ||
# the task to raise this error, but that doesn't matter for testing | ||
# the mechanics. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's some useful clarification.
# When we do raise, the ISR quantum that raised should not have | ||
# metadata written, but it should have logs, and downstream tasks | ||
# should not have either, because they are never run. | ||
self.assertFalse(helper.butler.exists(get_mock_name("isr_metadata"), data_id, exposure=95104)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm concerned that the metadata isn't written on a failure: we add information about the failure to the task metadata, so I'd have hoped that it would get written.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately we'll break a lot of things if we start writing metadata for tasks that fail; I'd like to fix that eventually, but it's a couple of big projects down from the top of my work stack (I consider it part of the "butler provenance" work package). There may be a somewhat nearer-term possibility of getting it into the "quantum execution reports" that I believe BPS uses, but I'm not super familiar with how information flows into and out of those.
To be clear, the metadata absolutely is being written when we take the "qualified success" route.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, the task metadata on qualified success is probably good enough; AP's use case doesn't care about the outputs of failing tasks, so it shouldn't matter.
c209ef1
to
8a799e2
Compare
Checklist
doc/changes
(in ctrl_mpexec)