-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
metrics/params: diff: don't show diff if active branch is the same as workspace #8716
Conversation
843d63f
to
90de4a6
Compare
Codecov ReportBase: 93.55% // Head: 93.54% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #8716 +/- ##
==========================================
- Coverage 93.55% 93.54% -0.01%
==========================================
Files 457 457
Lines 36237 36254 +17
Branches 5260 5262 +2
==========================================
+ Hits 33900 33914 +14
- Misses 1832 1835 +3
Partials 505 505
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
027763f
to
065686c
Compare
dvc/repo/metrics/diff.py
Outdated
metrics = repo.metrics.show(*args, **kwargs, revs=[a_rev, b_rev]) | ||
|
||
# workspace may have been replaced by active branch | ||
workspace_rev = (a_rev == "workspace") or (b_rev == "workspace") | ||
if workspace_rev and "workspace" not in metrics: | ||
active_branch = repo.scm.active_branch() | ||
if active_branch in metrics: | ||
metrics["workspace"] = metrics[active_branch] | ||
|
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.
Does seem pretty weird that show
doesn't return it at all. Looks like
Line 145 in 0e5674f
# Hide workspace metrics if they are the same as in the active branch |
metrics show
-specific behaviour that leaked into metrics diff
. Probably needs a flag not to hide workspace metrics or it should hide it in CLI (that would be my expectation). Taking another look at #3025 ...
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.
@dberenbaum @daavoo Guys, do you find this
Line 145 in 0e5674f
# Hide workspace metrics if they are the same as in the active branch |
diff
at all. If having that in API is useful, then I'll make it an opt-out flag, so again we don't have to fix it in diff
. I guess the latter is the simplest one anyway...
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.
Added a flag for now.
9b78895
to
ed689da
Compare
Before, if you did
dvc metrics diff main
from themain
branch, you would get:This made it look like the metrics were missing in the workspace and there was some difference from
main
even in a clean repo state.After this PR, there will be no output from the command above.