Path not supported for some path arguments

[AlexandreKempf]

Most of the path arguments handle both str and pathlib.Path object.

It is not the case for Live's dvcyaml while it should probably be.

[AbdullahMakhdoom]

Hi @dberenbaum, I'm interested in contributing to dvclive, and I believe this issue is a great starting point. If I'm not mistaken, it appears that the Live's dvcyaml argument should also accept pathlib.Path objects. Therefore, the argument should be defined as dvcyaml: Optional[Union[str, pathlib.Path]] = "dvc.yaml" Could you confirm if my understanding is correct?

[dberenbaum]

If I'm not mistaken, it appears that the Live's dvcyaml argument should also accept pathlib.Path objects.

Correct.

Therefore, the argument should be defined as dvcyaml: Optional[Union[str, pathlib.Path]] = "dvc.yaml"

Partially correct. Fixing the type hint would be nice, but the underlying issue goes deeper. If you try an example with dvcyaml set to some pathlib.Path value, you should see where it breaks. This issue is to fix any underlying problems so that pathlib.Path values work.

[AbdullahMakhdoom]

@dberenbaum I tried passing dvcyaml arguments a pathlib.Path value, it did not break but rather silently defaulted to "dvc.yaml" as mentioned by this function.

One solution I could think of is type converting to string, like....

  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
      return "dvc.yaml"

I've manually tested the changes, and they work as expected. What do you think of this solution?

[shcheklein]

@AbdullahMakhdoom I think it's fine to fix it this way. I would also add something like:

if self._dvcyaml: raise InvalidDvcyamlError("DVC file path mush be of str or Path type")

before the return dvc.yaml

so that we don't silently return some default value

[AbdullahMakhdoom]

Alright @shcheklein. One quick question before I open a PR: Do you think it's worth adding a test case for this change, or is it too minor to require one?

[AbdullahMakhdoom]

I can parametrize this test for dvcyaml argument, something like....

@pytest.mark.parametrize("dvcyaml_path", ["dvc.yaml", Path("dvcyaml/dvc.yaml")])
def test_test_mode(tmp_dir, monkeypatch, mocked_dvc_repo, dvcyaml_path):
    monkeypatch.setenv(DVCLIVE_TEST, "true")
    live = Live("dir", dvcyaml=dvcyaml_path)
    live.make_dvcyaml()
    assert live._dir != "dir"
    assert live._dvc_file != "dvc.yaml"
    assert live._save_dvc_exp is False
    assert not os.path.exists("dir")
    assert not os.path.exists(dvcyaml_path)

Took inspiration from this sibling function, which is in the same test_dvc.py file.