Pytzectomy by pganssle · Pull Request #11360 · matplotlib/matplotlib · GitHub

PR Summary

This removes pytz as a required dependency in favor of dateutil.tz, for the reasons detailed in the ticket. In addition to the "removing an unnecessary dependency" aspect of this, I have fleshed out the reasons to use dateutil over pytz in this blog post. I considered linking to the blog post in the "API changes" section to explain more of the reasoning, but I'm not sure it's actually necessary.

With regards to the testing, there may already be enough baseline "both pytz and dateutil" testing to ensure that pytz is still a supported option (specifically the test_rrulewrapper_pytz covers both the most important corner case and the use of pytz on an axis), but it wouldn't be a bad idea to add a few more tests for that. I recommend making sure that pytz is not installed during the main tests, though (the way I set it up the pytz-specific tests are run through tox so that it is only installed in the virtualenv), to avoid any accidental implicit dependencies that may arise.

PR Checklist

Has Pytest style unit tests
Code is PEP 8 compliant
Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

I'm getting the same Travis failures from invoking tox in my home directory. Is using tox not supported?

I will note that I've actually really liked it ever since I started using tox to drive all the builds and have both Travis and Appveyor just invoke various tox testenvs. Might not be a bad idea to switch matplotlib over to doing something similar (though because matplotlib has a much more expensive build, it's probably a good idea to figure out how to cache the builds between environments and such.

I think the difference in code coverage comes from the fact that there's a net negative number of covered lines in the PR, not any changes to the actual coverage.

On gitter you suggested this should be a blocker for 3.0. Care to elaborate the argument?

@jklymak In the original issue #10443, I suggested that this be done as part of a major release, because there is an argument to be made that this would break backwards compatibility in some rare situations, since pytz timezones have a different interface than standard tzinfo classes.

While one could argue that you maybe shouldn't be counting on the fact that the timezones used by matplotlib itself are pytz objects, I think the best thing to do is to just do it as part of a backwards-compat-breaking release. Preferably that would be 3.0 and not some future breaking release.

I'm still not sure how we feel about the tox requirement for the testing, so haven't reviewed this. The general idea seems fine to me...

Is that doing two full test suites, one with and one without pytz? If yes, that's way too much; if no, what is this doing?

In general I agree with @jklymak that we probably don't want the test suite to be based on tox.

No, tox -e pytz only runs the pytz environment, which runs pytest -m pytz, and thus only runs tests decorated with pytest.mark.pytz.

try adding a set -e to the script section? or combining the two lines via &. I think the issue is that the return value from the script section is the return of the last thing called.

Yes, I came to this conclusion as well, though the main problem with this is that if the first one fails the second one won't be run at all.

Ideally the main test suite would also be run with tox, so that you can do:

tox -e py,pytz

Which would run both environments and have a single return code. If the project doesn't want to use tox as the main test runner, we can use the -e or pytest ... && tox -e pytz solution as a "for now" and leave it to a later PR to refactor the test suite as preferred.

double underscore seems a bit overkill

I have no strong preference here. I think I used double underscore because I intended it to be ephemeral. I think maybe I should switch it to:

def __has_pytz():
    ...
HAS_PYTZ = __has_pytz()
del __has_pytz()

We can drop the del line and switch to a single underscore if that's preferred style.

What's this mark?

