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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Pytzectomy #11360
Pytzectomy #11360
Conversation
I'm getting the same Travis failures from invoking I will note that I've actually really liked it ever since I started using |
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 While one could argue that you maybe shouldn't be counting on the fact that the timezones used by |
I'm still not sure how we feel about the |
.travis.yml
Outdated
@@ -195,6 +196,7 @@ before_script: | | |||
script: | | |||
echo "Calling pytest with the following arguments: $PYTEST_ADDOPTS" | |||
python -mpytest | |||
tox -e pytz |
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 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.
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.
No, tox -e pytz
only runs the pytz
environment, which runs pytest -m pytz
, and thus only runs tests decorated with pytest.mark.pytz
.
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.
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.
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 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.
|
||
from matplotlib.testing.decorators import image_comparison | ||
import matplotlib.pyplot as plt | ||
from matplotlib.cbook import MatplotlibDeprecationWarning | ||
import matplotlib.dates as mdates | ||
|
||
|
||
def __has_pytz(): |
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.
double underscore seems a bit overkill
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 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.
_test_rrulewrapper(attach_tz, dateutil.tz.gettz) | ||
|
||
|
||
@pytest.mark.pytz |
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.
What's this mark?
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.
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 think the In any case, I think it should probably not block this PR for the following reasons:
|
import pytz | ||
return True | ||
except ImportError: | ||
return False |
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.
Hm, disturbing that this line isn't being hit. Something is indirectly pulling in a pytz
dependency in the main test suite.
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 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 The problem is that the 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?
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 |
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. |
1a12144
to
7578b32
Compare
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 |
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.
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 |
Thanks @pganssle ! This was a PR that travis did not want to go in.... |
PR Summary
Closes #10443.
This removes
pytz
as a required dependency in favor ofdateutil.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 usedateutil
overpytz
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 thetest_rrulewrapper_pytz
covers both the most important corner case and the use ofpytz
on an axis), but it wouldn't be a bad idea to add a few more tests for that. I recommend making sure thatpytz
is not installed during the main tests, though (the way I set it up thepytz
-specific tests are run throughtox
so that it is only installed in the virtualenv), to avoid any accidental implicit dependencies that may arise.PR Checklist