Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Sep 19, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ba66faa
refactor: Update `Live` class to handle `pathlib.Path` object for `dv…
AbdullahMakhdoom Sep 8, 2024
838d9d9
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Sep 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 17 additions & 8 deletions src/dvclive/live.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: Union[str, os.PathLike, bool, None] = "dvc.yaml",
cache_images: bool = False,
exp_name: Optional[str] = None,
exp_message: Optional[str] = None,
Expand All @@ -104,11 +104,11 @@ def __init__(
part of `Live.end()`. Defaults to `True`. If you are using DVCLive
inside a DVC Pipeline and running with `dvc exp run`, the option will be
ignored.
dvcyaml (str | None): where to write dvc.yaml file, which adds DVC
dvcyaml (str | Path | None): where to write dvc.yaml file, which adds DVC
configuration for metrics, plots, and parameters as part of
`Live.next_step()` and `Live.end()`. If `None`, no dvc.yaml file is
written. Defaults to `"dvc.yaml"`. See `Live.make_dvcyaml()`.
If a string like `"subdir/dvc.yaml"`, DVCLive will write the
If a string or Path like `"subdir/dvc.yaml"`, DVCLive will write the
configuration to that path (file must be named "dvc.yaml").
If `False`, DVCLive will not write to "dvc.yaml" (useful if you are
tracking DVCLive metrics, plots, and parameters independently and
Expand Down Expand Up @@ -265,11 +265,19 @@ def _init_dvc(self): # noqa: C901
self._include_untracked.append(self.dir)

def _init_dvc_file(self) -> str:
if isinstance(self._dvcyaml, str):
if os.path.basename(self._dvcyaml) == "dvc.yaml":
return self._dvcyaml
raise InvalidDvcyamlError
return "dvc.yaml"
if self._dvcyaml is None:
return "dvc.yaml"
if isinstance(self._dvcyaml, bool):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AbdullahMakhdoom could you please point me to that test?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shcheklein, you can read the docstring for dvcyaml.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return "dvc.yaml"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@AbdullahMakhdoom AbdullahMakhdoom Sep 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


self._dvcyaml = os.fspath(self._dvcyaml)
if (
isinstance(self._dvcyaml, str)
and os.path.basename(self._dvcyaml) == "dvc.yaml"
):
return self._dvcyaml

raise InvalidDvcyamlError

def _init_dvc_pipeline(self):
if os.getenv(env.DVC_EXP_BASELINE_REV, None):
Expand Down Expand Up @@ -334,6 +342,7 @@ def _init_test(self):
"""
with tempfile.TemporaryDirectory() as dirpath:
self._dir = os.path.join(dirpath, self._dir)
self._dvcyaml = os.fspath(self._dvcyaml)
if isinstance(self._dvcyaml, str):
self._dvc_file = os.path.join(dirpath, self._dvcyaml)
self._save_dvc_exp = False
Expand Down
3 changes: 2 additions & 1 deletion tests/test_make_dvcyaml.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import pytest
from PIL import Image
from pathlib import Path

from dvclive import Live
from dvclive.dvc import make_dvcyaml
Expand Down Expand Up @@ -423,7 +424,7 @@ def test_warn_on_dvcyaml_output_overlap(tmp_dir, mocker, mocked_dvc_repo, dvcyam

@pytest.mark.parametrize(
"dvcyaml",
[True, False, "dvc.yaml"],
[True, False, "dvc.yaml", Path("dvc.yaml")],
)
def test_make_dvcyaml(tmp_dir, mocked_dvc_repo, dvcyaml):
dvclive = Live("logs", dvcyaml=dvcyaml)
Expand Down