-
Notifications
You must be signed in to change notification settings - Fork 220
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
Create Github Action workflow for reporting DVC image diffs #1104
Changes from 12 commits
6d8fb52
03d95d5
e14674a
a314617
b4c1da5
ee10108
3061b80
1db5328
bb6d5ff
6963611
8fa0afe
de763c9
d482a97
6fa103f
554ba6d
3b890a8
6ea495e
85df20a
024c54e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,91 @@ | ||
# Checks for image diffs in a Pull Request and adds a GitHub comment showing the diff | ||
name: DVC image diff | ||
|
||
on: | ||
pull_request: | ||
paths: | ||
- 'pygmt/tests/baseline/*.png.dvc' | ||
|
||
jobs: | ||
dvc-diff: | ||
name: DVC image diff | ||
runs-on: ubuntu-latest | ||
|
||
steps: | ||
- name: Checkout | ||
uses: actions/[email protected] | ||
with: | ||
# fetch all history so that git diff works | ||
weiji14 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
fetch-depth: 0 | ||
|
||
- name: Setup data version control (DVC) | ||
uses: iterative/[email protected] | ||
|
||
- name: Setup continuous machine learning (CML) | ||
uses: iterative/[email protected] | ||
|
||
# Produce the markdown diff report, which should look like: | ||
# ## Summary of changed images | ||
# | ||
# This is an auto-generated report of images that have changed on the DVC remote | ||
# | ||
# | Status | Path | | ||
# |----------|-------------------------------------| | ||
# | added | pygmt/tests/baseline/test_image.png | | ||
- name: Put list of images that were added or changed into report | ||
run: | | ||
echo -e "## Summary of changed images\n" > report.md | ||
echo -e "This is an auto-generated report of images that have changed on the DVC remote\n" >> report.md | ||
dvc diff --show-md master HEAD >> report.md | ||
weiji14 marked this conversation as resolved.
Show resolved
Hide resolved
seisman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
cat report.md | ||
|
||
- name: Pull image data from cloud storage | ||
run: dvc pull --remote upstream | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can deal with fork on the second iteration of this dvc-diff-action workflow. Technically we should be able to do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe very few users will work with DAGsHub forks, so not a big issue for us. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, will kick the can down the road then. |
||
|
||
- name: Put image diff(s) into report | ||
env: | ||
repo_token: ${{ secrets.GITHUB_TOKEN }} | ||
seisman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
id: image-diff | ||
run: | | ||
# Get just the filename of the changed image from the report | ||
tail --lines=+7 report.md | cut --delimiter=' ' --fields=7 > diff_files.txt | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The command doesn't work for deleted and modified files.
I ran the following command to the above "report.md" file:
it gives me:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This command gives me the expected output:
NOTE: you need to change There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the
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:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good to me. |
||
|
||
# Append each image to the markdown report | ||
echo -e "## Image diff(s)\n" >> report.md | ||
while IFS= read -r line; do | ||
echo -e "- $line \n" >> report.md | ||
cml-publish --title $line --md "$line" >> report.md | ||
done < diff_files.txt | ||
weiji14 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
# Send diff report as GitHub comment | ||
# cml-send-comment report.md | ||
|
||
weiji14 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
# Format report to escape newlines before publishing as GitHub comment | ||
report=$(cat report.md) | ||
report="${report//'%'/'%25'}" | ||
report="${report//$'\n'/'%0A'}" | ||
report="${report//$'\r'/'%0D'}" | ||
echo ::set-output name=report::$report | ||
|
||
- name: Find comment with image diff report | ||
uses: peter-evans/[email protected] | ||
id: fc | ||
with: | ||
issue-number: ${{ github.event.pull_request.number }} | ||
comment-author: 'github-actions[bot]' | ||
body-includes: 'This is an auto-generated report of images that have changed on the DVC remote' | ||
|
||
- name: Create comment with image diff report | ||
if: steps.fc.outputs.comment-id == '' | ||
uses: peter-evans/[email protected] | ||
with: | ||
issue-number: ${{ github.event.pull_request.number }} | ||
body: ${{ steps.image-diff.outputs.report }} | ||
|
||
- name: Update comment with new image diff report | ||
if: steps.fc.outputs.comment-id != '' | ||
uses: peter-evans/[email protected] | ||
with: | ||
comment-id: ${{ steps.fc.outputs.comment-id }} | ||
body: ${{ steps.image-diff.outputs.report }} | ||
edit-mode: replace |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
outs: | ||
- md5: 4d1ed417fd2625d0b2fa5c17dafd32fd | ||
size: 73728 | ||
path: test_legend_entries.png |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,6 @@ | |
from pygmt import Figure | ||
from pygmt.exceptions import GMTInvalidInput | ||
from pygmt.helpers import GMTTempFile | ||
from pygmt.helpers.testing import check_figures_equal | ||
|
||
|
||
@pytest.mark.mpl_image_compare | ||
|
@@ -44,42 +43,25 @@ def test_legend_default_position(): | |
return fig | ||
|
||
|
||
@check_figures_equal() | ||
@pytest.mark.mpl_image_compare | ||
def test_legend_entries(): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rewrite the tests using a simpler 1d array, rather than the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, but best to not modify the test too much in this PR as mentioned in #1104 (comment) |
||
""" | ||
Test different marker types/shapes. | ||
""" | ||
fig_ref, fig_test = Figure(), Figure() | ||
|
||
# Use single-character arguments for the reference image | ||
fig_ref = Figure() | ||
fig_ref.basemap(J="x1i", R="0/7/3/7", B="") | ||
fig_ref.plot( | ||
data="@Table_5_11.txt", | ||
S="c0.15i", | ||
G="lightgreen", | ||
W="faint", | ||
l="Apples", | ||
) | ||
fig_ref.plot(data="@Table_5_11.txt", W="1.5p,gray", l='"My lines"') | ||
fig_ref.plot(data="@Table_5_11.txt", S="t0.15i", G="orange", l="Oranges") | ||
fig_ref.legend(D="JTR+jTR") | ||
|
||
fig_test.basemap(projection="x1i", region=[0, 7, 3, 7], frame=True) | ||
fig_test.plot( | ||
fig = Figure() | ||
fig.basemap(projection="x1i", region=[0, 7, 3, 7], frame=True) | ||
fig.plot( | ||
data="@Table_5_11.txt", | ||
style="c0.15i", | ||
color="lightgreen", | ||
pen="faint", | ||
label="Apples", | ||
) | ||
fig_test.plot(data="@Table_5_11.txt", pen="1.5p,gray", label='"My lines"') | ||
fig_test.plot( | ||
data="@Table_5_11.txt", style="t0.15i", color="orange", label="Oranges" | ||
) | ||
fig_test.legend(position="JTR+jTR") | ||
fig.plot(data="@Table_5_11.txt", pen="1.5p,gray", label='"My lines"') | ||
fig.plot(data="@Table_5_11.txt", style="t0.15i", color="orange", label="Oranges") | ||
fig.legend(position="JTR+jTR") | ||
|
||
return fig_ref, fig_test | ||
return fig | ||
|
||
|
||
@pytest.mark.mpl_image_compare | ||
|
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.
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.