exp show: handle extending/renaming of metrics/params. #7264
Conversation
daavoo
changed the title
daavoo
force-pushed
the
issue-7070
branch
3 times, most recently
from
ea09084 to
27a4ce2
Compare
| all_headers, | ||
| metric_headers, | ||
| param_headers, |
There was a problem hiding this comment.
Would it be possible to just use TabularData.row_from_dict() here? The parameters are getting complicated here.
There was a problem hiding this comment.
Tried adding a change using row_from_dict but it only saves us from passing all_headers.
Unfortunately, we still need metric_headers and param_headers later in this function because of the bug being fixed.
The overall handling of metrics and params could be simplified but feels out of the scope of this fix.
There was a problem hiding this comment.
We should simplify these parameters soon though, as it is getting complicated as exp show is becoming complex.
May get more confusing in the future. Anyway, thanks for looking into it. 🙂
| row_dict["Experiment"] = exp.get("name", "") | ||
| row_dict["rev"] = name_rev | ||
| row_dict["typ"] = typ | ||
| row_dict["Created"] = _format_time( | ||
| exp.get("timestamp"), fill_value, iso | ||
| ) | ||
| row_dict["parent"] = parent | ||
| row_dict["State"] = state | ||
| row_dict["Executor"] = exp.get("executor", fill_value) |
There was a problem hiding this comment.
Now this requires us to keep headers in sync in two places. (we did that before by forcing an order anyway though).
There was a problem hiding this comment.
That's true but, as you mentioned, the sync was required before. By using dict keys, at least the sync is explicit (and prevents bug being fixed)
|
Need to rebase it. |
_extend_row did not considered partially empty `items`, which can occur when metris or params are renamed between commits. Fixes #7070
| row_dict["Experiment"] = exp.get("name", "") | ||
| row_dict["rev"] = name_rev | ||
| row_dict["typ"] = typ | ||
| row_dict["Created"] = _format_time( | ||
| exp.get("timestamp"), fill_value, iso | ||
| ) | ||
| row_dict["parent"] = parent | ||
| row_dict["State"] = state | ||
| row_dict["Executor"] = exp.get("executor", fill_value) |
There was a problem hiding this comment.
Just a nitpicky suggestion, what do you think of using row_dict.update() here? I'm okay with how it is as well though.
There was a problem hiding this comment.
Do you mean like the following?
| row_dict["Experiment"] = exp.get("name", "") | |
| row_dict["rev"] = name_rev | |
| row_dict["typ"] = typ | |
| row_dict["Created"] = _format_time( | |
| exp.get("timestamp"), fill_value, iso | |
| ) | |
| row_dict["parent"] = parent | |
| row_dict["State"] = state | |
| row_dict["Executor"] = exp.get("executor", fill_value) | |
| row_dict.update( | |
| Experiment=exp.get("name", ""), | |
| rev=name_rev, | |
| typ=typ, | |
| Created=_format_time( | |
| exp.get("timestamp"), fill_value, iso | |
| ), | |
| parent=parent, | |
| State=state, | |
| Executor=exp.get("executor", fill_value) | |
| ) |
No strong preference, is update more efficient or is there another reason?
There was a problem hiding this comment.
@daavoo, I meant it more like following, to use dict arg than as kwargs:
row_dict.update({
"Experiment": exp.get("name", ""),
"rev": name_rev,
"typ": typ,
"Created": _format_time(exp.get("timestamp"), fill_value, iso),
"parent": parent,
"State": state,
"Executor": exp.get("executor", fill_value),
...
})It's a matter of personal preference, I guess. As I said, I am fine as-is too.
3 participants
_extend_row did not consider partially empty
items, which can occur when metris or params are renamed between commits.Fixes #7070
❗ 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. 🙏