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

worktree add: preserve version metadata for unmodified files on dvc add #8595

Merged
merged 2 commits into from
Nov 21, 2022

Conversation

pmrowla
Copy link
Contributor

@pmrowla pmrowla commented Nov 21, 2022

Thank you for the contribution - we'll try to review it as soon as possible. πŸ™

Fixes #8293

@pmrowla pmrowla added A: data-management Related to dvc add/checkout/commit/move/remove A: cloud-versioning Related to cloud-versioned remotes labels Nov 21, 2022
@pmrowla pmrowla self-assigned this Nov 21, 2022
Comment on lines +118 to +120
old_out = old_outs.get(out.def_path, None)
if old_out is not None:
out.restore_fields(old_out)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Continually adding fields to these tuples was not ideal as far as maintaining it goes, is clearer/easier to update in the future by just keeping these assignments in Output

@codecov
Copy link

codecov bot commented Nov 21, 2022

Codecov Report

Base: 94.28% // Head: 94.28% // No change to project coverage πŸ‘

Coverage data is based on head (77ae4aa) compared to base (77ae4aa).
Patch has no changes to coverable lines.

❗ Current head 77ae4aa differs from pull request most recent head 228ec57. Consider uploading reports for the commit 228ec57 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #8595   +/-   ##
=======================================
  Coverage   94.28%   94.28%           
=======================================
  Files         431      431           
  Lines       32931    32931           
  Branches     4607     4607           
=======================================
  Hits        31048    31048           
  Misses       1461     1461           
  Partials      422      422           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

β˜” View full report at Codecov.
πŸ“’ Do you have feedback about the report comment? Let us know in this issue.

Comment on lines +493 to +495
if merge_versioned:
try:
old = self.reload()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This extra reload on save might be expensive, so it's tied to the merge_versioned flag for now.

The problem is that we can't do this in the same place we are doing restore_fields. restore_fields is done on repo.stage.create and is intended to set fields that may be replaced/updated afterwards (i.e. we load original annotations from a dvcfile in restore_fields and then use out.annot.update() later to replace the loaded values with any new/updated ones from the command line.

But for the version metadata merge, we cannot do it until after the new outs have been save()'d, since we need the new out md5's to be computed before we can compare them to the old ones.

@pmrowla pmrowla merged commit 2a1af0b into iterative:main Nov 21, 2022
@pmrowla pmrowla deleted the out-preserve-version-meta branch November 21, 2022 10:18
@skshetry skshetry changed the title wokrtree add: preserve version metadata for unmodified files on dvc add worktree add: preserve version metadata for unmodified files on dvc add Nov 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: cloud-versioning Related to cloud-versioned remotes A: data-management Related to dvc add/checkout/commit/move/remove
Projects
None yet
Development

Successfully merging this pull request may close these issues.

worktree add: dvc add removes valid metadata for files which are already tracked
1 participant