Skip to content
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

Organize baseline images by method and/or cache baseline images during GitHub workflows #2117

Closed
maxrjones opened this issue Sep 15, 2022 · 8 comments
Labels
maintenance Boring but important stuff for the core devs

Comments

@maxrjones
Copy link
Member

Description of the desired feature

I've noticed that our test workflows often fail during the dvc pull step due to a failure to download one or two of the baseline images. For example, 8 of the last 10 runs in https://github.com/GenericMappingTools/pygmt/actions/workflows/ci_tests.yaml report at least one job failing for this reason. This is because it is unstable (and slow) to rely on >160 https calls.

This proposal is to restructure the baseline images according to the suggestion in #1490 (comment), which is to organize the baseline images in directories by method with one .dvc file for each directory rather than a 1:1 match between baseline images and .dvc files. This should reduce the number of https calls from >160 to ~25, which would increase stability and speed. The DAGsHub team previously mentioned that these problems could be eventually fixed by using a different connection protocol, but I don't think we should wait on that.

Another option is to cache the dvc files (both .dvc/cache and pygmt/test/baseline/*.png) similar to how we cache the .gmt files so that the dvc pull step only updates outdated files. But this is really only working around the core issue, so I suggest that this would be in addition to the restructure proposed above.

Are you willing to help implement and maintain this feature? Yes

@maxrjones maxrjones added the question Further information is requested label Sep 15, 2022
@seisman
Copy link
Member

seisman commented Sep 16, 2022

rganize the baseline images in directories by method with one .dvc file for each directory rather than a 1:1 match between baseline images and .dvc files. This should reduce the number of https calls from >160 to ~25, which would increase stability and speed.

Sounds good.

Another possible option: running dvc pull twice. Not sure if it works.

@weiji14
Copy link
Member

weiji14 commented Sep 16, 2022

Here are the number of test images grouped by module-name at commit 561eb41. Maybe we can start opening individual PRs for the modules with >10 baseline images like plot, text, plot3d, and work our way down the list.

  • plot (21)
  • text (18)
  • plot3d (18)
  • grdview (14)
  • meca (11)
  • config (10)
  • basemap (10)
  • makecpt ( 9)
  • rose ( 7)
  • colorbar ( 6)
  • grdimage ( 5)
  • legend ( 4)
  • geopandas ( 4)
  • coast ( 4)
  • subplot ( 4)
  • grdcontour ( 4)
  • contour ( 3)
  • logo ( 2)
  • inset ( 2)
  • velo ( 2)
  • solar ( 2)
  • histogram ( 1)
  • grd2cpt ( 1)
  • figure ( 1)
  • image ( 1)
  • ternary ( 1)
  • wiggle ( 1)

Not sure if we could just skip the modules with 1 baseline image? Or should we just do all for consistency? We might see at some point that the number of HTTP calls falls below the threshold once the top modules are all completed.

P.S., Here's the script to generate the statistics:

import os
import glob
import pandas as pd

png_images = glob.glob("pygmt/tests/baseline/test_*.png")
df = pd.DataFrame(
    data=[os.path.splitext(os.path.basename(filepath))[0] for filepath in png_images],
    columns=["filename"],
)
df_modulename = df.filename.str.split("_", expand=True)[1]
print(len(df_modulename))
print(df_modulename.value_counts())

@weiji14 weiji14 added the maintenance Boring but important stuff for the core devs label Sep 16, 2022
@maxrjones
Copy link
Member Author

Thanks @weiji14 for compiling that information! The plan to start with the methods with many baseline images sounds good to me.

I don't think we'll need to do the methods with just one image if the tests stop failing frequently for this reason.

@weiji14
Copy link
Member

weiji14 commented Sep 16, 2022

Another possible option: running dvc pull twice. Not sure if it works.

Or if you want to give @seisman's suggestion a quick try, could modify

# Pull baseline image data from dvc remote (DAGsHub)
- name: Pull baseline image data from dvc remote
run: |
dvc pull --verbose
ls -lhR pygmt/tests/baseline/
to use https://github.com/nick-fields/retry/tree/v2.8.1#only-retry-after-error. But again, probably not nice to keep pinging DAGsHub with so many HTTP requests 🙂

@maxrjones
Copy link
Member Author

I think it's best to try to fix the root of the problem, which is that we are not using dvc in an optimal way.

@seisman seisman removed the question Further information is requested label Sep 17, 2022
@seisman seisman added this to the 0.8.0 milestone Sep 17, 2022
@willschlitzer
Copy link
Contributor

I'm on board with the plan to make one .dvc file per module. Is the process as simple as moving all of the baseline images from a specifc module (e.g. plot3d) into pygmt/tests/baseline/plot3d, deleting the corresponding .dvc files, and then using the command dvc add pygmt/tests/baseline/plot3d/?

@seisman
Copy link
Member

seisman commented Sep 21, 2022

Is the process as simple as moving all of the baseline images from a specifc module (e.g. plot3d) into pygmt/tests/baseline/plot3d,

I don't think it will work. The tests will fail to find the baseline images.

@seisman seisman modified the milestones: 0.8.0, 0.9.0 Dec 11, 2022
@weiji14 weiji14 removed this from the 0.9.0 milestone Mar 6, 2023
@seisman
Copy link
Member

seisman commented Dec 13, 2023

I don't think we will organize baseline images by method (i.e., tracking directories rather than tracking individual files) as proposed in the OP. The main reasons are:

  1. We haven't seen dvc failures recently. I think it means connections to the DVC server is very stable and our ~180 dvc files are not that much.
  2. Tracking directories will cause more troubles based on the experiences with the GMT repository (see my comment Proposal for managing test baseline images using data version control (dvc) gmt#5724 (comment)).

So, I'm inclined to close the issue. Feel free to re-open it if you don't agree.

@seisman seisman closed this as not planned Won't fix, can't repro, duplicate, stale Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Boring but important stuff for the core devs
Projects
None yet
Development

No branches or pull requests

4 participants