All otherwise unspecified marks are used to register a test with the mark of that name. This indicates that the decorated test uses pytz, so that you can filter out tests that require pytz (or that don't require pytz).

I'm still not sure how we feel about the tox requirement for the testing, so haven't reviewed this. The general idea seems fine to me...

I think the tox thing doesn't really matter very much. It's just a convenient way to run the test with its own virtual environment so that pytz doesn't need to be installed during the main run of the tests. End users probably will have pytz installed (or can install it themselves), and the pytz-specific tests are opportunistic in that they will run whenever pytz is installed.

In any case, I think it should probably not block this PR for the following reasons:

So long as tox.ini exists, it's actually probably good to have a test that uses it, since otherwise it's prone to bitrot. If you want to drop tox entirely, I think a separate PR that removes tox and replaces it in this case would be appropriate.
It is only a build-time dependency and has no impact on end-users, and it already works. There's almost no cost to doing it this way and it can be changed at any point on master, whereas the substantive changes should be in before 3.0 and are user facing.

Hm, disturbing that this line isn't being hit. Something is indirectly pulling in a pytz dependency in the main test suite.

I think the problem is pandas. I was under the impression that the pandas integration tests were being run separately from the main test suite. If that's no longer the case, then this is a bit of a problem, since you could have implicit dependencies on any of the pandas dependencies.

This seems like a useful PR that has hung up because of how the tests are set up. Can we defer the argument about tox for a different PR, and set these tests up more traditionally so we can merge it before 3.0?

Of course if the tox setup is something core devs with more experience with the test setup want to keep, please feel free to move forward w/ this. I'm only objecting out of lack of familiarity and because its not something we typically do, not for any fundamental reasons.

@jklymak I do think we should try and get this merged ASAP, but the problem of tox vs. no tox is not the main problem. The main reason I brought in tox was because I was trying to run the tests without pytz to prove that matplotlib doesn't have an unnoticed dependency on pytz, but also have some tests that run with pytz installed to prove that matplotlib still works if you give it pytz datetimes.

The problem is that the pandas tests are no longer run in a separate environment from the rest of the tests, and pandas pulls in pytz. I'm not really sure where to go from here. Should we separate the pandas tests back out? Should we try to run just the explicitly datetime-related tests that don't overlap with the pandas-specific tests in a fresh environment with no pytz installed?

I would advocate for running all tests that involve optional dependencies in separate environments to avoid accidentally upgrading them from optional to required dependencies.

We should be running at least one test without pandas (see https://travis-ci.org/matplotlib/matplotlib/jobs/391289494#L2387 where one of these test suites is skipping due to a missing pandas dep).

Can your rebase this (the requirements got re-organized into stand-alone-files recently)?

I am worried that we have coverage set up slightly wrong and it is not properly merging reports...

@tacaswell OK, done. Hopefully I got the travis configuration right.

On the job you link to, did you notice line 2381?

....................Coverage.py warning: No data was collected. (no-data-collected)

Presumably that is the problem? Not sure why, though.

I can't really make heads or tails of what's going on here. This CI run has actual test failures!, but it still gets a green check.

Edit: This may be because I'm running tox -e pytz afterwards, so only the return value of the last run tests matters.

The culprit installing pytz is sphinx via babel

Collecting pytz>=0a (from babel!=2.0,>=1.3->sphinx->-r requirements/testing/travis_all.txt (line 18))

@pganssle may I push to your branch?

@tacaswell Good find, go for it.

Looks like the coverage problem is fixed, so I'm going to go ahead and squash my fixup commits.

In the interest of expediancy we are merging allowing that wx failure on mac (which we are still having issues sorting out if it is a bug on our side or dependent on the wx version).

@tacaswell I squashed your last commit into the one where the .travis.yml was originally changed - I think that was an artifact of the merge conflict from the original rebase.

LGTM, except for the test stuff I still haven't groked.

Any chance this can be squashed before merging?

I can squash it if you want, but I'm pretty meticulous about my history, so I always squash related commits and use small, atomic commits so it's cleaner when looking through the history or doing a git bisect. I've already squashed all the various fixups (except for Tom's, but I can squash those all into the CI-changes commit if you'd prefer).

The artistry of how much gets squashed before a merge is beyond my pay grade - just noting that we don't usually have 9 commits for a change this size.

@pganssle I rebased and force-pushed to your branch to resolve a conflict in the travis file.

Actually, force-pushed twice to fix a commit message. Plan to merge this when it passes if @pganssle do not protest.

The separate tests with and without pytz are done to try to minimize the possibility of implicit dependencies on pytz. Run the second test in it's own entry in the script section of the travis file so that it does not mask failures from other sections.

Sphinx pulls in pytz which we want to avoid to test running without it. The sphinx related code is well tested by the circle CI that builds the docs.

I re-signed all my commits (I don't know if you care about that). Feel free to merge whenever, or just let me know how you'd like the commits squashed and I can squash and sign the squashed commits.

Great, will merge this when it passes.

Looks like the tox call is creating a bunch of extra files, which the PEP8 checker doesn't like, maybe adding .tox/* to .gitignore will help, or similar thing to the PEP8 configuration file?

Thanks @pganssle !

This was a PR that travis did not want to go in....

pganssle force-pushed the pytzectomy branch from 5fd67b5 to 4c35da0 Compare June 1, 2018 14:34

pganssle force-pushed the pytzectomy branch from 4c35da0 to d6ada55 Compare June 1, 2018 15:25

pganssle mentioned this pull request Jun 1, 2018

Tox.ini does not work out of the box #11361

Closed

pganssle force-pushed the pytzectomy branch from d6ada55 to a3ea1b5 Compare June 12, 2018 14:17

jklymak added this to the v3.0 milestone Jun 12, 2018

anntzer reviewed Jun 12, 2018

View reviewed changes

pganssle commented Jun 12, 2018

View reviewed changes

dstansby added the topic: date handling label Jul 5, 2018

jklymak added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Jul 5, 2018

pganssle force-pushed the pytzectomy branch from a3ea1b5 to 695143e Compare July 5, 2018 17:08

pganssle force-pushed the pytzectomy branch from 695143e to 9fd5036 Compare July 5, 2018 18:15

pganssle force-pushed the pytzectomy branch 2 times, most recently from 1a12144 to 7578b32 Compare July 6, 2018 14:03

pganssle force-pushed the pytzectomy branch from dbecfdd to 56340b6 Compare July 6, 2018 15:11

tacaswell approved these changes Jul 6, 2018

View reviewed changes

jklymak approved these changes Jul 6, 2018

View reviewed changes

tacaswell force-pushed the pytzectomy branch from 56340b6 to 36eef9f Compare July 8, 2018 18:40

tacaswell force-pushed the pytzectomy branch from 36eef9f to 124cadf Compare July 8, 2018 18:43

pganssle and others added 9 commits July 8, 2018 15:18

Remove pytz dependency from axis.py

5129b0b

Remove pytz from dates.py

eec4a5a

TST: Remove pytz as hard dependency in tests

d6339b5

Remove pytz from build dependencies

23b41b1

DOC: Remove pytz from documentation

75c5c48

DOC: Add API changes entry for pytz removal

f8e76bf

CI: only install sphinx on py35 build

ab92163

Sphinx pulls in pytz which we want to avoid to test running without it. The sphinx related code is well tested by the circle CI that builds the docs.

TST: skip sphinx tests if no sphinx installed

07fdbe8

pganssle force-pushed the pytzectomy branch from 124cadf to 07fdbe8 Compare July 8, 2018 19:18

TST: exclude tox files from flake8 checking

590ee8e

tacaswell force-pushed the pytzectomy branch from d58357e to 590ee8e Compare July 9, 2018 02:51

tacaswell merged commit 9324b27 into matplotlib:master Jul 9, 2018

timhoffm mentioned this pull request Dec 16, 2018

Update windows build instructions #13001

Merged

               script: |
                 echo "Calling pytest with the following arguments: $PYTEST_ADDOPTS"
                 python -mpytest
+                tox -e pytz

+                      import pytz
+                      return True
+                  except ImportError:
+                      return False

		_test_rrulewrapper(attach_tz, dateutil.tz.gettz)


		@pytest.mark.pytz

Pytzectomy by pganssle · Pull Request #11360 · matplotlib/matplotlib · GitHub

Repository navigation

Pytzectomy - #11360

Pytzectomy#11360tacaswell merged 10 commits intomatplotlib:mastermatplotlib/matplotlib:masterfrom pganssle:pytzectomypganssle/matplotlib:pytzectomyCopy head branch name to clipboard

Conversation

pganssle commented Jun 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

pganssle commented Jun 1, 2018

Uh oh!

pganssle commented Jun 12, 2018

Uh oh!

jklymak commented Jun 12, 2018

Uh oh!

pganssle commented Jun 12, 2018

Uh oh!

jklymak commented Jun 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pganssle commented Jun 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jklymak commented Jul 5, 2018

Uh oh!

pganssle commented Jul 5, 2018

Uh oh!

tacaswell commented Jul 5, 2018

Uh oh!

pganssle commented Jul 5, 2018

Uh oh!

pganssle commented Jul 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pganssle commented Jul 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tacaswell commented Jul 5, 2018

Uh oh!

pganssle commented Jul 5, 2018

Uh oh!

pganssle commented Jul 6, 2018

Uh oh!

tacaswell commented Jul 6, 2018

Uh oh!

pganssle commented Jul 6, 2018

Uh oh!

jklymak left a comment

Choose a reason for hiding this comment

Uh oh!

jklymak commented Jul 6, 2018

Uh oh!

pganssle commented Jul 6, 2018

Uh oh!

jklymak commented Jul 6, 2018

Uh oh!

tacaswell commented Jul 8, 2018

Uh oh!

tacaswell commented Jul 8, 2018

Uh oh!

pganssle commented Jul 8, 2018

Pytzectomy#11360
tacaswell merged 10 commits into
matplotlib:masterfrom
pganssle:pytzectomy

pganssle commented Jun 1, 2018 •
edited
Loading

pganssle commented Jul 5, 2018 •
edited
Loading

pganssle commented Jul 5, 2018 •
edited
Loading