Skip to content

CI: Unify required dependencies installation #16117

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 17, 2020
Merged

CI: Unify required dependencies installation #16117

merged 1 commit into from
Jan 17, 2020

Conversation

Kojoley
Copy link
Member

@Kojoley Kojoley commented Jan 6, 2020 •
edited
Loading

  • Do not update default Conda packages (saves 5 minutes on 3.6 job)
  • Do not install Freetype2 from Conda (we do not use it anyway)
  • Install dependencies via pip, the same way as it's done on Travis.
    However, travis36minver.txt cannot be used, because of some bugs
    in the pinned versions (something with pandas/pytz/dateutils)
  • Test sphinext on Travis too

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@tacaswell tacaswell added this to the v3.3.0 milestone Jan 6, 2020
@Kojoley Kojoley force-pushed the ci-appveyor-unify-reqs branch 16 times, most recently from dd84710 to df5b180 Compare January 6, 2020 17:14
@Kojoley
Copy link
Member Author

Kojoley commented Jan 6, 2020 •
edited
Loading

Sigh. Having installed both PySide2/Qt5 and Tk seems to produce a conflict in tests on Appveyor.

SKIPPED [11] C:\projects\matplotlib\lib\matplotlib\tests\test_backend_qt.py:32: Qt5 binding already imported
SKIPPED [1] C:\projects\matplotlib\lib\matplotlib\testing\conftest.py:68: Failed to switch to backend TkAgg (Cannot load backend 'TkAgg' which requires the 'tk' interactive framework, as 'qt5' is currently running).

@Kojoley Kojoley force-pushed the ci-appveyor-unify-reqs branch 2 times, most recently from 22daf59 to a8a0520 Compare January 6, 2020 19:42
NelleV
NelleV reviewed Jan 15, 2020
View reviewed changes
.appveyor.yml
@@ -84,7 +84,7 @@ test_script:
- '"%DUMPBIN%" /DEPENDENTS lib\matplotlib\ft2font*.pyd | findstr freetype.*.dll && exit /b 1 || exit /b 0'

# this are optional dependencies so that we don't skip so many tests...
- if x%TEST_ALL% == xyes conda install -q ffmpeg inkscape miktex pillow
- if x%TEST_ALL% == xyes conda install -q ffmpeg inkscape miktex
Copy link
Member

Choose a reason for hiding this comment

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

We seem to have lost pillow here as an optional dependency. Do you have a sense of many tests are not being ran as a result?

Copy link
Member Author

Choose a reason for hiding this comment

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

We do not. It is installed via pip from travis_all.txt.

Copy link
Member

Choose a reason for hiding this comment

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

Great! Thanks for the clarification :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I think we should just delete cycler/numpy/pillow from travis_all.txt, given that we can rely on pip to install them (they're all straight dependencies). (and travis36minver will pin the oldest versions we want to test on that single test instance)

Copy link
Member Author

Choose a reason for hiding this comment

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

That makes sense, but I will leave it for other PR.

NelleV
NelleV approved these changes Jan 15, 2020
View reviewed changes
Copy link
Member

@NelleV NelleV left a comment

Choose a reason for hiding this comment

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

👍
Thanks @Kojoley !

anntzer
anntzer reviewed Jan 15, 2020
View reviewed changes
@Kojoley Kojoley force-pushed the ci-appveyor-unify-reqs branch from a8a0520 to 0e018ce Compare January 15, 2020 13:18
anntzer
anntzer reviewed Jan 15, 2020
View reviewed changes
anntzer
anntzer reviewed Jan 15, 2020
View reviewed changes
.appveyor.yml
- python -mpip install --upgrade -r requirements/testing/travis_all.txt %EXTRAREQS% %PINNEDVERS%
# Install optional dependencies from PyPI.
# Sphinx is needed to run sphinxext tests
- python -mpip install --upgrade sphinx
Copy link
Contributor

Choose a reason for hiding this comment

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

should this just go into EXTRAREQS?

Copy link
Member Author

Choose a reason for hiding this comment

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

I do not know, probably then GUIs also should go into EXTRAREQS

anntzer
anntzer reviewed Jan 15, 2020
View reviewed changes
.travis.yml
@@ -133,6 +133,8 @@ install:
python -mpip install --upgrade $PRE -r requirements/testing/travis_all.txt $EXTRAREQS $PINNEDVERS
- |
# Install optional dependencies from PyPI.
# Sphinx is needed to run sphinxext tests
python -mpip install --upgrade sphinx
Copy link
Contributor

Choose a reason for hiding this comment

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

as above

@Kojoley
CI: Unify required dependencies installation
9c8c743 
- Do not update default Conda packages (saves 5 minutes on 3.6 job)
- Do not install Freetype2 from Conda (we do not use it anyway)
- Install dependencies via pip, the same way as it's done on Travis.
  However, `travis36minver.txt` cannot be used, because of some bugs
  in the pinned versions (something with pandas/pytz/dateutils)
- Test sphinext on Travis too
@Kojoley Kojoley force-pushed the ci-appveyor-unify-reqs branch from 0e018ce to 9c8c743 Compare January 15, 2020 14:09
@NelleV
Copy link
Member

NelleV commented Jan 17, 2020

@tacaswell @anntzer Are we good on merging this PR

anntzer
anntzer approved these changes Jan 17, 2020
View reviewed changes
Copy link
Contributor

@anntzer anntzer left a comment

Choose a reason for hiding this comment

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

could be further refactored, but sure

@anntzer anntzer changed the title CI: Unify required dependencies installation Merge pull request #16117 from Kojoley/ci-appveyor-unify-reqs Jan 17, 2020
@anntzer anntzer merged commit db8cdf2 into matplotlib:master Jan 17, 2020
@Kojoley Kojoley deleted the ci-appveyor-unify-reqs branch January 17, 2020 12:20
@QuLogic
Copy link
Member

QuLogic commented Jan 30, 2020

@anntzer I don't understand why you changed the PR title that way?

@anntzer
Copy link
Contributor

anntzer commented Jan 30, 2020

Oops, sorry, this was due to refined-github/refined-github#2713. Deactivated it, and renamed.

@anntzer anntzer changed the title Merge pull request #16117 from Kojoley/ci-appveyor-unify-reqs CI: Unify required dependencies installation Jan 30, 2020
@Kojoley Kojoley mentioned this pull request Feb 19, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NelleV NelleV NelleV approved these changes

@anntzer anntzer anntzer approved these changes

Assignees
No one assigned
Labels
topic: testing
Projects
None yet
Milestone
v3.3.0
Development

Successfully merging this pull request may close these issues.
5 participants
@Kojoley @NelleV @QuLogic @anntzer @tacaswell