search

iterative / dvclive

📈 Log and track ML metrics, parameters, models with Git and/or DVC
https://dvc.org/doc/dvclive
Apache License 2.0
161 stars 35 forks source link

Path not supported for some path arguments #769

Open AlexandreKempf opened 7 months ago

AlexandreKempf commented 7 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 2 hours ago

@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?