Support `pathlib.Path` values for `dvcyaml` argument in `Live()`. #842

AbdullahMakhdoom · 2024-09-09T10:30:05Z

…cyaml` argument.

❗ I have followed the Contributing to DVCLive guide.
📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

skshetry · 2024-09-09T10:38:50Z

src/dvclive/live.py

@@ -82,7 +82,7 @@ def __init__(
        resume: bool = False,
        report: Literal["md", "notebook", "html", None] = None,
        save_dvc_exp: bool = True,
-        dvcyaml: Optional[str] = "dvc.yaml",
+        dvcyaml: Optional[str, Path] = "dvc.yaml",


Suggested change

dvcyaml: Optional[str, Path] = "dvc.yaml",

dvcyaml: Optional[str, os.Pathlike[str]] = "dvc.yaml",

Use os.PathLike[str], which is more generic than Path.

Hi @skshetry , thanks for reviewing this.
So this PR fixes this issue, for dvcyaml argument to support path.Pathlib value. Will the use of os.PathLike[str] be more appropriate here instead of pathlib.Path?

@AbdullahMakhdoom Yes, I think the suggestion from @skshetry is just a best practice for typing hinting paths. For example, a PurePath will fail against pathlib.Path but not against os.PathLike[str]. Everything else in the PR LGTM.

AbdullahMakhdoom · 2024-09-09T10:46:38Z

Fixes Issue#769

dberenbaum · 2024-09-09T17:36:32Z

src/dvclive/live.py

@@ -265,10 +265,14 @@ def _init_dvc(self):  # noqa: C901
            self._include_untracked.append(self.dir)

    def _init_dvc_file(self) -> str:
+        if isinstance(self._dvcyaml, Path):
+            self._dvcyaml = str(self._dvcyaml)
        if isinstance(self._dvcyaml, str):
            if os.path.basename(self._dvcyaml) == "dvc.yaml":
                return self._dvcyaml
            raise InvalidDvcyamlError


Can we drop this line if we are adding the exception below?

Yep, makes sense.

dberenbaum · 2024-09-09T17:44:25Z

tests/test_dvc.py

@@ -220,12 +221,13 @@ def test_get_exp_name_duplicate(tmp_dir, mocked_dvc_repo, mocker, caplog):
    assert msg in caplog.text


-def test_test_mode(tmp_dir, monkeypatch, mocked_dvc_repo):
+@pytest.mark.parametrize("dvcyaml_path", ["dvc.yaml", Path("dvcyaml/dvc.yaml")])


Sorry for not getting back to you on this earlier, but I would vote to put this in a separate test in tests/test_make_dvcyaml.py. It's probably enough to add a parameter here.

Yeah, this makes sense.

dberenbaum · 2024-09-11T20:47:05Z

Thanks @AbdullahMakhdoom!

shcheklein · 2024-09-11T21:44:09Z

@AbdullahMakhdoom it seems mypy failed, could you please take a look. Thanks!

codecov-commenter · 2024-09-11T21:59:49Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.19%. Comparing base (602c053) to head (838d9d9).
Report is 33 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #842      +/-   ##
==========================================
- Coverage   95.47%   95.19%   -0.29%     
==========================================
  Files          57       55       -2     
  Lines        3889     3909      +20     
  Branches      353      350       -3     
==========================================
+ Hits         3713     3721       +8     
- Misses        124      138      +14     
+ Partials       52       50       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

shcheklein · 2024-09-11T22:49:00Z

src/dvclive/live.py

-        return "dvc.yaml"
+        if self._dvcyaml is None:
+            return "dvc.yaml"
+        if isinstance(self._dvcyaml, bool):


hmm, so is it something new? do we want to support bool as well?

we need to update docs then?

what happens if it's False (sounds like dvc.yaml should not be produced at all then?)

On True/False/None, it defaults to "dvc.yaml".
I had to make this change to pass an already written test case, which testsTrue, False values to dvcyaml argument.

@AbdullahMakhdoom could you please point me to that test?

@shcheklein, you can read the docstring for dvcyaml.

okay, so:

If `False`, DVCLive will not write to "dvc.yaml" (useful if you are tracking DVCLive metrics, plots, and parameters independently and want to avoid duplication).

it means we can't default to dvc.yaml - we should probably not running this function at all then if it's False.

@shcheklein The code is maybe a bit of a mess here, but this sets self._dvcfile, which shouldn't matter for whether the dvc.yaml file is written. The code still checks self._dvcyaml to decide whether to write dvc.yaml here. This is tested here:

dvclive/tests/test_make_dvcyaml.py

Lines 461 to 463 in 6888462

def test_make_dvcyaml_false(tmp_dir, mocker):

dvclive = Live("logs", dvcyaml=False)

dvclive.end()

It shouldn't matter much if self._dvcfile is set in this scenario, but the slight justification for still setting it to dvc.yaml is that if someone has set Live(dvcyaml=False) but then manually calls make_dvcyaml(), it will still write to dvc.yaml.

It's unclear why using False is necessary here, as setting dvcyaml to None appears to produce the same outcome. Both tests ensure that the dvc.yaml file is not created when dvcyaml is either False or None, indicating that these options are functionally equivalent in this context.

def test_make_dvcyaml_false(tmp_dir, mocker): dvclive = Live("logs", dvcyaml=False) dvclive.end() assert not os.path.exists("dvc.yaml") def test_make_dvcyaml_none(tmp_dir, mocker): dvclive = Live("logs", dvcyaml=None) dvclive.end() assert not os.path.exists("dvc.yaml")

Also, apologize for the delayed responses, I've been quite busy with my day job. Please let me know the best way to move forward, and I'll make sure to do my best.

No problem @AbdullahMakhdoom, thanks for keeping on top of this!

dvcyaml used to be a bool argument and support for bools was left to avoid making a breaking change. We should probably have dropped this during the last major release but missed it.

src/dvclive/live.py

…cyaml` argument.

