Stage: fix used_cache warning on no checksum #3365
Conversation
dvc/output/base.py
Outdated
| # TODO it will always call get, need to write it somewhere | ||
| checksum = self.info.get(self.remote.PARAM_CHECKSUM) | ||
| if not checksum: | ||
| checksum = self.remote.get_checksum(self.path_info) |
There was a problem hiding this comment.
This property should only return what is already computed, it shouldn't start computing it all over again.
See dvc/outpt/base.py:get_used_cache, where the issue rose. We check if not self.info there, while we should probably check if not self.checksum. Push shouldn't push uncommited changes, as it doesn't even have the cache for them in .dvc/cache.
pared
force-pushed
the
3359_md5_problem
branch
3 times, most recently
from
90cebdc
to
e4b4506
Compare
pared
changed the title
| "\n" | ||
| "You can also use `dvc commit {stage}` to associate " | ||
| "existing '{out}' with `{stage}`.".format( | ||
| out=self, stage=self.stage.relpath |
There was a problem hiding this comment.
| out=self, stage=self.stage.relpath | |
| out=self, stage=self.stage |
same as above 🙂
There was a problem hiding this comment.
AFAIK it will result in dvc commit 'Stage file.dvc' ;)
There was a problem hiding this comment.
@pared Oh, but then the upper warning is broken too, right?
There was a problem hiding this comment.
@efiop
I wouldn't say so: the upper message is pointing user to which stage particular output is associated to, while the lower one suggests command that can fix your situation. So in the upper one, its not crucial for stage to be printed just as relpath.
tests/func/test_stage.py
Outdated
| def test_warn_on_no_out_checksum(tmp_dir, dvc, tmp_path_factory, caplog): | ||
| tmp_dir.dvc_gen("file", "file content") | ||
|
|
||
| local_remote = tmp_path_factory.mktemp("local_remote") | ||
| with dvc.config.edit() as conf: | ||
| from dvc.utils import fspath | ||
|
|
||
| conf["remote"]["local_remote"] = {"url": fspath(local_remote)} | ||
| conf["core"]["remote"] = "local_remote" | ||
|
|
||
| stage_content = load_stage_file("file.dvc") | ||
| first(stage_content["outs"])["md5"] = None | ||
| dump_stage_file("file.dvc", stage_content) | ||
|
|
||
| with caplog.at_level(logging.WARNING, logger="dvc"): | ||
| dvc.push() | ||
|
|
||
| assert first(caplog.messages) == ( | ||
| "Output 'file'(Stage: 'file.dvc') is missing version info. " | ||
| "Cache for it will not be collected. " | ||
| "Use `dvc repro` to get your pipeline up to date.\n" | ||
| "You can also use `dvc commit file.dvc` to associate existing 'file' " | ||
| "with `file.dvc`." | ||
| ) |
There was a problem hiding this comment.
Maybe let's narrow it down to get_used_cache unit test for output?
pared
force-pushed
the
3359_md5_problem
branch
from
1215516
to
5a711e8
Compare
❗ Have you followed the guidelines in the Contributing to DVC list?
📖 Check this box if this PR does not require documentation updates, or if it does and you have created a separate PR in dvc.org with such updates (or at least opened an issue about it in that repo). Please link below to your PR (or issue) in the dvc.org repo.
❌ Have you checked DeepSource, CodeClimate, and other sanity checks below? We consider their findings recommendatory and don't expect everything to be addressed. Please review them carefully and fix those that actually improve code or fix bugs.
Thank you for the contribution - we'll try to review it as soon as possible. 🙏