Create Github Action workflow for reporting DVC image diffs #1104

weiji14 · 2021-03-22T23:40:17Z

Description of proposed changes

To make reviewing new baseline test images (*.png) easier, this workflow checks what images have been added or modified in a Pull Request. The changes are published in a table and as a series of images by a bot-generated GitHub comment.

References:

Addresses a painpoint of #963

Reminders

Run make format and make check to make sure the code follows the style guide.
Add tests for new features or tests that would have caught the bug that you're fixing.
Add new public functions/methods/classes to doc/api/index.rst.
Write detailed docstrings for all functions/methods.
If adding new functionality, add an example to docstrings or tutorials.

Slash Commands

You can write slash commands (/command) in the first line of a comment to perform
specific operations. Supported slash commands are:

/format: automatically format and lint the code
/test-gmt-dev: run full tests on the latest GMT development version

To make reviewing new baseline test images (*.png) easier. this workflow checks what images have been added or modified in a Pull Request. The changes are published in a table and as a series of images by a bot-generated GitHub comment.

weiji14 · 2021-03-23T01:49:33Z

.github/workflows/dvc-diff.yml

+        cat report.md
+
+    - name: Pull image data from cloud storage
+      run: dvc pull --remote upstream


Currently this line can only pull from the upstream dvc remote at https://dagshub.com/GenericMappingTools/pygmt. Will need to think about how to make it work for forks as well.

Do we need to care about forks, if people follow the contributing guides (i.e., asking for permission on DAGsHub)? If people fork the DAGsHub repository and upload DVC images to their fork, our Tests workflow will fail, because it can't download the DVC images, right?

I think we can deal with fork on the second iteration of this dvc-diff-action workflow. Technically we should be able to do dvc pull from DAGsHub forks, but that'll be quite a bit of work to code up as we need to point to another DVC remote (and also not sure if this is the best way to do things).

I believe very few users will work with DAGsHub forks, so not a big issue for us.

Ok, will kick the can down the road then.

weiji14 · 2021-03-23T01:54:25Z

.github/workflows/dvc-diff.yml

+on:
+  pull_request:
+    paths:
+      - 'pygmt/tests/baseline/*.png.dvc'


Is there a way to get this workflow to run only on commits where *.png.dvc files have changed? The dvc-diff workflow still appears to run on commits where no *.png.dvc files have changed, e.g. at e14674a.

.github/workflows/dvc-diff.yml

seisman · 2021-03-23T03:22:44Z

.github/workflows/dvc-diff.yml

+        cat report.md
+
+    - name: Pull image data from cloud storage
+      run: dvc pull --remote upstream


Do we need to care about forks, if people follow the contributing guides (i.e., asking for permission on DAGsHub)? If people fork the DAGsHub repository and upload DVC images to their fork, our Tests workflow will fail, because it can't download the DVC images, right?

seisman · 2021-03-23T03:24:01Z

.github/workflows/dvc-diff.yml

+        # Get just the filename of the changed image from the report
+        tail --lines=+3 report.md | cut --delimiter=' ' --fields=7 > diff_files.txt
+
+        # Append each image to the markdown report
+        echo -e "## Image diff(s)\n" >> report.md
+        while IFS= read -r line; do
+          cml-publish --md "$line" >> report.md
+        done < diff_files.txt
+
+        # Send diff report as GitHub comment
+        cml-send-comment report.md


Do these commands show both the old and new images, or only the new image?

Currently only the new image. It should be possible to get the old image as well, but we don't really have many old images on dvc/DAGsHub yet.

Co-authored-by: Dongdong Tian <[email protected]>

seisman · 2021-03-23T03:38:26Z

I don't like the new workflow because:

It makes a new comment for each commit, so the length of the PR page will increase quickly.
It comments on both commits and PRs, so we will get two notifications for every commit.

.github/workflows/dvc-diff.yml

Co-Authored-By: Dongdong Tian <[email protected]>

.github/workflows/dvc-diff.yml

seisman · 2021-03-26T01:43:42Z

.github/workflows/dvc-diff.yml

+        cat report.md
+
+    - name: Pull image data from cloud storage
+      run: dvc pull --remote upstream


I believe very few users will work with DAGsHub forks, so not a big issue for us.

Co-authored-by: Dongdong Tian <[email protected]>

.github/workflows/dvc-diff.yml

Co-Authored-By: Dongdong Tian <[email protected]>

.github/workflows/dvc-diff.yml

seisman · 2021-03-26T03:00:01Z

.github/workflows/dvc-diff.yml

