[caching] fix issues that execution metadata with overwrite cache is not written to artifacts table #4550
Conversation
|
Thank you for opening this pull request! 🙌 These tips will help get your PR across the finish line:
|
dosubot
bot
added
the
size:XXL
ysysys3074
changed the title
ysysys3074
force-pushed
the
yushang/overwrite-cache
branch
from
06f4228
to
15c0407
Compare
|
Thanks for the PR! Can you add DCO signoff to all commits as well? This is necessary as we are a Linux Foundation project. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4550 +/- ##
==========================================
+ Coverage 58.98% 59.06% +0.07%
==========================================
Files 621 587 -34
Lines 52483 50339 -2144
==========================================
- Hits 30957 29731 -1226
+ Misses 19059 18232 -827
+ Partials 2467 2376 -91
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Can you rebase? The |
ysysys3074
force-pushed
the
yushang/overwrite-cache
branch
from
8061c62
to
d0fb2b5
Compare
sure i have added that in my commit thanks! |
yeah i just rebase, thanks |
add metadata in updateartifact request in propeller change in catalog add test Signed-off-by: Yue Shang <[email protected]>
Signed-off-by: Yue Shang <[email protected]>
Signed-off-by: Yue Shang <[email protected]>
ysysys3074
force-pushed
the
yushang/overwrite-cache
branch
from
d0fb2b5
to
ef9a400
Compare
| metaData := &datacatalog.Metadata{ | ||
| KeyMap: map[string]string{"key2": "value2"}, | ||
| } | ||
| serializedMetadata, err := transformers.SerializedMetadata(metaData) |
There was a problem hiding this comment.
looks like there is a lint err here because err is not used. maybe add assert.Nil(...) on the err here.
| metaData := &datacatalog.Metadata{ | ||
| KeyMap: map[string]string{"key2": "value2"}, | ||
| } | ||
| serializedMetadata, err := transformers.SerializedMetadata(metaData) |
There was a problem hiding this comment.
also assert on error here.
| logger.Debugf(ctx, "ArtifactModel is %+v", artifactModel) | ||
| logger.Debugf(ctx, "ArtifactModel ArtifactData is %+v", artifactModel.ArtifactData) | ||
| logger.Debugf(ctx, "ArtifactModel SerializedMetadata is %+v", artifactModel.SerializedMetadata) |
There was a problem hiding this comment.
if we're going to leave this, can it be a single log line rather than 3? The first line should print the entire artifactModel, then the next two are printing fields that should already be exposed in the first.
|
closing in favor of #4617 |
Why are the changes needed?
When we execute workflow with overwrite cache flag and then run the execution again, "view source execution" is still linked to the old cached execution link instead of execution with overwrite cache which is confusing, so this PR is correct this behavior.
What changes were proposed in this pull request?
The root cause for this issue is new metadata from execution with overwrite cache is not passed to UpdateArtifactRequest, and artifacts table is not really updated with new serializedmetadata.
How was this patch tested?
unit tests added
E2E tests are done in Linkedin's internal flyte platform so can not share the execution link.
screeshots:
second run, i chose overwrite cache button from UI
link to first execution - fopdnzo2lxkd5a
fourth run, it picks the cache from second run, link to the right execution
link to the third execution - f14xkrarmxyiws. (it will link to first run before my code changes)
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link