hail icon indicating copy to clipboard operation
hail copied to clipboard

[batch] Add always_copy_output option

Open jigold opened this issue 2 years ago • 6 comments

Thoughts on introducing this "breaking" change? I think it's okay and we will announce the change on Zulip. I think the benefits of getting rid of this not so intuitive behavior where we copy files despite the main container's completion state outweighs possibly breaking someone's pipeline that relied on files being there when the copy step could have failed as well.

jigold avatar Jun 02 '22 15:06 jigold

I think the proposed new default and the option to change it is much more intuitive than the current behavior and worth a change. Though, I think it would be most polite to announce it on zulip/email list a week or two in advance of the release (which you may already planned to do).

daniel-goldstein avatar Jun 02 '22 20:06 daniel-goldstein

Another thing I just thought of, I think we should add raise some form of error if a job that is always_run has inputs from a job that is not always_copy_output. I can't imagine something like that being intentional, but if there is a valid use case, maybe we have a warning instead of an error.

daniel-goldstein avatar Jun 22 '22 09:06 daniel-goldstein

I'm not sure how I feel about the warning / error suggestion when always run jobs have inputs that aren't always copied out. Cleanup jobs shouldn't care whether the outputs don't get copied out on failure.

jigold avatar Jun 24 '22 18:06 jigold

Do cleanup jobs take input files from the parent job? IIUC, if I have an always_run job that takes an input from a job that failed, and whose outputs were not written, the input container for the always_run job would fail.

daniel-goldstein avatar Jun 24 '22 23:06 daniel-goldstein

Hopefully the new warning gets at the case you were thinking of as well as a more general problem beyond always_copy_output

jigold avatar Jun 29 '22 21:06 jigold

Let's leave the WIP tag on, but this should be all set.

jigold avatar Jul 08 '22 18:07 jigold

We cannot take off WIP and merge this until November 1st.

jigold avatar Oct 27 '22 16:10 jigold

Agreed I think this properly preserves behavior for old clients

daniel-goldstein avatar Nov 04 '22 13:11 daniel-goldstein

@danking This is assigned to me but since you're the last person to leave changes_requested it seems appropriate that you re-review.

daniel-goldstein avatar Nov 09 '22 16:11 daniel-goldstein