Pooch paths don't work with GitHub Actions.

[mkcor]

Hello @BierretA,

It definitely looks like an issue with Pooch via GitHub Actions (or vice versa). Tests run fine locally for me as well. Looking into the first failure (TestColorconv.test_lab2xyz), we can see that, over GitHub Actions, Pooch is attempting the following

Downloading file 'color/tests/data/lab_array_a_10.npy' from 'https://github.com/scikit-image/scikit-image/raw/main/skimage/color/tests/data/lab_array_a_10.npy' to '/home/runner/.cache/scikit-image/main'.

Unsurprisingly, the expected file doesn't live at https://github.com/scikit-image/scikit-image/raw/main/skimage/color/tests/data/ (yet) since skimage/color/tests/data/lab_array_a_10.npy ships with this PR!

I know I didn't run into any issue with Pooch when I added data files (#4939 merged on Sep 2, 2020). Actually, it is the first time (with this PR) we've added files in the data registry since we started running tests with GitHub Actions. Indeed, the time sequence goes like this:

• Oct 31, 2020: merge latest additions to data registry (Add PIV challenge images to our data #5041)
• Nov 26, 2020: start running test suite with GitHub Actions (github actions for tests #5074)

I need to dig deeper to find out how Pooch builds paths, unless someone else at @scikit-image/core knows it right away. Thanks!

Originally posted by @mkcor in #5234 (comment)

[stefanv]

I think this is to be expected; how could Pooch know where to find the data otherwise? The solution may be to make a single PR to add the data, and another to utilize the data.

[hmaarrfk]

i suspect the file was changed, and the hash was left unchanged in the registry.

pooch should use the local version for builds installed in development mode.

[mkcor]

I think this is to be expected; how could Pooch know where to find the data otherwise? The solution may be to make a single PR to add the data, and another to utilize the data.

Oh, right... I was trying to imagine something dynamic with branch names! 🤦‍♀️

@BierretA could you please submit a separate PR only with the new data files (and updated data registry)? Let me know if there's anything you need help with. Thanks!

[stefanv]

Looks like @hmaarrfk has a better answer; thanks!

[hmaarrfk]

can you confirm that the hash of the file in question matches?

[BierretA]

Hello @hmaarrfk ,

The file color/tests/data/lab_array_a_10.npy was added with the PR. I added the sha256 hash in the registry data/_registry.py by hand. Is there another way to do it? I am not familiar with Pooch at all.
I just checked the hash in the registry and it matches the current file.

To me, it looks like Pooch used the local version of the file when the test was run locally, but not on Github Actions.

@mkcor : I can do the separate PR if it proves useful

[mkcor]

can you confirm that the hash of the file in question matches?

@hmaarrfk the respective hashes of all data files (added with the PR) do match.

@BierretA you did the right thing -- there's just this data file, skimage/color/tests/data/lab_B_2.npy, which does not appear in the data registry...

I can do the separate PR if it proves useful

In my current understanding, this would resolve the following failures:

FAILED color/tests/test_colorconv.py::TestColorconv::test_lab2xyz - requests....
FAILED color/tests/test_colorconv.py::TestColorconv::test_luv2xyz - requests....
FAILED color/tests/test_colorconv.py::TestColorconv::test_xyz2lab - requests....
FAILED color/tests/test_colorconv.py::TestColorconv::test_xyz2luv - requests....

but not these:

FAILED data/tests/test_data.py::test_download_all_with_pooch - requests.excep...
FAILED io/tests/test_collection.py::TestImageCollection::test_custom_load_func_w_kwarg

pooch should use the local version for builds installed in development mode.

@hmaarrfk well, it looks for the data files on "main" where they will ultimately live, indeed (once the PR is merged). By 'should' do you mean it's Pooch's expected/default behaviour or should we configure it to ensure "it uses the local version for builds installed in development mode?"

Thanks!

[hmaarrfk]

@mkcor I think the only test that should fail is the one marked with INSTALL_FROM_SDIST where tests, and everything is installed from the source distribution.

All others should be able to use the local builds.

If only the sdist test is failing, I would merge and move on.

[hmaarrfk]

Its also possible that the build procedure got changed more than I kept up with.

It used to be that the tests were run in editable mode installs by default. In those cases it was able to find the local versions of the files.

For

pip install . -vv

would require files to be pulled from github

pip install -e . -vv

would be able to pull the files from the local git repo. <- edit added the word local

I'm not sure which command is being used in the github actions.

It might be simplest to merge the files directly before this particular PR.

[mkcor]

In my current understanding, this would resolve the following failures: [...] but not these: [...]

Confirming this: https://github.com/scikit-image/scikit-image/actions/runs/664335330

I think the only test that should fail is the one marked with INSTALL_FROM_SDIST where tests, and everything is installed from the source distribution. [...]
I'm not sure which command is being used in the github actions.

@hmaarrfk if not INSTALL_FROM_SDIST, it's the command you except that is used in GitHub Actions:

scikit-image/.github/workflows/tests.yml

Line 92 in a69d492

pip install -vv -e .;

Nonetheless, the tests (other than sdist) apparently pull the files from GitHub (not from the local repo), again:

requests.exceptions.HTTPError: 404 Client Error: Not Found for url: https://github.com/scikit-image/scikit-image/raw/main/skimage/color/tests/data/lab_array_a_10.npy
[...]
Downloading file 'color/tests/data/lab_array_a_10.npy' from 'https://github.com/scikit-image/scikit-image/raw/main/skimage/color/tests/data/lab_array_a_10.npy' to '/home/runner/.cache/scikit-image/main'.

from https://github.com/scikit-image/scikit-image/runs/2139138616 for example.

So, in PR #5276, I reverted the update of the data registry, including only the new data files. Once #5276 is merged, we can update the data registry and tests such as test_download_all_with_pooch will pass.

[stefanv]

Thank you for investigating @mkcor. When we're done, it would be good to add a note about this process to the developer guide too.

[grlee77]

@mkcor, can this issue be closed or is there still something remaining to be resolved?

[mkcor]

@grlee77 it can be closed, right, thanks for keeping tabs! Closing now.