cleanup dvcyaml

[dberenbaum]

Fixes #611

[codecov-commenter]

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01 🎉

Comparison is base (cae5129) 89.70% compared to head (ab189a1) 89.72%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #615      +/-   ##
==========================================
+ Coverage   89.70%   89.72%   +0.01%     
==========================================
  Files          43       43              
  Lines        2983     2987       +4     
  Branches      247      248       +1     
==========================================
+ Hits         2676     2680       +4     
  Misses        268      268              
  Partials       39       39              

Impacted Files	Coverage Δ
src/dvclive/live.py	94.81% <100.00%> (+0.03%)	⬆️
tests/test_main.py	100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[daavoo]

I remember we had a discussion the first time we added the make_dvcyaml about not wanting to remove it on cleanup #366 (comment)

Do you think the concerns no longer apply (the config for sklearn is now auto-generated) and we can consider the dvclive/dvc.yaml not modifiable?

[daavoo]

Nevermind, it looks like we are already overwriting in the code, so we never preserved the existing dvcyaml 😅

[daavoo]

The code changes look good to me, and it is consistent with the behavior we already applying *#615 (comment).

My only worry is that I am not understanding the issue behind #611 and when this is a fix. I assume that this is a fix only if Live gets instantiated before params_show is called?

[dberenbaum]

@daavoo Where are we overwriting in the code? Is this PR doing anything then? 😅

[daavoo]

@daavoo Where are we overwriting in the code?

dvclive/src/dvclive/dvc.py

Lines 86 to 116 in d261760

	def make_dvcyaml(live):
	dvcyaml = {}
	if live._params:
	dvcyaml["params"] = [os.path.relpath(live.params_file, live.dir)]
	if live._metrics or live.summary:
	dvcyaml["metrics"] = [os.path.relpath(live.metrics_file, live.dir)]
	plots = []
	plots_path = Path(live.plots_dir)
	if live._metrics:
	metrics_path = (plots_path / Metric.subfolder).relative_to(live.dir).as_posix()
	metrics_config = {metrics_path: {"x": "step"}}
	plots.append(metrics_config)
	if live._images:
	images_path = (plots_path / Image.subfolder).relative_to(live.dir)
	plots.append(images_path.as_posix())
	if live._plots:
	for plot in live._plots.values():
	plot_path = plot.output_path.relative_to(live.dir)
	plots.append({plot_path.as_posix(): plot.plot_config})
	if plots:
	dvcyaml["plots"] = plots
	if live._artifacts:
	dvcyaml["artifacts"] = copy.deepcopy(live._artifacts)
	for artifact in dvcyaml["artifacts"].values():
	abs_path = os.path.abspath(artifact["path"])
	abs_dir = os.path.realpath(live.dir)
	relative_path = os.path.relpath(abs_path, abs_dir)
	artifact["path"] = Path(relative_path).as_posix()
	dump_yaml(dvcyaml, live.dvc_file)

This only happens when make_dvcyaml is called.

Is this PR doing anything then? 😅

Yes, it makes the behavior consistent with the rest of the DVCLive-generated files, where we clean up at Live instantiation

[dberenbaum]

I wonder if we need to be so particular anymore about which files to drop inside the dvclive dir or if we should just drop the whole dir. Seems just as likely that someone will end up with stale files than deleting something someone wanted to keep. AFAIR we used to drop more aggressively before #102, but maybe we are more clear now that the dvclive dir should be a separate dir managed only by dvclive.