+        # Get just the filename of the changed image from the report
+        tail --lines=+7 report.md | cut --delimiter=' ' --fields=7 > diff_files.txt


The command doesn't work for deleted and modified files.

| Status | Path | |----------|-------------------------------------------------------------| | added | pygmt/tests/baseline/test_basemap.png | | deleted | pygmt/tests/baseline/test_solar_terminators.png | | modified | pygmt/tests/baseline/test_logo_on_a_map.png |

I ran the following command to the above "report.md" file:

tail --lines=+3 report.md | cut --delimiter=' ' --fields=7 > diff_files.txt

it gives me:

pygmt/tests/baseline/test_basemap.png

It would be better if you can delete a dvc file and update a dvc file in this PR, so that we can know the workflow works for added, deleted and modified images.

tail --lines=+3 report.md | awk '{print $4}'

This command gives me the expected output:

pygmt/tests/baseline/test_basemap.png pygmt/tests/baseline/test_solar_terminators.png pygmt/tests/baseline/test_logo_on_a_map.png

NOTE: you need to change +3 to +7.

Thanks for the awk command suggestion, really need to pick up more bash scripting skills!

It would be better if you can delete a dvc file and update a dvc file in this PR, so that we can know the workflow works for added, deleted and modified images.

Probably won't work for deleted images since there's nothing to report. But I'm pretty sure added and modified images will work. I'd prefer to test this in a separate PR so that:

We keep this PR small and focused. Modifying too many test images will result in a bigger diff to review.

People can start using the dvc-diff action in their PRs. At this point in time, most of the changes will be adding new images to dvc so we don't need to worry about deleting/modifying images yet.

Sounds good to me.

Co-Authored-By: Dongdong Tian <[email protected]>

seisman · 2021-03-26T16:42:50Z

pygmt/tests/test_legend.py

@@ -44,42 +43,25 @@ def test_legend_default_position():
    return fig


-@check_figures_equal()
+@pytest.mark.mpl_image_compare
 def test_legend_entries():


Rewrite the tests using a simpler 1d array, rather than the @Table_5_11.txt file?

I agree, but best to not modify the test too much in this PR as mentioned in #1104 (comment)

MAINTENANCE.md

.github/workflows/dvc-diff.yml

Co-authored-by: Dongdong Tian <[email protected]>

seisman · 2021-03-29T04:01:24Z

