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

Switch to pytest-mpl for image testing #1891

Merged
merged 2 commits into from
Sep 29, 2021
Merged

Conversation

QuLogic
Copy link
Member

@QuLogic QuLogic commented Sep 24, 2021

With pytest-mpl, we can throw away a bunch of code for image comparison and testing. I don't consider that public API, so didn't bother writing any deprecation.

To minimize changes, I returned ax.figure when we have an Axes, but no explicit Figure. Also, I added in a pytest hook to generate the baseline image directory in some manner that matches the old one.

@QuLogic QuLogic added this to the 0.21 milestone Sep 24, 2021
@QuLogic
Copy link
Member Author

QuLogic commented Sep 24, 2021

You can see that failed uploads work here where I removed all non-default tolerances: https://github.com/QuLogic/cartopy/actions/runs/1268110201

@QuLogic QuLogic force-pushed the pytest-mpl branch 2 times, most recently from 42ca86f to 8e9b239 Compare September 24, 2021 08:07
Copy link
Contributor

@greglucas greglucas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we may need to think about the PYPROJ_GLOBAL_CONTEXT as there are locks on the PROJ database that occasionally cause the tests to fail.

lib/cartopy/tests/mpl/conftest.py Outdated Show resolved Hide resolved
@QuLogic
Copy link
Member Author

QuLogic commented Sep 28, 2021

There used to be a lock on all image tests, but they were only supposed to prevent the 1 or 2 tests that used the same test image from clashing. Since pytest-mpl saves the test images in different directories, this shouldn't be needed. But it seems that the slowdown of the locking was enough to prevent Proj contexts from stomping on each other.

I don't know if pre-downloading the required grids will help (I already know which ones are needed from #1880).

There have been several threading/multiprocess bugs in both Proj and pyproj, and I'm not sure if conda-forge has all the fixes, or if they are all released. Maybe @snowman2 knows?

@snowman2
Copy link
Contributor

I'm not sure if conda-forge has all the fixes, or if they are all released

They are all released. You just can't use the global context if you need thread safety.

@QuLogic
Copy link
Member Author

QuLogic commented Sep 28, 2021

I'm not sure if conda-forge has all the fixes, or if they are all released

They are all released.

But on conda-forge too? We only get proj 8.0.1, looks like.

You just can't use the global context if you need thread safety.

pytest-xdist is process based, I believe, not threaded.

@snowman2
Copy link
Contributor

pytest-xdist is process based, I believe, not threaded

I would be surprised if the global context was safe for processes.

@snowman2
Copy link
Contributor

snowman2 commented Sep 28, 2021

But on conda-forge too? We only get proj 8.0.1, looks like.

It depends on the dependencies you have. If you only install pyproj, you will get the latest. Other dependencies might have a migration PR pending.

@snowman2
Copy link
Contributor

I looked at the failure message and I think it is due to the fact that PROJ NETWORK is attempting to write to the cache database at the same time. A workaround is to download the grids and disable the network. Might also be worthwhile to open an issue in PROJ.

@greglucas
Copy link
Contributor

I'm not sure it is due the lockfile change in this PR either (I think this has happened sporadically on some of the other PRs and even for me locally too), so I think this PR is good, modulo my comment above about the naming of "mpl".

@greglucas greglucas merged commit b4368b2 into SciTools:master Sep 29, 2021
@QuLogic QuLogic deleted the pytest-mpl branch September 29, 2021 23:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants