Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
tacaswell merged 3 commits intomatplotlib:mainfromksunden:appveyor
Nov 14, 2022
Merged

Conversation

ksunden
Copy link
Member

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

  • Has pytest style unit tests (andpytest passes).
  • IsFlake 8 compliant (installflake8-docstrings and runflake8 --docstring-convention=all).

Documentation

  • Documentation is sphinx and numpydoc compliant (the docs shouldbuild without error).
  • New plotting related features are documented with examples.

Release Notes

  • New features are marked with a.. versionadded:: directive in the docstring and documented indoc/users/next_whats_new/
  • API changes are marked with a.. versionchanged:: directive in the docstring and documented indoc/api/next_api_changes/
  • Release notes conform with instructions innext_whats_new/README.rst ornext_api_changes/README.rst

@ksundenksundenforce-pushed theappveyor branch 5 times, most recently from3d7ebd5 tob619c4bCompareNovember 8, 2022 07:30
@ksunden
Copy link
MemberAuthor

Notquite 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 withwx which was inenvironment.yml but was not previously installed)

@@ -22,7 +22,6 @@ dependencies:
- python-dateutil>=2.1
- setuptools
- setuptools_scm
- wxpython
Copy link
Member

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.

Copy link
MemberAuthor

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)

Copy link
MemberAuthor

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.

timhoffm reacted with thumbs up emoji
@@ -44,6 +43,7 @@ dependencies:
- coverage
- flake8>=3.8
- flake8-docstrings>=1.4.0
- fontconfig
Copy link
Member

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

Copy link
MemberAuthor

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.

Copy link
MemberAuthor

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.

Copy link
MemberAuthor

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)

Copy link
MemberAuthor

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

Copy link
Member

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.

@ksunden
Copy link
MemberAuthor

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.

@tacaswell
Copy link
Member

I'm inclined to skip it on appveyor and get CI green again in the immediate term

I am 👍 on this plan if the the issue is the memleak tests.

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
@ksunden
Copy link
MemberAuthor

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
Copy link
Member

Choose a reason for hiding this comment

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

Likely

Suggested change
-conda install codecov
-conda install-c conda-forgecodecov

because we don't want to mix conda forge and main channel.

Copy link
MemberAuthor

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

Copy link
Member

@timhoffmtimhoffmNov 11, 2022
edited
Loading

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.

Copy link
MemberAuthor

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:

-conda config --prepend channels conda-forge

Copy link
Member

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.



@pytest.mark.parametrize('module_name', module_names)
@pytest.mark.filterwarnings('ignore::DeprecationWarning')
@pytest.mark.parametrize("module_name", module_names)
Copy link
Member

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.

Copy link
MemberAuthor

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.

@tacaswelltacaswell merged commit37c4e9d intomatplotlib:mainNov 14, 2022
@tacaswelltacaswell added this to thev3.7.0 milestoneNov 14, 2022
@ksundenksunden deleted the appveyor branchNovember 14, 2022 20:21
@tacaswell
Copy link
Member

@meeseeksbot please backport to v3.6.x

@tacaswell
Copy link
Member

@meeseeksdev please backport to v3.6.x

meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull requestDec 4, 2022
oscargus added a commit that referenced this pull requestDec 4, 2022
…397-on-v3.6.xBackport PR#24397 on branch v3.6.x (Simplify appveyor to only use conda)
@ksundenksunden mentioned this pull requestFeb 20, 2023
6 tasks
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@QuLogicQuLogicQuLogic approved these changes

@timhoffmtimhoffmtimhoffm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.7.0
Development

Successfully merging this pull request may close these issues.

[TST]: Appveyor Qt tests failing
4 participants
@ksunden@tacaswell@QuLogic@timhoffm

[8]ページ先頭

©2009-2025 Movatter.jp