Only the first image is shown (See #1096 (comment)).

…appingTools#1104) To make reviewing new baseline test images (*.png) easier. the dvc-diff workflow checks what images have been added or modified in a Pull Request. The changes are published in a table and as a series of images by a bot-generated GitHub comment. * Refactor test_legend_entries to use mpl_image_compare * Let actions/checkout fetch all history so that dvc diff works * Use peter-evans/create-or-update-comment to publish image diff report * Add bullet points with names for each of the images that have changed * Collapsible image diff section and use correct git SHA in the report * List dvc-diff.yml under MAINTENANCE.md * Use awk 'NR>=7' instead of tail and add some whitespace indentation Co-authored-by: Dongdong Tian <[email protected]>

weiji14 added the maintenance Boring but important stuff for the core devs label Mar 22, 2021

weiji14 added this to the 0.4.0 milestone Mar 22, 2021

Refactor test_legend_entries to use mpl_image_compare

03d95d5

vercel bot temporarily deployed to Preview March 22, 2021 23:54 Inactive

Let actions/checkout fetch all history so that git diff works

e14674a

vercel bot temporarily deployed to Preview March 23, 2021 00:45 Inactive

Cut by using whitespace instead of pipe

a314617

vercel bot temporarily deployed to Preview March 23, 2021 01:00 Inactive

Ensure png image is viewable and add markdown header before image diff

b4c1da5

weiji14 force-pushed the dvc-diff-action branch from 7bf8be6 to b4c1da5 Compare March 23, 2021 01:08

vercel bot temporarily deployed to Preview March 23, 2021 01:15 Inactive

weiji14 commented Mar 23, 2021

View reviewed changes

seisman reviewed Mar 23, 2021

View reviewed changes

Use specific versions of setup-dvc and setup-cml

ee10108

Co-authored-by: Dongdong Tian <[email protected]>

vercel bot temporarily deployed to Preview March 23, 2021 03:26 Inactive

weiji14 self-assigned this Mar 24, 2021

seisman reviewed Mar 24, 2021

View reviewed changes

.github/workflows/dvc-diff.yml Outdated Show resolved Hide resolved

Merge branch 'master' into dvc-diff-action

3061b80

vercel bot temporarily deployed to Preview March 25, 2021 23:51 Inactive

Use peter-evans/create-or-update-comment to publish image diff report

1db5328

Co-Authored-By: Dongdong Tian <[email protected]>

weiji14 force-pushed the dvc-diff-action branch from 8b32f3a to 1db5328 Compare March 25, 2021 23:55

This comment has been minimized.

Sign in to view

vercel bot temporarily deployed to Preview March 25, 2021 23:59 Inactive

vercel bot temporarily deployed to Preview March 26, 2021 00:06 Inactive

weiji14 force-pushed the dvc-diff-action branch 2 times, most recently from 5c5e66a to 45b2a96 Compare March 26, 2021 00:14

vercel bot temporarily deployed to Preview March 26, 2021 00:15 Inactive

Make GitHub comment more descriptive and easier to regex match

bb6d5ff

Add bullet points with names for each of the images that have changed

de763c9

vercel bot temporarily deployed to Preview March 26, 2021 00:50 Inactive

weiji14 changed the title ~~WIP: Create Github Action workflow for reporting DVC image diffs~~ Create Github Action workflow for reporting DVC image diffs Mar 26, 2021

seisman reviewed Mar 26, 2021

View reviewed changes

seisman self-assigned this Mar 26, 2021

Remove cml-send-comment lines

d482a97

Co-authored-by: Dongdong Tian <[email protected]>

vercel bot temporarily deployed to Preview March 26, 2021 01:54 Inactive

Mention GitHub SHA in the report

6fa103f

vercel bot temporarily deployed to Preview March 26, 2021 02:05 Inactive

seisman reviewed Mar 26, 2021

View reviewed changes

.github/workflows/dvc-diff.yml Outdated Show resolved Hide resolved

vercel bot temporarily deployed to Preview March 26, 2021 02:29 Inactive

Collapsible image diff section and use correct git SHA

554ba6d

Co-Authored-By: Dongdong Tian <[email protected]>

weiji14 force-pushed the dvc-diff-action branch from f906e89 to 554ba6d Compare March 26, 2021 02:32

vercel bot temporarily deployed to Preview March 26, 2021 02:36 Inactive

seisman reviewed Mar 26, 2021

View reviewed changes

weiji14 and others added 2 commits March 26, 2021 20:50

Use awk instead of cut to handle multiple whitespace better

3b890a8

Co-Authored-By: Dongdong Tian <[email protected]>

List dvc-diff.yml under MAINTENANCE.md

6ea495e

vercel bot temporarily deployed to Preview March 26, 2021 07:57 Inactive

seisman reviewed Mar 26, 2021

View reviewed changes

Use awk 'NR>=7' instead of tail and add some whitespace indentation

85df20a

Co-authored-by: Dongdong Tian <[email protected]>

vercel bot temporarily deployed to Preview March 29, 2021 01:41 Inactive

seisman approved these changes Mar 29, 2021

View reviewed changes

weiji14 marked this pull request as ready for review March 29, 2021 03:16

Merge branch 'master' into dvc-diff-action

024c54e

vercel bot temporarily deployed to Preview March 29, 2021 03:16 Inactive

weiji14 merged commit 8445639 into master Mar 29, 2021

weiji14 deleted the dvc-diff-action branch March 29, 2021 03:35

seisman mentioned this pull request Mar 29, 2021

Fix dvc-diff workflow for multiple changes #1144

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create Github Action workflow for reporting DVC image diffs #1104

Create Github Action workflow for reporting DVC image diffs #1104

weiji14 commented Mar 22, 2021 •

edited

Loading

weiji14 Mar 23, 2021

seisman Mar 23, 2021

weiji14 Mar 26, 2021

seisman Mar 26, 2021

weiji14 Mar 26, 2021

weiji14 Mar 23, 2021

seisman Mar 23, 2021

seisman Mar 23, 2021

weiji14 Mar 23, 2021

seisman commented Mar 23, 2021

This comment has been minimized.

seisman Mar 26, 2021

seisman Mar 26, 2021

seisman Mar 26, 2021

seisman Mar 26, 2021

weiji14 Mar 26, 2021

seisman Mar 26, 2021

seisman Mar 26, 2021

weiji14 Mar 29, 2021

seisman commented Mar 29, 2021

		# Get just the filename of the changed image from the report
		tail --lines=+7 report.md \| cut --delimiter=' ' --fields=7 > diff_files.txt

Create Github Action workflow for reporting DVC image diffs #1104

Create Github Action workflow for reporting DVC image diffs #1104

Conversation

weiji14 commented Mar 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seisman commented Mar 23, 2021

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seisman commented Mar 29, 2021

weiji14 commented Mar 22, 2021 •

edited

Loading