for more information, see https://pre-commit.ci

shcheklein · 2024-09-12T21:11:33Z

@skshetry please approve and merge if everything looks good to you.

skshetry · 2024-09-17T14:54:16Z

Apologies for multiple approval. GH bugged out. :(

skshetry · 2024-09-17T14:56:09Z

src/dvclive/live.py

+        if self._dvcyaml is None:
+            return "dvc.yaml"
+        if isinstance(self._dvcyaml, bool):
+            return "dvc.yaml"


Are we okay with returning dvc.yaml in case of dvcyaml=False?

But this is unrelated to this PR. We were already doing that.

@skshetry, as pointed out by @dberenbaum, "the slight justification for still setting it to dvc.yaml is that if someone has set Live(dvcyaml=False) but then manually calls make_dvcyaml(), it will still write to dvc.yaml. "dvc.yaml" is being returned ".
Also, there was a test case for backward compatability of accepting bool values of dvc_yaml argument.

AbdullahMakhdoom force-pushed the issue-769-arg-path-support branch from a2153eb to 61a9fdc Compare September 9, 2024 10:37

skshetry reviewed Sep 9, 2024

View reviewed changes

dberenbaum reviewed Sep 9, 2024

View reviewed changes

AbdullahMakhdoom force-pushed the issue-769-arg-path-support branch from 73d300f to a96e739 Compare September 9, 2024 22:17

AbdullahMakhdoom requested review from skshetry and dberenbaum September 9, 2024 22:25

AbdullahMakhdoom force-pushed the issue-769-arg-path-support branch from a96e739 to f18c308 Compare September 11, 2024 09:30

shcheklein approved these changes Sep 11, 2024

View reviewed changes

dberenbaum approved these changes Sep 11, 2024

View reviewed changes

AbdullahMakhdoom force-pushed the issue-769-arg-path-support branch from f18c308 to 66daf24 Compare September 11, 2024 21:53

AbdullahMakhdoom requested a review from shcheklein September 11, 2024 22:06

shcheklein reviewed Sep 11, 2024

View reviewed changes

skshetry reviewed Sep 12, 2024

View reviewed changes

src/dvclive/live.py Outdated Show resolved Hide resolved

refactor: Update Live class to handle pathlib.Path object for `dv…

ba66faa

…cyaml` argument.

AbdullahMakhdoom force-pushed the issue-769-arg-path-support branch from 4f0f16b to ba66faa Compare September 12, 2024 18:27

[pre-commit.ci] auto fixes from pre-commit.com hooks

838d9d9

for more information, see https://pre-commit.ci

AbdullahMakhdoom requested a review from skshetry September 12, 2024 18:39

skshetry approved these changes Sep 17, 2024

View reviewed changes

skshetry reviewed Sep 17, 2024

View reviewed changes

skshetry merged commit f69d198 into iterative:main Sep 19, 2024
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support `pathlib.Path` values for `dvcyaml` argument in `Live()`. #842

Support `pathlib.Path` values for `dvcyaml` argument in `Live()`. #842

AbdullahMakhdoom commented Sep 9, 2024

skshetry Sep 9, 2024

AbdullahMakhdoom Sep 9, 2024 •

edited

Loading

dberenbaum Sep 10, 2024

AbdullahMakhdoom commented Sep 9, 2024

dberenbaum Sep 9, 2024

AbdullahMakhdoom Sep 9, 2024

dberenbaum Sep 9, 2024

AbdullahMakhdoom Sep 9, 2024

dberenbaum commented Sep 11, 2024

shcheklein commented Sep 11, 2024

codecov-commenter commented Sep 11, 2024 •

edited

Loading

shcheklein Sep 11, 2024

AbdullahMakhdoom Sep 11, 2024

shcheklein Sep 11, 2024

skshetry Sep 12, 2024

shcheklein Sep 12, 2024

dberenbaum Sep 12, 2024

AbdullahMakhdoom Sep 12, 2024

AbdullahMakhdoom Sep 12, 2024

dberenbaum Sep 12, 2024

shcheklein commented Sep 12, 2024

skshetry commented Sep 17, 2024

skshetry Sep 17, 2024

skshetry Sep 17, 2024

AbdullahMakhdoom Sep 19, 2024 •

edited

Loading

	dvcyaml: Optional[str, Path] = "dvc.yaml",
	dvcyaml: Optional[str, os.Pathlike[str]] = "dvc.yaml",

	def test_make_dvcyaml_false(tmp_dir, mocker):
	dvclive = Live("logs", dvcyaml=False)
	dvclive.end()

Support pathlib.Path values for dvcyaml argument in Live(). #842

Support pathlib.Path values for dvcyaml argument in Live(). #842

Conversation

AbdullahMakhdoom commented Sep 9, 2024

Choose a reason for hiding this comment

AbdullahMakhdoom Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AbdullahMakhdoom commented Sep 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dberenbaum commented Sep 11, 2024

shcheklein commented Sep 11, 2024

codecov-commenter commented Sep 11, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shcheklein commented Sep 12, 2024

skshetry commented Sep 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AbdullahMakhdoom Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

Support `pathlib.Path` values for `dvcyaml` argument in `Live()`. #842

Support `pathlib.Path` values for `dvcyaml` argument in `Live()`. #842

AbdullahMakhdoom Sep 9, 2024 •

edited

Loading

codecov-commenter commented Sep 11, 2024 •

edited

Loading

AbdullahMakhdoom Sep 19, 2024 •

edited

Loading