Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Simplify appveyor to only use conda#24397
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
3d7ebd5
tob619c4b
CompareNotquite 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm unclear whether removal is ok. The original purpose ofenvironment.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
the wx tests which are failing are all using thewx
backend (rather thanwxagg
orwxcairo
) 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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()) > 1E assert 0 > 1E + where 0 = len([])E + where [] = _get_fontconfig_fonts()lib\matplotlib\tests\test_font_manager.py:78: AssertionError
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.
Eh, not exactly,fontconfig
is actually a transitive dependency anyway, so that test was failing and I triedadding it explicitly to see if thatfixed 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 viapango
and throughlibgd
(graphviz directly depends on fontconfig on unix)
Some of the gtk/cairo may also depend on it transitively, though those were mostlyhost
dependencies notrun
dependencies from what I saw.
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.
Okay, the crux is that_get_fontconfig_fonts
relies on subprocess call tofc-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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, I don't think there's any point in running this test on Windows, asfontconfig
is not used there.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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. |
Uh oh!
There was an error while loading.Please reload this page.
Closesmatplotlib#24394Adjust environment.ymlskip fontconfig private method test on windowsskip fontconfig private method test on windowsrevert adding fontconfig explicitly to environmentAdd wxpython backSkip some wx testsUpgrade to gtk4 in environment.ymlignore private modules in getattr test_backend_gtk would fail because it expects one of gtk4 or gtk3 to be imported firstClean up appveyor.yml commentsskip memleak test for wxaggconda activate does not, in fact, workrevert 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Likely
-conda install codecov | |
-conda install-c conda-forgecodecov |
because we don't want to mix conda forge and main channel.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Where does conda say that the channel config from an environment.yml is preseved? I just tested with a minimal
name: cftestchannels: - conda-forgedependencies: - python=3.7
and when I do aconda install numpy
in that environment,conda
wants to install from the main channel.
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.
In that case, its probably because we prepend conda-forge to the base channels list before creating the environment:
Line 49 ine2131bb
-conda config --prepend channels conda-forge |
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.
Ah, that should be it.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Guilty... I did applyblack
(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.xBackport 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
pytest
passes).flake8-docstrings
and 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.rst
ornext_api_changes/README.rst