plots/metrics/params/experiments show: stop throwing exceptions #5984
Conversation
pared
force-pushed
the
unify_show_error_handling
branch
3 times, most recently
from
7650d03
to
9501cf2
Compare
|
In this change I try to provide a new way of handling errors. Providing test code for reproduction puproses: @pytest.fixture
def custom_exp_stage(tmp_dir, scm, dvc):
dump_yaml(tmp_dir / "params.yaml", {"foo": 111, "bar": 222})
dump_yaml(tmp_dir / "other.yaml", {"a": 333, "b": 999})
CODE = dedent(
"""
from ruamel.yaml import YAML
ruyaml=YAML()
with open('params.yaml', 'r') as fd:
data=ruyaml.load(fd)
with open('other.yaml', 'r') as fd:
other=ruyaml.load(fd)
def to_metric(d):
res = {}
for n,v in d.items():
res[n + "_met"] = v/10
return res
with open('out.yaml', 'w') as fd:
ruyaml.dump(to_metric(data), fd)
with open('other_out.yaml', 'w') as fd:
ruyaml.dump(to_metric(other), fd)"""
)
tmp_dir.gen("code.py", CODE)
scm.add(["params.yaml", "other.yaml", "code.py"])
scm.commit("init")
stage = dvc.run(
name="train",
params=["foo,bar", "other.yaml:a,b"],
metrics_no_cache=["out.yaml", "other_out.yaml"],
cmd="python code.py",
deps=["code.py"],
)
scm.add(
["dvc.yaml", "dvc.lock", ".gitignore", "out.yaml", "other_out.yaml"]
)
scm.commit("initial run done")
return stage
def test_show_broken(tmp_dir, scm, dvc, custom_exp_stage):
init_rev = scm.get_rev()
dvc.experiments.run(
custom_exp_stage.addressing, params=["foo=2"], name="good_experiment"
)
scm.gitpython.git.reset(init_rev, hard=True)
BREAK_METRIC_CODE = dedent(
"""
with open("out.yaml", "a") as fd:
fd.write("break the yaml!")
"""
)
with open("code.py", "a") as fd:
fd.write(BREAK_METRIC_CODE)
dvc.experiments.run(custom_exp_stage.addressing, name="break_metric")
scm.gitpython.git.reset(init_rev, hard=True)
dvc.checkout()
with open("dvc.yaml", "a") as fd:
fd.write("break the yaml")
assert main(["exp", "show", "--no-pager"]) == 0In current master ERROR: failed to show experiments - unable to read: 'dvc.yaml', YAML file structure is corrupted: while scanning a simple key
in "<unicode string>", line 17, column 1
could not find expected ':'
in "<unicode string>", line 18, column 1The result for this change: ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━┓
┃ Experiment ┃ Created ┃ other_out.yaml:a_met ┃ other_out.yaml:b_met ┃ out.yaml:foo_met ┃ out.yaml:bar_met ┃ params.yaml:foo ┃ params.yaml:bar ┃ other.yaml:a ┃ other.yaml:b ┃
┡━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━┩
│ workspace │ - │ ERR_BRANCH │ ERR_BRANCH │ ERR_BRANCH │ ERR_BRANCH │ 111 │ 222 │ ERR_BRANCH │ ERR_BRANCH │
│ master │ 06:11 PM │ 33.3 │ 99.9 │ 11.1 │ 22.2 │ 111 │ 222 │ 333 │ 999 │
│ ├── bbfde7f [good_experiment] │ 06:11 PM │ 33.3 │ 99.9 │ 0.2 │ 22.2 │ 2 │ 222 │ 333 │ 999 │
│ └── a6b1a41 [break_metric] │ 06:11 PM │ 33.3 │ 99.9 │ ERR_FILE │ ERR_FILE │ 111 │ 222 │ 333 │ 999 │
└───────────────────────────────┴──────────┴──────────────────────┴──────────────────────┴──────────────────┴──────────────────┴─────────────────┴─────────────────┴──────────────┴──────────────┘ |
|
Hi, @pared. Could you please provide me the issue tickets and other contexts when and why we started to suppress those error messages? I have been trying to understand the issue on generalizing this thing as a guideline, on why it's okay to suppress exceptions for some commands and not on others. But I am having issues understanding why we don't print errors to the terminal saying that the "view" or "table" might not be "complete" because of such and such reasons and proceed with the rendering not-so complete table and then exit with 1 error code. This is not related to this PR, but the status quo. Btw, have you looked into #5038? I feel like tolerating errors is a more general issue than the metrics/params/plots/experiments, and we need a general framework to do this. Regarding the metrics/params/plots/experiments, with #5324, these commands started fetching files from remote in case it didn't exist before. You might have noticed that remote errors are making these commands fail now as we are only catching Take an example of a recently cloned dvc repo with no cache but remote configured with credentials that are invalid. We'd have just failed before, because there's no way to proceed further. But now with this PR, we'd just be hammering the remote trying to find all the files in the remote. So, I don't feel good about catch-all here in the PR. But I also don't think catching a set of exceptions is the answer. Earlier [in a test repo I had], So, I'd prefer we spend some time on having a general framework around this and try to harmonize exceptions. Regarding UI, I'd prefer we print error messages to |
There is none, the discussion regarding this behaviour is conducted under linked issues, from first message in this PR.
This might be my reluctance to use
I guess its related to first question.
Yes, in current master state we are not able to catch them all. This change however makes handling those errors possible. The
Yes this is unfortunate, but this could be handled later. Instead of erroring on every exception, suppress all and implement special handling for the ones we know how to handle. In this case we could implement onerror that raises exception when there is authorization problem, and ignores others.
Lets start the discussion then? My main point in this change is that i believe we need a shift in how we handle errors for let's name it |
|
@skshetry What should the table look like in your mind? Should the
If we are still going to continue and possibly encounter multiple errors, how do we decide which exit code to show? @pared Is there a way to see the individual error messages? |
|
@dberenbaum yes, they are put into log on debug level. I still consider putting them into trace, but no matter what we decide, it is still possible to see them. As to exit code, it seems to me that |
I am not convinced that we need to indicate in the table itself, but I am not opposed to it.
I am not saying that we print all the logs. I feel that there should be at least a message printed in stderr indicating that the table is not complete to the user.
If we decide to fail in all cases, I'd say we return 1 if it is minor failures and 2 if it is serious (similar to |
This looks like a valid solution for this case. I guess we would need to analyze other commands whether they also need such granularity. EDIT: |
pared
force-pushed
the
unify_show_error_handling
branch
2 times, most recently
from
fc8bd38
to
90cfc5a
Compare
|
@skshetry I started a discussion regarding brancher: #6039 #6039 will deal with how to approach With the problem how we should inform user about some problems, we should probably deal here. So, during the demo i proposed 3 solutions:
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━┓
┃ Experiment ┃ Created ┃ other_out.yaml:a_met ┃ other_out.yaml:b_met ┃ out.yaml:foo_met ┃ out.yaml:bar_met ┃ params.yaml:foo ┃ params.yaml:bar ┃ other.yaml:a ┃ other.yaml:b ┃
┡━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━┩
│ workspace │ - │ ERR_BRANCH │ ERR_BRANCH │ ERR_BRANCH │ ERR_BRANCH │ 111 │ 222 │ ERR_BRANCH │ ERR_BRANCH │
│ master │ 05:36 PM │ 33.3 │ 99.9 │ 11.1 │ 22.2 │ 111 │ 222 │ 333 │ 999 │
│ ├── f36320f [break_metric] │ 05:36 PM │ 33.3 │ 99.9 │ ERR_FILE │ ERR_FILE │ 111 │ 222 │ 333 │ 999 │
│ └── 3b77c91 [good_experiment] │ 05:36 PM │ 33.3 │ 99.9 │ 0.2 │ 22.2 │ 2 │ 222 │ 333 │ 999 │
└───────────────────────────────┴──────────┴──────────────────────┴──────────────────────┴──────────────────┴──────────────────┴─────────────────┴─────────────────┴──────────────┴──────────────┘
WARNING: Loading files for some revisions failed (`workspace, f36320f4f9f65d3a856ba621274b05e602fe12d7`) Use `-v` to get more info.
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━┓
┃ Experiment ┃ Created ┃ other_out.yaml:a_met ┃ other_out.yaml:b_met ┃ out.yaml:foo_met ┃ out.yaml:bar_met ┃ params.yaml:foo ┃ params.yaml:bar ┃ other.yaml:a ┃ other.yaml:b ┃
┡━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━┩
│ workspace │ - │ ! │ ! │ ! │ ! │ 111 │ 222 │ ! │ ! │
│ master │ 05:36 PM │ 33.3 │ 99.9 │ 11.1 │ 22.2 │ 111 │ 222 │ 333 │ 999 │
│ ├── 3b77c91 [good_experiment] │ 05:36 PM │ 33.3 │ 99.9 │ 0.2 │ 22.2 │ 2 │ 222 │ 333 │ 999 │
│ └── f36320f [break_metric] │ 05:36 PM │ 33.3 │ 99.9 │ ! │ ! │ 111 │ 222 │ 333 │ 999 │
└───────────────────────────────┴──────────┴──────────────────────┴──────────────────────┴──────────────────┴──────────────────┴─────────────────┴─────────────────┴──────────────┴──────────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━┓
┃ Experiment ┃ Created ┃ other_out.yaml:a_met ┃ other_out.yaml:b_met ┃ out.yaml:foo_met ┃ out.yaml:bar_met ┃ params.yaml:foo ┃ params.yaml:bar ┃ other.yaml:a ┃ other.yaml:b ┃
┡━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━┩
│ workspace │ - │ ! │ ! │ ! │ ! │ 111 │ 222 │ ! │ ! │
│ master │ 05:36 PM │ 33.3 │ 99.9 │ 11.1 │ 22.2 │ 111 │ 222 │ 333 │ 999 │
│ ├── 3b77c91 [good_experiment] │ 05:36 PM │ 33.3 │ 99.9 │ 0.2 │ 22.2 │ 2 │ 222 │ 333 │ 999 │
│ └── f36320f [break_metric] │ 05:36 PM │ 33.3 │ 99.9 │ ! │ ! │ 111 │ 222 │ 333 │ 999 │
└───────────────────────────────┴──────────┴──────────────────────┴──────────────────────┴──────────────────┴──────────────────┴─────────────────┴─────────────────┴──────────────┴──────────────┘
! means some problem occured. Use `-v` to get more info.I think we should agree which way we will go and keep it consistient for
@skshetry @pmrowla @dberenbaum which one do you prefer? |
|
With 3, your issue with the pager is just that currently we can only paginate the table itself in @skshetry It seems like our pager handling should be a level above that, so that we can paginate any UI output at all (and not just a single table). Then for 3 we would just write the table and then the error information and it would all show up in the pager. This would also allow us to do things like writing a second (optional?) table that could potentially contain rows of commits we failed to parse and the full string error message for each row |
@pmrowla, I have been thinking about My preference would be to not show any indication on the table at all. If we must, I'd prefer And, regarding errors, I believe they should go to stderr and:
|
|
But when would we be displaying the error messages? Before showing the table or after? I like @pmrowla's idea for creating second table with errors, because it puts the errors into background - we keep the ability to show whatever works, and there is a way to show errors.
The question here is how do we differentiate what is the important error. I think this is a good idea, though this assumes we create logic reacting to different errors. But what do we do with unknown errors?
Well, if one is aware that |
pared
force-pushed
the
unify_show_error_handling
branch
8 times, most recently
from
1508b6a
to
f078c5d
Compare
dvc/command/plots.py
Outdated
| else: | ||
| from dvc.ui import ui | ||
|
|
||
| ui.warn( |
There was a problem hiding this comment.
We used to create file with no actual plots. After this change we will not create it if there are no plots.
There was a problem hiding this comment.
@pared, we haven't yet decided on colors yet, so we are using ui.error_write in other places. It does look good, so maybe that's okay, but just wanted to let you know.
pared
changed the title
|
Migth stil need to reiterate on type checks, though the implementation should be finished. Some remarks:
|
|
@pared good stuff! For the sake of the VS Code team, could you please summarize how does it affect |
|
@shcheklein In current state there is no change in |
pared
force-pushed
the
unify_show_error_handling
branch
from
07b0865
to
325c160
Compare
|
@pared I guess will handle that while updating dvc. |
tests/unit/test_compare.py
Outdated
| metrics={ | ||
| "branch_1": { | ||
| "data": { | ||
| "metrics.json": {"data": {"b": {"ad": 1, "bc": 2}, "c": 4}} | ||
| } | ||
| }, | ||
| "branch_2": { | ||
| "data": { | ||
| "metrics.json": {"data": {"a": 1, "b": {"ad": 3, "bc": 4}}} | ||
| } | ||
| }, | ||
| }, | ||
| all_branches=True, | ||
| ) | ||
| out, _ = capsys.readouterr() | ||
| assert out == textwrap.dedent( | ||
| """\ | ||
| Path diff new old | ||
| x.b - 6 5 | ||
| a.d.e 1 4 3 | ||
| a.b.c 1 2 1 | ||
| Revision Path a b.ad b.bc c | ||
| branch_1 metrics.json - 1 2 4 | ||
| branch_2 metrics.json 1 3 4 - | ||
| """ | ||
| ) | ||
|
|
||
|
|
||
| def test_metrics_show_md(capsys): | ||
| show_metrics( | ||
| { | ||
| "metrics.yaml": { | ||
| "x.b": {"old": 5, "new": 6}, | ||
| "a.d.e": {"old": 3, "new": 4, "diff": 1}, | ||
| "a.b.c": {"old": 1, "new": 2, "diff": 1}, | ||
| } | ||
| metrics={ | ||
| "branch_1": { | ||
| "data": { | ||
| "metrics.json": {"data": {"b": {"ad": 1, "bc": 2}, "c": 4}} | ||
| } | ||
| }, | ||
| "branch_2": { | ||
| "data": { | ||
| "metrics.json": {"data": {"a": 1, "b": {"ad": 3, "bc": 4}}} | ||
| } | ||
| }, | ||
| }, | ||
| all_branches=True, | ||
| markdown=True, | ||
| ) | ||
| out, _ = capsys.readouterr() | ||
| assert out == textwrap.dedent( | ||
| """\ | ||
| | Path | diff | new | old | | ||
| |--------|--------|-------|-------| | ||
| | x.b | - | 6 | 5 | | ||
| | a.d.e | 1 | 4 | 3 | | ||
| | a.b.c | 1 | 2 | 1 | | ||
| | Revision | Path | a | b.ad | b.bc | c | | ||
| |------------|--------------|-----|--------|--------|-----| | ||
| | branch_1 | metrics.json | - | 1 | 2 | 4 | | ||
| | branch_2 | metrics.json | 1 | 3 | 4 | - | |
There was a problem hiding this comment.
Do we need a test to check that they don't trip over new field "error"? Maybe, try sprinkling them on a few tests?
| # When `metrics` contains a `None` key, it means that some files | ||
| # specified as `targets` in `repo.metrics.show` didn't contain any | ||
| # metrics. | ||
| missing = metrics.pop(None, None) | ||
| if missing: | ||
| raise BadMetricError(missing) | ||
|
|
There was a problem hiding this comment.
So, we don't raise any error now?
There was a problem hiding this comment.
Yes, I had 2 reasons for removing that:
- I was unable to retrigger this behavior.
- New approach assumes we do not throw exceptions but rather gather them. Lack of metrics will be represented as empty files content.
There was a problem hiding this comment.
Should/can we at least provide a warning or something that some metrics are missing?
|
The change is on the master now. |
❗ I have followed the Contributing to DVC checklist.
📖 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. 🙏
Related: #5525 #5667 #5845