TST: Fail on warning #22699

TomAugspurger · 2018-09-13T20:03:15Z

Sets our pytest config to fail on unhandled warnings.

Fixes a bunch of tests to not fail.

This still has a few TODOs.

Closes #16481
Closes #19677
Closes #13962
Closes #22020

rth · 2018-09-13T22:23:08Z

Fixing all the warnings is definitely great!

About harcoding -Werror in setup.cfg that can lead to various issues (cf scikit-learn/scikit-learn#12043 for more details). Adding those flags in CI setup might be more robust. Among other things hardcoding such a flag in setup.cfg may lead to the test suite failing (without good reason) with older or not yet released versions of dependencies, when using PyPy or some other less common environments.

h-vetinari · 2018-09-13T22:26:33Z

If it turns all warnings into errors, it might close #13962 as well.

TomAugspurger · 2018-09-14T01:34:58Z

@rth thanks for the context, that's helpful. I think that elevating warnings to errors only on worker with the latest dependencies makes the most sense. I'll probably keep at this until things are passing, and then limit the elevation once things are passing.

If it turns all warnings into errors, it might close #13962 as well.

Thanks for the link. It should close that.

TomAugspurger · 2018-09-14T13:06:08Z

pandas/conftest.py

@@ -31,6 +32,16 @@ def pytest_addoption(parser):
                     help="Fail if a test is skipped for missing data file.")


+def pytest_collection_modifyitems(items):


I don't think we'll want this permanently. This is just to track down #22675 in case it happens on this branch.

Do we need this as part of the ultimate change or no?

I'm tempted to leave it in master for a week or two to see if we catch any of the failures in action. I've made myself a reminder to remove it.

sighhhhhhhhhhhhhhhhhhhhhhhhhhhh. we got one: https://travis-ci.org/pandas-dev/pandas/jobs/429657921#L2280, but that doesn't actually fail the right test, because the warning is in the file object's __del__, just prints out to stderr rather than raising an exception (to fail the test).

I'm going to do some debugging on the branch.

TomAugspurger · 2018-09-17T16:07:43Z

I wasn't able to get the suggestion of

error:::pandas[.*]

to work reliably. In my small test of

def test_warns():
    pd.Panel()
    pd.DataFrame({"A": [1, 2]}).ix[0]

the panel warning was not elevated to an error, but the .ix was. I didn't investigate too deeply.

This reverts commit cf60217.

TomAugspurger · 2018-09-17T20:36:10Z

Those interested in the ResourceWarning failures may want to take a look at cfb5ae2. I tried to squash all the places we may have had unclosed files sitting around. If I had to guess, either cfb5ae2#diff-0d7b5a2c72b4dfc11d80afe159d45ff8R389 or cfb5ae2#diff-9d7323cc6b0cfa3f4482bc8d4268966dR213 is going to be the one that fixes it, but who knows.

TomAugspurger · 2018-09-18T11:35:21Z

I think I've addressed all the inline comments

Sorry to push on this relatively quickly, but I want to get our CI in a healthy state again. If possible, I'd like to merge this today.

I currently have two followup issues for warnings that need some care

Handle FutureWarning from NumPy in Series Construction #22698 (Series ctor)
Unhandled warning from NumPy dev in median #22712 (median with all-NA slice)

@jreback can you clarify our current policy on "allowed failures"? I don't recall why they're in the allowed-failures section.

jreback

@TomAugspurger awesome effort here. This was for sure a sore point. Some minor comments.

jreback · 2018-09-18T11:37:03Z

doc/source/contributing.rst

+   with tm.assert_prodcues_warning(FutureWarning):
+       df.some_operation()
+
+We prefer this to the ``pytest.warns`` context manager because ours checks that the warning's


you may want to mention that we will actually fail if pytest.warns will fail the linter :>

jreback · 2018-09-18T11:39:41Z

pandas/core/arrays/integer.py

+                (dtype.startswith("Int") or dtype.startswith("UInt"))):
+            # Avoid DeprecationWarning from NumPy about np.dtype("Int64")
+            # https://github.com/numpy/numpy/pull/7476
+            dtype = dtype.lower()


oh I didn't know numpy did this:

In [1]: np.dtype('Int64') /Users/jreback/miniconda3/envs/pandas/bin/ipython:1: DeprecationWarning: Numeric-style type codes are deprecated and will result in an error in the future. #!/Users/jreback/miniconda3/envs/pandas/bin/python Out[1]: dtype('int64')

I guess so, thanks

jreback · 2018-09-18T11:40:34Z

pandas/core/groupby/generic.py

@@ -758,7 +758,7 @@ def aggregate(self, func_or_funcs, *args, **kwargs):
        if isinstance(func_or_funcs, compat.string_types):
            return getattr(self, func_or_funcs)(*args, **kwargs)

-        if isinstance(func_or_funcs, collections.Iterable):
+        if isinstance(func_or_funcs, compat.Iterable):


in theory we could add a lint rule to avoid using collection.Iterable, and instead just use our compat

I think we're close enough to py3-only that we'll be OK. Writing that regex would be a bit fiddly since we'd need to enumerate all the ABCs.

