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.

Sign up for GitHub

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

Jump to bottom

feat(controller): Step level memoization. Closes #944 #3356

Merged
merged 62 commits into from
Jul 22, 2020
Merged

feat(controller): Step level memoization. Closes #944 #3356

merged 62 commits into from
Jul 22, 2020

Conversation

rbreeze
Copy link
Member

@rbreeze rbreeze commented Jun 30, 2020 •
edited
Loading

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
  • The title of the PR is (a) conventional, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
  • I've signed the CLA.
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My builds are green. Try syncing with master if they are not.
  • My organization is added to USERS.md.

terrytangyuan
terrytangyuan reviewed Jun 30, 2020
View reviewed changes
pkg/apis/workflow/v1alpha1/workflow_types.go Outdated
@@ -504,6 +504,9 @@ type Template struct {

// Metrics are a list of metrics emitted from this template
Metrics *Metrics `json:"metrics,omitempty" protobuf:"bytes,35,opt,name=metrics"`

// Memoize allows templates to use outputs generated from already executed templates
Memoize *Memoize `json:"memoize,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

How do we handle the co-existence of this and argo resubmit --memoized?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think there's any overlap, since the resubmit flag is a CLI feature and the memoization option here is set in a workflow spec. The resubmit flag also only uses memoization for the same instance of a workflow, while this feature allows different workflows to share the cache.

@terrytangyuan terrytangyuan mentioned this pull request Jun 30, 2020
Closed
rbreeze
rbreeze commented Jun 30, 2020
View reviewed changes
pkg/apis/workflow/v1alpha1/workflow_types.go Outdated Show resolved Hide resolved
pkg/apis/workflow/v1alpha1/workflow_types.go Outdated Show resolved Hide resolved
workflow/controller/cache.go Outdated Show resolved Hide resolved
workflow/controller/cache.go Outdated Show resolved Hide resolved
workflow/controller/operator.go Outdated Show resolved Hide resolved
@rbreeze rbreeze requested a review from simster7 July 1, 2020 00:42
@rbreeze rbreeze requested a review from alexec July 1, 2020 15:28
alexec
alexec reviewed Jul 1, 2020
View reviewed changes
Copy link
Contributor

@alexec alexec left a comment

Choose a reason for hiding this comment

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

some first pass comments

pkg/apis/workflow/v1alpha1/workflow_types.go Outdated Show resolved Hide resolved
pkg/apis/workflow/v1alpha1/workflow_types.go Outdated Show resolved Hide resolved
workflow/controller/cache.go Outdated Show resolved Hide resolved
workflow/controller/cache.go Outdated Show resolved Hide resolved
workflow/controller/cache.go Outdated Show resolved Hide resolved
workflow/controller/operator.go Outdated Show resolved Hide resolved
workflow/controller/operator.go Outdated Show resolved Hide resolved
workflow/controller/operator.go Show resolved Hide resolved
workflow/controller/operator.go Outdated Show resolved Hide resolved
workflow/controller/operator.go Outdated Show resolved Hide resolved
simster7
simster7 suggested changes Jul 1, 2020
View reviewed changes
Copy link
Member

@simster7 simster7 left a comment

Choose a reason for hiding this comment

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

Great work! First round of comments

pkg/apis/workflow/v1alpha1/workflow_types.go Outdated Show resolved Hide resolved
pkg/apis/workflow/v1alpha1/workflow_types.go Outdated Show resolved Hide resolved
workflow/controller/cache.go Outdated Show resolved Hide resolved
workflow/controller/cache.go Outdated Show resolved Hide resolved
workflow/controller/cache.go Outdated Show resolved Hide resolved
workflow/controller/operator.go Outdated Show resolved Hide resolved
workflow/controller/operator.go Outdated Show resolved Hide resolved
workflow/controller/operator.go Show resolved Hide resolved
workflow/controller/operator.go Outdated Show resolved Hide resolved
workflow/controller/operator.go Outdated Show resolved Hide resolved
@Ark-kun
Copy link
Member

Ark-kun commented Jul 2, 2020 •
edited
Loading

What are your thoughts about #944 (comment) ?

One this it proposes is adding memoize to task/step instead of template. Same template may be used for different tasks with different parameters and caching requirements.

It also proposes by default calculating the cache_key based on the pod fields: inputs, container.{image,command,args,env, volumeMounts}, volumes.

@rbreeze
More code review fixes. Change cache mutex to use standard golang mut…
a200671 
…ex. Remove configMapName from cache struct. Fix tests
rbreeze added 18 commits July 16, 2020 10:26
@rbreeze
Merge w master and codegen
b54e9d1
@rbreeze
Move cache_test back to controller directory. Lint
4d6470b
@rbreeze
Refactor controller to be cache type agnostic. Fix tests to account f…
ee5c576 
…or all recent changes.
@rbreeze
Remove pointer from mutex
68958bf
@rbreeze
Resolve merge conflict
9a7512a
@rbreeze
Begin converting to structured logging in cache.go
cc71313
@rbreeze
Finished adding structured logging to cache.go
d53c325
@rbreeze
Fix small things in cache test
5d726ec
@rbreeze
Convert memoization related code in operator to structured logging
343e9dc
@rbreeze
Merge branch 'master' into memoization-2
9561840
@rbreeze
codegen
1a89519
@rbreeze
Merge branch 'master' into memoization-2
3cad6f8
@rbreeze
Merge branch 'master' into memoization-2
2fec94b
@rbreeze
Merge branch 'master' into memoization-2
7199cac
@rbreeze
codegen
0afb3ef
@rbreeze
Merge branch 'master' into memoization-2
421e2f7
@rbreeze
Merge branch 'master' into memoization-2
92baf28
@rbreeze
Merge branch 'master' into memoization-2
293a07d
@rbreeze rbreeze merged commit 13b1d3c into argoproj:master Jul 22, 2020
@rbreeze rbreeze deleted the memoization-2 branch July 22, 2020 22:51
@alexec
Copy link
Contributor

alexec commented Sep 2, 2020

Available for testing in v2.10.0-rc1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@terrytangyuan terrytangyuan terrytangyuan left review comments

@alexec alexec alexec left review comments

@jessesuen jessesuen jessesuen approved these changes

@simster7 simster7 simster7 approved these changes

Assignees

@jessesuen jessesuen

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@rbreeze @Ark-kun @alexec @simster7 @terrytangyuan @jessesuen