Skip to content
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

feat: Allow memoization without outputs #11379

Merged
merged 1 commit into from
Jul 25, 2023

Conversation

Joibel
Copy link
Member

@Joibel Joibel commented Jul 17, 2023

Please note: I am not totally in favour of this change, it makes memoization work without outputs and I'm not conviced this is actually healthy, but it's what people seem to expect. See the linked issues in the fixes list below.

This change will cause memoization to work for all step and dag tasks even without outputs. Note: They were working semi-erroneously for some dag tasks due to implicit outputs before this change.

Fixes #11280 (raised to cover this desire)
Fixes #10769 (already closed by documentation)
Partially addresses #10426: Dags will memoize now, but retries still won't - manually tested.

Please note: I am not totally in favour of this change, it makes
memoization work without outputs and I'm not conviced this is actually
healthy, but it's what people seem to expect. See the linked issues in
the fixes list below.

This change will cause memoization to work for all step and dag tasks
even without outputs. Note: They were working semi-erroneously for
some dag tasks due to implicit outputs before this change.

Fixes argoproj#11280 (raised to cover this desire)
Fixes argoproj#10769 (already closed by documentation)
Partially addresses argoproj#10426: Dags will memoize now, but retries still
won't

Signed-off-by: Alan Clucas <[email protected]>
@Joibel Joibel marked this pull request as ready for review July 18, 2023 10:07
Copy link
Member

@isubasinghe isubasinghe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@terrytangyuan
Copy link
Member

A bit hesitant but there seems to be more good than harm

@terrytangyuan terrytangyuan merged commit a76674c into argoproj:master Jul 25, 2023
@Joibel
Copy link
Member Author

Joibel commented Jul 25, 2023

A bit hesitant but there seems to be more good than harm.

Agreed, but thanks for merging.

Joibel added a commit to Joibel/argo-workflows that referenced this pull request Nov 3, 2023
The merge with argoproj#11451 reverted this, so this commit is just to
reinstate that.
The tests included in argoproj#11379 failed to catch this, I've raised argoproj#12129
for this, but in the interests of matching the documentation and
kubecon next week I'm putting this PR in now.

Fixes argoproj#12117

Signed-off-by: Alan Clucas <[email protected]>
@Joibel Joibel deleted the memoize-all branch November 20, 2023 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants