-
Notifications
You must be signed in to change notification settings - Fork 674
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
Provide a way to ignore portions of the signature for cache key calcu… #4324
Conversation
…lation purposes Signed-off-by: troychiu <[email protected]>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #4324 +/- ##
==========================================
+ Coverage 59.81% 59.83% +0.01%
==========================================
Files 632 632
Lines 53644 53655 +11
==========================================
+ Hits 32089 32102 +13
+ Misses 19029 19028 -1
+ Partials 2526 2525 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: troychiu <[email protected]>
var inputsAfterIgnore *core.LiteralMap | ||
if cacheIgnoreInputVars != nil { | ||
inputsAfterIgnore = &core.LiteralMap{Literals: make(map[string]*core.Literal)} | ||
for name, literal := range inputs.Literals { | ||
if slices.Contains(*cacheIgnoreInputVars, name) { | ||
continue | ||
} | ||
inputsAfterIgnore.Literals[name] = literal | ||
} | ||
} else { | ||
inputsAfterIgnore = inputs | ||
} |
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.
Rather than creating a new LiteralMap
here would it make more sense to pass the cacheIgnoreInputVars
to the HashLiteralMap
function. Then in this code here we just need to update to:
for name, literal := range literalMap.Literals {
if !slices.Contains(cacheIgnoreInputVars, name) {
hashifiedLiteralMap[name] = hashify(literal)
}
}
This achieves the same outcome in 2 LoC rather than 12. I think it makes more sense to only create a new LiteralMap in one place as well.
Closing as satisfied by #4618 |
Tracking issue
Closes #3058
Describe your changes
Check all the applicable boxes
Screenshots
Note to reviewers
The local version is here: flyteorg/flytekit#1914