true, certainly a follow up issue is ok

jreback · 2018-09-18T11:41:14Z

pandas/core/internals/blocks.py

@@ -3490,6 +3490,7 @@ def _putmask_smart(v, m, n):

        # we ignore ComplexWarning here
        with warnings.catch_warnings(record=True):
+            warnings.simplefilter("ignore", np.ComplexWarning)


does the context manager restore all of the warnings filters after? (I guess that is the point?)

Yeah, that's the idea. Ideally libraries shouldn't modify the global warnings registry without good cause.

jreback · 2018-09-18T11:42:10Z

pandas/tests/api/test_api.py

@@ -175,23 +174,23 @@ def test_get_store(self):

 class TestParser(object):

+    @pytest.mark.filterwarnings("ignore")
    def test_deprecation_access_func(self):


should these actually be tm.assert_produces_warning(FutureWarning)?

nvm I see your comment below

jreback · 2018-09-18T11:44:07Z

pandas/tests/frame/test_indexing.py

@@ -1134,19 +1175,22 @@ def test_ix_frame_align(self):
        df = df_orig.copy()

        with catch_warnings(record=True):
+            simplefilter("ignore", DeprecationWarning)


grr....we should just remove all of this .ix bizness

jreback · 2018-09-18T11:45:34Z

pandas/tests/groupby/test_groupby.py

@@ -508,30 +507,30 @@ def test_frame_multi_key_function_list():




we could prob split the panel tests to a separate file (and just ignore warnings there), maybe make an issue

jreback · 2018-09-18T11:46:07Z

pandas/tests/indexes/datetimes/test_tools.py

-def epochs(request):
-    return request.param
+@pytest.fixture(params=['timestamp', 'pydatetime', 'datetime64', 'str_1960'])
+def epochs(epoch_1960, request):


can you add a doc-string

jreback · 2018-09-18T11:47:22Z

pandas/tests/indexing/test_floats.py

@@ -10,6 +10,9 @@
 import pandas.util.testing as tm


+ignore_ix = pytest.mark.filterwarnings("ignore:\\n.ix:DeprecationWarning")


maybe move this to the top-level conftest and use it above (a number of files where catching all of the ix warnings)

Hmm do you know how to use marks defined in the top-level conftest.py? I'm not actually sure that's possible.

i think you can just import it

jreback · 2018-09-18T11:47:45Z

pandas/tests/indexing/test_iloc.py

@@ -388,52 +388,61 @@ def test_iloc_getitem_frame(self):

        result = df.iloc[2]
        with catch_warnings(record=True):
+            filterwarnings("ignore", "\\n.ix", DeprecationWarning)


use ignore_ix?

That filters the warning for the entire test. I was trying to exactly match the tests previous intent, of only filtering within the catch_warnings blocks.

jreback · 2018-09-18T12:03:02Z

@jreback can you clarify our current policy on "allowed failures"? I don't recall why they're in the allowed-failures section.

so numpy-dev is allowed to fail because there may be a change which breaks us but is not pushed to public (ideally would fail, except for when numpy / dateutil breaks things everything breaks and all builds would then fail)
slow tests are there just to make the build green faster
clipboard test uses to fail pretty unexpectedly
doc build takes a long time.

so they are really there to get a probable green faster. I don't really have a problem moving them to required builds actually.

TomAugspurger · 2018-09-18T13:02:55Z

Thanks. So I think it's best to treat numpy / dateutil dev as "allowed failures" but as soon as we see a failure we should open an issue that would block the next release (and maybe report upstream if it looks like an unintentional break).

We might consider moving the doc build to azure. Apparently the workers more powerful than Travis.

jreback · 2018-09-18T13:03:48Z

Thanks. So I think it's best to treat numpy / dateutil dev as "allowed failures" but as soon as we see a failure we should open an issue that would block the next release (and maybe report upstream if it looks like an unintentional break).

yep, that's the idea

We might consider moving the doc build to azure. Apparently the workers more powerful than Travis.

certainly

jreback · 2018-09-18T13:04:06Z

@TomAugspurger merge when you are ready

* fixture docstrings * linter fails

TomAugspurger · 2018-09-18T13:04:59Z

👍 pushed some doc fixes. Will merge on green.

TomAugspurger · 2018-09-18T16:51:55Z

Travis all passed. Merging.

TST: Fail on warning

9480916

TomAugspurger added the Testing pandas testing functions or related to the test suite label Sep 13, 2018

TomAugspurger added this to the 0.24.0 milestone Sep 13, 2018

TomAugspurger mentioned this pull request Sep 13, 2018

BLD/IO: Elusive Travis ResourceWarning & closing files by default #22675

Closed

more

bd5a419

TomAugspurger added 13 commits September 13, 2018 20:50

Fixed base

36279c9

Merge remote-tracking branch 'upstream/master' into pytest-warnings

ad69f50

ABC compat

43d7a78

limit to dev

4919b0a

Explicit warnings

2804192

Fixed plotting check

7ad249a

Set for NumPy dev

953fde1

collections

d2dd0af

more collections, actually set

b813e41

Always error ResourceWarning

3baba66

DeprecationWarnings

3067ed1

Pop from sys.modules

614f514

redo resourcewarning

37a3d39

TomAugspurger commented Sep 14, 2018

View reviewed changes

TomAugspurger added 8 commits September 14, 2018 08:42

wip

80ce447

Panel

61beba7

more warnings

cf1ff63

lint

a9a672d

another

da4961a

some more

bed003b

future->depr

8bd8711

future->depr

ba46eef

fixups

83cd9ae

TomAugspurger added 4 commits September 17, 2018 11:40

Filter again

d10a0cc

print

cf60217

Revert "print"

61587ec

This reverts commit cf60217.

Close all the handles

cfb5ae2

TomAugspurger added 2 commits September 18, 2018 06:17

update contributing

2591677

Merge remote-tracking branch 'upstream/master' into pytest-warnings

1d39520

jreback requested changes Sep 18, 2018

View reviewed changes

jreback approved these changes Sep 18, 2018

View reviewed changes

Docs

5f0eefe

* fixture docstrings * linter fails

jreback mentioned this pull request Sep 18, 2018

BUG: fix numpy deprecation warning for scalar datetime64 values #22742

Closed

4 tasks

TomAugspurger added 2 commits September 18, 2018 10:01

Merge remote-tracking branch 'upstream/master' into pytest-warnings

7f618e9

Fixed merge conflict

4990fc2

TomAugspurger merged commit 1a12c41 into pandas-dev:master Sep 18, 2018

TomAugspurger deleted the pytest-warnings branch September 18, 2018 16:52

h-vetinari mentioned this pull request Sep 19, 2018

Set up CI with Azure Pipelines #22760

Merged

aeltanawy pushed a commit to aeltanawy/pandas that referenced this pull request Sep 20, 2018

TST: Fail on warning (pandas-dev#22699)

0ba7b16

Sup3rGeo pushed a commit to Sup3rGeo/pandas that referenced this pull request Oct 1, 2018

TST: Fail on warning (pandas-dev#22699)

253ed15

h-vetinari mentioned this pull request Nov 16, 2018

CI for boto: fix errors; add coverage; add skip for uncatchable ResourceWarning #23731

Merged

2 tasks

TomAugspurger mentioned this pull request Jan 30, 2019

Handle FutureWarning from NumPy in Series Construction #22698

Closed

WillAyd mentioned this pull request Jun 11, 2020

Removed __div__ impls from Cython #34718

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TST: Fail on warning #22699

TST: Fail on warning #22699

TomAugspurger commented Sep 13, 2018 •

edited

Loading

rth commented Sep 13, 2018

h-vetinari commented Sep 13, 2018

TomAugspurger commented Sep 14, 2018

TomAugspurger Sep 14, 2018

WillAyd Sep 17, 2018

TomAugspurger Sep 17, 2018

TomAugspurger Sep 17, 2018

TomAugspurger commented Sep 17, 2018

TomAugspurger commented Sep 17, 2018

TomAugspurger commented Sep 18, 2018

jreback left a comment

jreback Sep 18, 2018

jreback Sep 18, 2018

jreback Sep 18, 2018

TomAugspurger Sep 18, 2018

jreback Sep 18, 2018

jreback Sep 18, 2018

TomAugspurger Sep 18, 2018

jreback Sep 18, 2018

jreback Sep 18, 2018

jreback Sep 18, 2018

jreback Sep 18, 2018

jreback Sep 18, 2018

jreback Sep 18, 2018

TomAugspurger Sep 18, 2018

jreback Sep 18, 2018

jreback Sep 18, 2018

TomAugspurger Sep 18, 2018

jreback commented Sep 18, 2018

TomAugspurger commented Sep 18, 2018

jreback commented Sep 18, 2018

jreback commented Sep 18, 2018

TomAugspurger commented Sep 18, 2018

TomAugspurger commented Sep 18, 2018

		@@ -31,6 +32,16 @@ def pytest_addoption(parser):
		help="Fail if a test is skipped for missing data file.")


		def pytest_collection_modifyitems(items):

		@@ -508,30 +507,30 @@ def test_frame_multi_key_function_list():

		@@ -10,6 +10,9 @@
		import pandas.util.testing as tm


		ignore_ix = pytest.mark.filterwarnings("ignore:\\n.ix:DeprecationWarning")

TST: Fail on warning #22699

TST: Fail on warning #22699

Conversation

TomAugspurger commented Sep 13, 2018 • edited Loading

rth commented Sep 13, 2018

h-vetinari commented Sep 13, 2018

TomAugspurger commented Sep 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomAugspurger commented Sep 17, 2018

TomAugspurger commented Sep 17, 2018

TomAugspurger commented Sep 18, 2018

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Sep 18, 2018

TomAugspurger commented Sep 18, 2018

jreback commented Sep 18, 2018

jreback commented Sep 18, 2018

TomAugspurger commented Sep 18, 2018

TomAugspurger commented Sep 18, 2018

TomAugspurger commented Sep 13, 2018 •

edited

Loading