preserve summary metrics on resume #756
Conversation
| def read_step(self): | ||
| if Path(self.metrics_file).exists(): | ||
| latest = self.read_latest() | ||
| return latest.get("step", 0) | ||
| return 0 | ||
| latest = self.read_latest() | ||
| return latest.get("step", 0) | ||
|
|
||
| def read_latest(self): | ||
| with open(self.metrics_file, encoding="utf-8") as fobj: | ||
| return json.load(fobj) | ||
| if Path(self.metrics_file).exists(): | ||
| with open(self.metrics_file, encoding="utf-8") as fobj: | ||
| return json.load(fobj) | ||
| return {} |
There was a problem hiding this comment.
This was just refactoring so I could use read_latest elsewhere without having to check if the summary metrics file exists.
|
|
||
| assert read_history(dvclive, "metric") == (steps, metrics) | ||
| assert read_latest(dvclive, "metric") == (steps[-1], metrics[-1]) | ||
| if resume: | ||
| assert dvclive.read_latest()["summary"] == 1 |
There was a problem hiding this comment.
Without this PR, the summary metric would have been lost
|
Test failure is unrelated (see this comment) |
| @@ -24,9 +26,14 @@ def test_resume(tmp_dir, resume, steps, metrics): | |||
| for new_metric in [0.7, 0.6]: | |||
| dvclive.log_metric("metric", new_metric) | |||
| dvclive.next_step() | |||
| dvclive.end() | |||
There was a problem hiding this comment.
QQ: is this one actually needed or it's just for a symmetry?
There was a problem hiding this comment.
Just for symmetry/best practice. I think the original year might have been from before we had and end method.
|
We have a test failure, needs to be fixed. Otherwise good catch, thanks @dberenbaum ! |
|
@shcheklein See the comment above about the test failure being unrelated. We will keep having that test failure until both of these happen:
We in fact had the test failure for a long time, but we weren't testing the frameworks in CI until recently. |
Dvclive was not preserving summary metrics when
resume=True.