-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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.
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
Show csv format of experiments #6468
Changes from 8 commits
d7193bc
f881475
bbbf15c
8db41af
d315c32
5253c4c
b3680b1
4d692bb
3195d45
825352b
063345f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -9,6 +9,7 @@ | |||
from dvc.main import main | ||||
from dvc.repo.experiments.base import EXPS_STASH, ExpRefInfo | ||||
from dvc.repo.experiments.executor.base import BaseExecutor, ExecutorInfo | ||||
from dvc.repo.experiments.utils import exp_refs_by_rev | ||||
from dvc.utils.fs import makedirs | ||||
from dvc.utils.serialize import YAMLFileCorruptedError, dump_yaml | ||||
from tests.func.test_repro_multistage import COPY_SCRIPT | ||||
|
@@ -269,6 +270,10 @@ def test_show_filter( | |||
included, | ||||
excluded, | ||||
): | ||||
from contextlib import contextmanager | ||||
karajan1001 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
|
||||
from dvc.ui import ui | ||||
|
||||
capsys.readouterr() | ||||
div = "β" if os.name == "nt" else "β" | ||||
|
||||
|
@@ -312,13 +317,30 @@ def test_show_filter( | |||
if e_params is not None: | ||||
command.append(f"--exclude-params={e_params}") | ||||
|
||||
assert main(command) == 0 | ||||
@contextmanager | ||||
def console_with(console, width): | ||||
console_options = console.options | ||||
original = console_options.max_width | ||||
con_width = console._width | ||||
|
||||
try: | ||||
console_options.max_width = width | ||||
console._width = width | ||||
yield | ||||
finally: | ||||
console_options.max_width = original | ||||
console._width = con_width | ||||
|
||||
with console_with(ui.rich_console, 255): | ||||
assert main(command) == 0 | ||||
cap = capsys.readouterr() | ||||
|
||||
for i in included: | ||||
assert f"{div} {i} {div}" in cap.out | ||||
assert f"{div} params.yaml:{i} {div}" in cap.out | ||||
assert f"{div} metrics.yaml:{i} {div}" in cap.out | ||||
for e in excluded: | ||||
assert f"{div} {e} {div}" not in cap.out | ||||
assert f"{div} params.yaml:{e} {div}" not in cap.out | ||||
assert f"{div} metrics.yaml:{e} {div}" not in cap.out | ||||
|
||||
|
||||
def test_show_multiple_commits(tmp_dir, scm, dvc, exp_stage): | ||||
|
@@ -412,7 +434,6 @@ def test_show_running_checkpoint( | |||
): | ||||
from dvc.repo.experiments.base import EXEC_BRANCH | ||||
from dvc.repo.experiments.executor.local import TempDirExecutor | ||||
from dvc.repo.experiments.utils import exp_refs_by_rev | ||||
|
||||
baseline_rev = scm.get_rev() | ||||
dvc.experiments.run( | ||||
|
@@ -471,3 +492,47 @@ def test_show_with_broken_repo(tmp_dir, scm, dvc, exp_stage, caplog): | |||
|
||||
paths = ["workspace", "baseline", "error"] | ||||
assert isinstance(get_in(result, paths), YAMLFileCorruptedError) | ||||
|
||||
|
||||
def test_show_csv(tmp_dir, scm, dvc, exp_stage, capsys): | ||||
karajan1001 marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need this test? Can this test be replaced with a mocked test that checks if Eg:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, In my previous version, what I planned is that we only test |
||||
baseline_rev = scm.get_rev() | ||||
|
||||
def _get_rev_isotimestamp(rev): | ||||
return datetime.fromtimestamp( | ||||
scm.gitpython.repo.rev_parse(rev).committed_date | ||||
).isoformat() | ||||
|
||||
result1 = dvc.experiments.run(exp_stage.addressing, params=["foo=2"]) | ||||
rev1 = first(result1) | ||||
ref_info1 = first(exp_refs_by_rev(scm, rev1)) | ||||
result2 = dvc.experiments.run(exp_stage.addressing, params=["foo=3"]) | ||||
rev2 = first(result2) | ||||
ref_info2 = first(exp_refs_by_rev(scm, rev2)) | ||||
|
||||
capsys.readouterr() | ||||
assert main(["exp", "show", "--show-csv"]) == 0 | ||||
cap = capsys.readouterr() | ||||
print(cap.out) | ||||
karajan1001 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
assert ( | ||||
"Experiment,rev,typ,Created,parent,metrics.yaml:foo,params.yaml:foo" | ||||
in cap.out | ||||
) | ||||
assert ",workspace,baseline,,,3,3" in cap.out | ||||
assert ( | ||||
"master,{},baseline,{},,1,1".format( | ||||
baseline_rev[:7], _get_rev_isotimestamp(baseline_rev) | ||||
) | ||||
in cap.out | ||||
) | ||||
assert ( | ||||
"{},{},branch_base,{},,2,2".format( | ||||
ref_info1.name, rev1[:7], _get_rev_isotimestamp(rev1) | ||||
) | ||||
in cap.out | ||||
) | ||||
assert ( | ||||
"{},{},branch_commit,{},,3,3".format( | ||||
ref_info2.name, rev2[:7], _get_rev_isotimestamp(rev2) | ||||
) | ||||
in cap.out | ||||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for #5989
The duplicated column name
foo
in the test stage will cause a wrong output in test.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@karajan1001, let's do the same for other headers that we have (like
Experiments
,rev
, etc.) to reduce chances of collision. You can hoist the headers from the following and reuse them here:dvc/dvc/command/experiments.py
Lines 341 to 349 in b3680b1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, should we rename the column
typ
toType
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, the lowercase names, should we capitalize it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for the revs and parent I think we should capitalize them, but for the user-defined ones, capitalization might cause confusion to the users, and make them hard to manage in code ( It is easy to capitalize a string but hard to recover it )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@karajan1001, I was only talking about that particular list: {typ, rev, parent}.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@skshetry
But one problem here, "rev" is consisitent with the some other functions for example in
dvc/repo/plots/template.py
,./dvc/scm/git/__init__.py
and./dvc/api.py
while
typ
andparent
are only used here. So I will only modifytyp
andparent
here, still imperfect.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But they are not part of a UI, they are mostly part of an API or a schema where that makes sense. Please check
dvc metrics show --all-commits
for example.