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

stage.save: Remove merge_versioned option. #8873

Conversation

daavoo
Copy link
Contributor

@daavoo daavoo commented Jan 24, 2023

If we want add, commit and move to work for cloud versioning, the option must be always True so it is the same as not having it.

Closes #8828
(We were not passing merge_versioned=True in dvc commit)


After the first push:

$ mkdir data
$ echo bar > data/bar
$ echo foo > data/foo
$ dvc add data
$ dvc push -v

Tracking local updates:

$ echo bar2 > data/bar

Works as expected with either dvc add data or dvc commit:

$ dvc add data
$ cat data.dvc
outs:
- path: data
  files:
  - size: 5
    md5: 042967ff088f7f436015fbebdcad1f28
    relpath: bar
  - size: 4
    version_id: Vnu6l9_ECdLGrp6B5iK6YmQTm87UaDzP
    etag: d3b07384d113edec49eaa6238ad5ff00
    md5: d3b07384d113edec49eaa6238ad5ff00
    relpath: foo
$ dvc commit
$ cat data.dvc
outs:
- path: data
  files:
  - size: 5
    md5: 042967ff088f7f436015fbebdcad1f28
    relpath: bar
  - size: 4
    version_id: Vnu6l9_ECdLGrp6B5iK6YmQTm87UaDzP
    etag: d3b07384d113edec49eaa6238ad5ff00
    md5: d3b07384d113edec49eaa6238ad5ff00
    relpath: foo

@daavoo daavoo requested review from a team, karajan1001 and dberenbaum and removed request for a team January 24, 2023 17:48
@codecov
Copy link

codecov bot commented Jan 24, 2023

Codecov Report

Base: 93.72% // Head: 93.72% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (e7451ea) compared to base (8bda365).
Patch coverage: 95.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #8873   +/-   ##
=======================================
  Coverage   93.72%   93.72%           
=======================================
  Files         453      453           
  Lines       36146    36154    +8     
  Branches     5239     5238    -1     
=======================================
+ Hits        33877    33885    +8     
  Misses       1761     1761           
  Partials      508      508           
Impacted Files Coverage Δ
dvc/repo/worktree.py 10.25% <0.00%> (ø)
dvc/repo/add.py 100.00% <100.00%> (ø)
dvc/stage/__init__.py 94.69% <100.00%> (-0.02%) ⬇️
tests/func/test_commit.py 100.00% <100.00%> (ø)

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.

Copy link
Contributor

@dberenbaum dberenbaum left a comment

Choose a reason for hiding this comment

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

I will leave QA up to you and reviewers, but LGTM otherwise

@pmrowla
Copy link
Contributor

pmrowla commented Jan 25, 2023

linking my comment from the old PR for future reference

We can probably just remove the flag then. This issue is more a side effect of our current add/commit/move behavior, where we always create a new stage from scratch, and then load the previous version if it exists later and try to merge the two, (as opposed to loading the stage if it exists first and then modifying it)

Originally posted by @pmrowla in #8829 (comment)

@daavoo daavoo enabled auto-merge (rebase) January 25, 2023 07:45
If we want `add`, `commit` and `move` to work for cloud versioning, option must be always True so it is the same as not having it.

Closes #8828
(We were not passing `merge_versioned=True` in `dvc commit`)
@daavoo daavoo force-pushed the 8828-cloud-versioning-using-dvc-commit-to-track-updates-doesnt-work-as-expected branch from ad83346 to e7451ea Compare January 25, 2023 07:45
@daavoo daavoo merged commit 80633f4 into main Jan 25, 2023
@daavoo daavoo deleted the 8828-cloud-versioning-using-dvc-commit-to-track-updates-doesnt-work-as-expected branch January 25, 2023 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cloud versioning: Using dvc commit to track updates doesn't work as expected
3 participants