Simplify appveyor to only use conda #24397
Conversation
3d7ebd5 to
b619c4b
Compare
|
Not quite fixed yet, though I suspect the tests which are failing were previously skipped due to differences in installed dependencies (I found one such example of that with |
environment.yml
Outdated
| @@ -22,7 +22,6 @@ dependencies: | |||
| - python-dateutil>=2.1 | |||
| - setuptools | |||
| - setuptools_scm | |||
| - wxpython | |||
There was a problem hiding this comment.
I'm unclear whether removal is ok. The original purpose of environment.yml is to provide contributors with an easy way to set up a dev environment. We should how complete that should be in terms of optional dependencies, but I'm inclined to make a dev environment as complete as possible.
There was a problem hiding this comment.
yeah, that was a point I wanted some clarification on as well, wx was failing some tests and wasn't installed in the pip-main version of appveyor, so I removed it mostly to see if it solved all of the failing tests... It got most of them, but there is still one for GTK and the fontconfig one.
I suppose one alternative is to create a new CI environment.yml instead that strips these out. (or figuring out why the interactive tests are failing in the first place, but that is a bit more scope than I really want to take on in a "get the CI running again" PR, especially if its windows specific as I do not have a windows box at the ready for local testing currently)
There was a problem hiding this comment.
the wx tests which are failing are all using the wx backend (rather than wxagg or wxcairo) which is deprecated anyway (and has been since 2.0...)
I think we just skip those tests, keep the environment file in tact.
That leaves only the the gtk one which is failing.
environment.yml
Outdated
| @@ -44,6 +43,7 @@ dependencies: | |||
| - coverage | |||
| - flake8>=3.8 | |||
| - flake8-docstrings>=1.4.0 | |||
| - fontconfig | |||
There was a problem hiding this comment.
This unskips the following test. Apparently fontconfig does not work.
__________________________ test_get_fontconfig_fonts __________________________
[gw0] win32 -- Python 3.8.13 C:\Miniconda3-x64\envs\mpl-dev\python.exe
@pytest.mark.skipif(not has_fclist, reason='no fontconfig installed')
def test_get_fontconfig_fonts():
> assert len(_get_fontconfig_fonts()) > 1
E assert 0 > 1
E + where 0 = len([])
E + where [] = _get_fontconfig_fonts()
lib\matplotlib\tests\test_font_manager.py:78: AssertionError
There was a problem hiding this comment.
Eh, not exactly, fontconfig is actually a transitive dependency anyway, so that test was failing and I tried adding it explicitly to see if that fixed it (it did not) because the error message indicated that it might be a version thing. Didn't get around to removing it again.
There was a problem hiding this comment.
fontconfig is brought in in a couple ways with this environment.yml: (and additional ones on unix systems, which made it harder to track down)
but notably, graphviz transitively depends on fontconfig via pango and through libgd (graphviz directly depends on fontconfig on unix)
Some of the gtk/cairo may also depend on it transitively, though those were mostly host dependencies not run dependencies from what I saw.
There was a problem hiding this comment.
Okay, the crux is that _get_fontconfig_fonts relies on subprocess call to fc-config --help (to determine if a supported version is available)
but the windows version of this utility does not use posix-style options. (At least I think that's why, testing out windows style options now)
There was a problem hiding this comment.
actually, looking at where _get_fontconfig_fonts is called, it doesn't get called on windows, so I just explicitly skipped that test
There was a problem hiding this comment.
Yes, I don't think there's any point in running this test on Windows, as fontconfig is not used there.
|
Down to one failure in the game of whack-a-mole that is appveyor... for some reason only after I swapped to gtk4 the wxagg memory leak test started failing. The test in question has been part of prior discussion (see #22988) as potentially suboptimal. I'm inclined to skip it on appveyor and get CI green again in the immediate term, but perhaps we should revisit the mem leak test and implement something like @tacaswell suggested in that issue. |
I am 👍 on this plan if the the issue is the memleak tests. |
Closes matplotlib#24394 Adjust environment.yml skip fontconfig private method test on windows skip fontconfig private method test on windows revert adding fontconfig explicitly to environment Add wxpython back Skip some wx tests Upgrade to gtk4 in environment.yml ignore private modules in getattr test _backend_gtk would fail because it expects one of gtk4 or gtk3 to be imported first Clean up appveyor.yml comments skip memleak test for wxagg conda activate does not, in fact, work revert to simply activate
|
well, it passed once, but then went back to a different backend failing the same test after I squashed/removed prints from the subprocess helper function. So just skipped the whole test. |
| @@ -104,7 +95,7 @@ artifacts: | |||
| type: zip | |||
|
|
|||
| on_finish: | |||
| - pip install codecov | |||
| - conda install codecov | |||
There was a problem hiding this comment.
Likely
| - conda install codecov | |
| - conda install -c conda-forge codecov |
because we don't want to mix conda forge and main channel.
There was a problem hiding this comment.
Conda forge is actually configured by the environment, so everything is conda-forge. The earlier instance of installing win32 could actually remove the explicit -c conda-forge
https://ci.appveyor.com/project/matplotlib/matplotlib/builds/45337448/job/aie7c1u8m0pf3nf6#L1893
There was a problem hiding this comment.
Where does conda say that the channel config from an environment.yml is preseved? I just tested with a minimal
name: cftest
channels:
- conda-forge
dependencies:
- python=3.7
and when I do a conda install numpy in that environment, conda wants to install from the main channel.
There was a problem hiding this comment.
In that case, its probably because we prepend conda-forge to the base channels list before creating the environment:
Line 49 in e2131bb
lib/matplotlib/tests/test_getattr.py
Outdated
|
|
||
|
|
||
| @pytest.mark.parametrize('module_name', module_names) | ||
| @pytest.mark.filterwarnings('ignore::DeprecationWarning') | ||
| @pytest.mark.parametrize("module_name", module_names) |
There was a problem hiding this comment.
Is this and the following from a code formatter? My understanding is that we don't want code churn like this. If we want to introduce more strict formatting, we need to decide this centrally and introduce this in a coordinated way.
There was a problem hiding this comment.
Guilty... I did apply black (only to this file) when flake8 started complaining about the formatting of the module name filter
I can revert the quote changes if we want.
|
@meeseeksbot please backport to v3.6.x |
|
@meeseeksdev please backport to v3.6.x |
…397-on-v3.6.x Backport PR #24397 on branch v3.6.x (Simplify appveyor to only use conda)
Closes #24394
PR Summary
Avoid mixing conda and pip (except for those pip dependencies in the environment.yml)
Should fix appveyor tests failing due to pip pywin32 version.
PR Checklist
Tests and Styling
pytestpasses).flake8-docstringsand runflake8 --docstring-convention=all).Documentation
Release Notes
.. versionadded::directive in the docstring and documented indoc/users/next_whats_new/.. versionchanged::directive in the docstring and documented indoc/api/next_api_changes/next_whats_new/README.rstornext_api_changes/README.rst