Skip to content

metrics/params: diff: don't show diff if active branch is the same as workspace#8716

Merged
efiop merged 4 commits into
mainfrom
metrics-diff-active-branch
Jan 9, 2023
Merged

metrics/params: diff: don't show diff if active branch is the same as workspace#8716
efiop merged 4 commits into
mainfrom
metrics-diff-active-branch

Conversation

@dberenbaum
Copy link
Copy Markdown
Contributor

Before, if you did dvc metrics diff main from the main branch, you would get:

$ dvc metrics diff main
Path                   Metric      main     workspace    Change
training/metrics.json  step        14       -            -
training/metrics.json  test.acc    0.7735   -            -
training/metrics.json  test.loss   0.95962  -            -
training/metrics.json  train.acc   0.7694   -            -
training/metrics.json  train.loss  0.9731   -            -

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.

@dberenbaum dberenbaum requested a review from a team December 20, 2022 15:53
@dberenbaum dberenbaum force-pushed the metrics-diff-active-branch branch from 843d63f to 90de4a6 Compare December 20, 2022 15:54
@codecov
Copy link
Copy Markdown

codecov Bot commented Dec 20, 2022

Codecov Report

Base: 93.55% // Head: 93.54% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (ed689da) compared to base (1d5de9c).
Patch coverage: 93.54% of modified lines in pull request are covered.

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              
Impacted Files Coverage Δ
dvc/repo/metrics/show.py 95.40% <71.42%> (+0.05%) ⬆️
dvc/repo/metrics/diff.py 100.00% <100.00%> (ø)
dvc/repo/params/diff.py 100.00% <100.00%> (ø)
dvc/repo/params/show.py 94.00% <100.00%> (+0.06%) ⬆️
tests/func/metrics/test_diff.py 100.00% <100.00%> (ø)
tests/func/params/test_diff.py 100.00% <100.00%> (ø)
tests/unit/repo/experiments/conftest.py 90.62% <0.00%> (-4.69%) ⬇️

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

Comment thread dvc/repo/metrics/diff.py Outdated
@dberenbaum dberenbaum force-pushed the metrics-diff-active-branch branch 2 times, most recently from 027763f to 065686c Compare January 5, 2023 14:19
Comment thread dvc/repo/metrics/diff.py
Comment on lines 14 to +17
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]

Copy link
Copy Markdown
Contributor

@efiop efiop Jan 9, 2023

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 https://github.com/iterative/dvc/blob/0e5674ffeabb4f911a9486da8ff3ba3a33d1d98c/dvc/repo/metrics/show.py#L145 is the 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 ...

Copy link
Copy Markdown
Contributor

@efiop efiop Jan 9, 2023

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 https://github.com/iterative/dvc/blob/0e5674ffeabb4f911a9486da8ff3ba3a33d1d98c/dvc/repo/metrics/show.py#L145 behaviour useful at all in the API? I'm kinda tempted to move that hiding logic to the CLI layer so we don't have to fix it in 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...

Copy link
Copy Markdown
Contributor

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.

@efiop efiop force-pushed the metrics-diff-active-branch branch from 9b78895 to ed689da Compare January 9, 2023 14:20
@efiop efiop changed the title Metrics diff with active branch metrics/params: diff: don't show diff if active branch is the same as workspace Jan 9, 2023
@efiop efiop requested a review from daavoo January 9, 2023 14:42
@efiop efiop merged commit b25ef5c into main Jan 9, 2023
@efiop efiop deleted the metrics-diff-active-branch branch January 9, 2023 14:57
@efiop efiop added enhancement Enhances DVC ui user interface / interaction A: params Related to dvc params A: metrics labels Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A: params Related to dvc params enhancement Enhances DVC ui user interface / interaction

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants