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

h-vetinari · 2018-11-16T01:12:11Z

fixture modified / tests pass
passes git diff upstream/master -u -- "*.py" | flake8 --diff

EDIT2: The warning has been identified as being caused by a vendored requests from botocore<1.11, which is solved by raising the minimum version to 1.11 for the only CI job (travis-36) that is testing boto.

This would then simultaneously run into #23754 due to a moto bug (getmoto/moto#1924 / getmoto/moto#1941), but setting the environment variables AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY to any dummy value fixes the issue (taken from getmoto/moto#1952).

I'm also adding the boto tests to the travis-37 job, just to have some more coverage in general (and the travis-37 is by far the fastest job right now).

EDIT: The warning has been identified as being caused by a vendored requests from botocore<1.11. Unfortunately, it's not possible to (just) increase the minimum version, as botocore>=1.11 currently runs into #23754 due to a moto bug (getmoto/moto#1941), which would (once #24073 is merged) that these would just be skipped silently. Thus, I'm adding a the boto tests to the travis-37 build with botocore>=1.11, which will start working once #23754 is skipped, while still testing boto on the travis-36 job by forcing botocore<1.11.

pep8speaks · 2018-11-16T01:12:15Z

Hello @h-vetinari! Thanks for submitting the PR.

There are no PEP8 issues in the file pandas/io/excel.py !
There are no PEP8 issues in the file pandas/io/parquet.py !
There are no PEP8 issues in the file pandas/tests/io/conftest.py !
There are no PEP8 issues in the file pandas/tests/io/json/test_compression.py !
There are no PEP8 issues in the file pandas/tests/io/test_excel.py !
There are no PEP8 issues in the file pandas/tests/io/test_s3.py !

codecov · 2018-11-16T01:43:14Z

Codecov Report

Merging #23731 into master will increase coverage by 0.05%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #23731      +/-   ##
==========================================
+ Coverage   92.22%   92.28%   +0.05%     
==========================================
  Files         162      162              
  Lines       51824    51830       +6     
==========================================
+ Hits        47795    47830      +35     
+ Misses       4029     4000      -29

Flag	Coverage Δ
#multiple	`90.68% <100%> (+0.06%)`	⬆️
#single	`43.01% <16.66%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
pandas/util/testing.py	`87.48% <100%> (+0.06%)`	⬆️
pandas/io/common.py	`72.86% <0%> (+0.77%)`	⬆️
pandas/io/parquet.py	`84.61% <0%> (+7.69%)`	⬆️
pandas/io/s3.py	`86.36% <0%> (+86.36%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7b0fa8e...b532696. Read the comment docs.

jreback · 2018-11-16T12:49:25Z

pandas/io/excel.py

@@ -425,6 +426,13 @@ def __init__(self, io, **kwds):
            raise ValueError('Must explicitly set engine if not passing in'
                             ' buffer or path for io.')

+        if should_close:


can you move this to a function in pandas.io.common, call it

maybe_close_filepath(should_close, io) to make this code more conscise

h-vetinari · 2018-11-16T20:49:12Z

After an absurd amount of time trying to hunt down these warnings, I think I found the culprit/solution boto/botocore#1464.

The warning is from a vendored requests/urrlib3 in botocore, which didn't close a session/socket. Unfortunately, there are no means that I found (and I tried a lot) that can catch this warning. Failed attempts include:

warnings.catch_warnings() with simplefilter or filterwarning
capsys/capfd/capsysbinary/capsysfd fixtures from pytest
tm.capture_stderr and tm.capture_stdout
setting os.environ["PYTHONWARNINGS"]
passing -W ignore::ResourceWarning to the pytest call

The only thing that had an effect (but still didn't work) was -W error::ResourceWarning:

(pandas-dev) C:\[...]\pddev>pytest pandas/tests/io/test_parquet.py -W error::ResourceWarning
============================= test session starts =============================
[...]
========== 1 failed, 39 passed, 6 skipped, 2 xpassed in 6.35 seconds ==========
Exception ignored in: <socket.socket fd=2680, family=AddressFamily.AF_INET, type=SocketKind.SOCK_DGRAM, proto=0>
ResourceWarning: unclosed <socket.socket fd=2680, family=AddressFamily.AF_INET, type=SocketKind.SOCK_DGRAM, proto=0>

i.e. even more spurious output (the failure is platform-specific and not worth mentioning here).

After I upgraded to the latest botocore (>=1.11 is the cutoff), things are working fine. As such, I decided to just (try to) force the travis-36 job to load that, and skip the s3-tests otherwise. I restricted the skips to the case that PANDAS_TESTING_MODE = "deprecate", because otherwise, the ResourceWarnings are filtered out anyway.

h-vetinari · 2018-11-17T15:30:22Z

So, the last run had no warnings anymore (failure was again only #23726), but I realized that the reason for this is that now the s3_resource is just skipping through some more restrictive authentication checks in the newer boto (#23754).

h-vetinari · 2018-11-17T17:56:37Z

I'm trying to cover more boto by adding it to the travis-37 job (which is also around 10min faster than the others). Moto is supporting 3.7. starting with 1.3.7 (the latest version), but that isn't reflected in the requirements yet: getmoto/moto#1886. Therefore, I'm installing through pip (after failing through conda).

h-vetinari · 2018-11-17T18:58:23Z

OK, good news is that the ResourceWarnings are gone from the travis-36 job (https://travis-ci.org/pandas-dev/pandas/jobs/456410643), which, I guess is a nice and closed-off extent for this PR.

The errors now in the travis-37 job due to #23754 will still need to be solved, but I guess that's something for a follow-up.

jreback · 2018-11-17T19:15:57Z

pandas/tests/io/conftest.py

@@ -37,6 +41,14 @@ def s3_resource(tips_file, jsonl_file):
    """
    pytest.importorskip('s3fs')
    boto3 = pytest.importorskip('boto3')
+    botocore = pytest.importorskip('botocore')
+    if (LooseVersion(botocore.__version__) < LooseVersion("1.11.0")


just make the minimum in the tests 1.11 then u don’t need this at all

You mean add it to the travis-36 dependencies? That would currently fail due to #23754

Adding botocore>=1.11 to the dependencies will mean either failures due to #23754 (which is most likely an upstream moto-bug), or that none of the boto tests are actually run (because they'd be skipped). The travis-36 build is the only build testing boto.

With this construct (and I admit it's not pretty), we could have one build doing botocore<1.11 (actually testing the code), and one with botocore>=1.11, which would be silently skipping them now but will start working again as soon as the moto bug is fixed and a new version available.

I haven't been following the boto / moto issues closely, but it seems like this is the best option for now if we want to have any of the boto stuff actually tested.

botocore is only a test dep, so i don't mind switching it to a higher version. Then simply add this to other builds until we are actually testing this.

From what I understand there's a conflict between the latest boto/botocore and moto
#23754

We could also try to fix this at the PANDAS_TESTING_MODE level. That adds a warnings.simplefilter at the start of the test. Perhaps the fixture could add an ignore to our filters? Though maybe @h-vetinari already tried that.

jreback · 2018-11-17T22:07:30Z

pandas/tests/io/json/test_compression.py


-        with tm.ensure_clean() as path:


this was mocked before, why are you now non-mocking it? this causes permission issues.

No, it's still mocked, because the function now uses the s3_resource fixture, which does the mocking.

So now, all s3 tests use the fixture instead of doing their own mocking.

jreback · 2018-11-17T22:07:57Z

pandas/tests/io/test_excel.py

-            url_table = read_excel(url)
-            local_table = self.get_exceldf('test1', ext)
-            tm.assert_frame_equal(url_table, local_table)
+    def test_read_from_s3_url(self, ext, s3_resource):


same why are you not mocking this?

@jreback, same as above, the mocking is done in the s3_resource fixture that I added to the sig

h-vetinari · 2018-11-21T16:28:23Z

@jreback
I responded to your feedback couple days ago, PTAL

h-vetinari

@jreback @TomAugspurger
Currently, boto is only tested in one build (and even those would be skipped if I incorporated your feedback). Please see my alternate suggestion in the comment.

h-vetinari · 2018-11-21T21:30:01Z

pandas/tests/io/conftest.py

@@ -37,6 +41,14 @@ def s3_resource(tips_file, jsonl_file):
    """
    pytest.importorskip('s3fs')
    boto3 = pytest.importorskip('boto3')
+    botocore = pytest.importorskip('botocore')
+    if (LooseVersion(botocore.__version__) < LooseVersion("1.11.0")


Adding botocore>=1.11 to the dependencies will mean either failures due to #23754 (which is most likely an upstream moto-bug), or that none of the boto tests are actually run (because they'd be skipped). The travis-36 build is the only build testing boto.

With this construct (and I admit it's not pretty), we could have one build doing botocore<1.11 (actually testing the code), and one with botocore>=1.11, which would be silently skipping them now but will start working again as soon as the moto bug is fixed and a new version available.

h-vetinari · 2018-11-29T16:40:34Z

@TomAugspurger Would you mind opining on #23731 (comment)? :)

h-vetinari · 2018-11-29T17:17:40Z

@TomAugspurger Thanks!

@jreback Should you agree as well, don't merge quite yet - I still need to set up boto to be tested in another CI job (as explained in the comment above). Will wait for your input here

TomAugspurger · 2018-11-29T17:22:08Z

What do you mean by "another" CI job? Can we take an existing one and pin moto, boto, and botocore to known versions?

h-vetinari · 2018-11-29T17:27:20Z

That's of course what I meant... Sorry for the confusing choice of words.

h-vetinari · 2018-11-29T20:54:06Z

@TomAugspurger

From what I understand there's a conflict between the latest boto/botocore and moto
#23754

Yes, there seems to be an error with the newer moto that will hopefully be fixed soon.

We could also try to fix this at the PANDAS_TESTING_MODE level. That adds a warnings.simplefilter at the start of the test. Perhaps the fixture could add an ignore to our filters? Though maybe @h-vetinari already tried that.

The warning unfortunately cannot be caught by warnings.simplefilter (or anything else I could try, see #23731 (comment)), because AFAICT, it is emitted from a finally/teardown state, where the usual mechanics don't apply anymore.

@jreback

botocore is only a test dep, so i don't mind switching it to a higher version. Then simply add this to other builds until we are actually testing this.

It's actually an indirect (optional) dependency through boto3, which directly depends on it (in version lockstep: boto3 1.x.yy <-> botocore 1.[x+3].yy).

I added the newer moto to to the travis-37 job, where those boto tests should now produce errors (after re-verifying that, I'll then add a specific except to the fixture-teardown). I'll also add PANDAS_TESTING_MODE="deprecate" to make sure these boto-tests are tested for warnings (once moto is fixed upstream).

On the other hand, I'm forcing botocore<1.11 on the travis-36 job (and removing PANDAS_TESTING_MODE="deprecate"), to make sure boto is tested until #23754 is solved.

EDIT: clarification about shifting PANDAS_TESTING_MODE="deprecate"

jreback · 2018-12-02T21:53:43Z

can you rebase

h-vetinari · 2018-12-02T23:17:41Z

Failure in azure is unrelated.

h-vetinari · 2018-12-03T17:47:36Z

I had closed this to avoid being merged, as I saw that there were some things that still need ironing out (but had no time to comment at work).
I'll split off the part that deals with the s3_resource fixture in a separate PR and leave the CI stuff here.

h-vetinari · 2018-12-07T17:37:07Z

@TomAugspurger
5979389 seems to have fixed the moto import issue.

h-vetinari

@jreback @TomAugspurger
Right now, this fixes two moto issues (the import error that was hacked around by @TomAugspurger in #24092, and #23754), but the ResourceWarnings are back for some reasons (despite the newest boto/moto):
travis-37: https://travis-ci.org/pandas-dev/pandas/jobs/465618259
travis-36: https://travis-ci.org/pandas-dev/pandas/jobs/465618262

I can split off another PR or rename this one, but at the moment, boto tests are skipped everywhere due to #24092, so I think this should be merged soon.

h-vetinari · 2018-12-09T15:05:24Z

ci/deps/travis-36.yaml

  - hypothesis>=3.58.0
  - pip:
    - brotlipy
    - coverage
+    - moto


conda pulls in moto 1.1.1, which is way too old.

h-vetinari · 2018-12-09T15:06:21Z

pandas/tests/io/conftest.py

+    if LooseVersion(botocore.__version__) < LooseVersion("1.11.0"):
+        # botocore leaks an uncatchable ResourceWarning before 1.11.0;
+        # see GH 23731 and https://github.com/boto/botocore/issues/1464
+        pytest.skip("botocore is leaking resources before 1.11.0")


actually this skip is needed because travis-27 runs an older boto (I just didn't see it in the .yml because its a transitive dependency of s3fs).

h-vetinari · 2018-12-09T15:06:35Z

pandas/tests/io/conftest.py

@@ -37,6 +40,12 @@ def s3_resource(tips_file, jsonl_file):
    """
    pytest.importorskip('s3fs')
    boto3 = pytest.importorskip('boto3')
+
+    # temporary workaround as moto fails for botocore >= 1.11 otherwise


jreback · 2018-12-07T21:21:51Z

pandas/tests/io/conftest.py

    finally:
        s3.stop()
+        os.environ.setdefault("AWS_ACCESS_KEY_ID", None)


this is not correct, you need to reset it to what it was before. maybe just use an environment context manager here

h-vetinari · 2018-12-10T19:02:19Z

@jreback
Now using a contextmanager like you asked. It's green too.

h-vetinari · 2018-12-12T19:13:47Z

@jreback
This is green, and ready for review.

jreback

looks good. 1 small addition, ping on green.

ci/deps/travis-36.yaml

jreback · 2018-12-14T23:06:33Z

lgtm. ping on green.

h-vetinari · 2018-12-15T00:55:22Z

@jreback Green

jreback · 2018-12-15T18:04:27Z

thanks @h-vetinari

…rceWarning (pandas-dev#23731)

h-vetinari mentioned this pull request Nov 16, 2018

TST/CLN: Fix/clean pytables test #23732

Merged

jreback requested changes Nov 16, 2018

View reviewed changes

jreback added the IO Data IO issues that don't fit into a more specific label label Nov 16, 2018

h-vetinari force-pushed the fix_resource_warn branch from a4b6912 to 0de6e8e Compare November 16, 2018 19:35

h-vetinari mentioned this pull request Nov 17, 2018

TST/DEPS: new boto breaks tests #23754

Closed

jreback requested changes Nov 17, 2018

View reviewed changes

h-vetinari changed the title ~~TST/CLN: fix sys1:ResourceWarning due to open sockets (WIP)~~ TST/CLN: fix sys1:ResourceWarning due to open sockets Nov 18, 2018

h-vetinari mentioned this pull request Nov 19, 2018

CI: Fixing possible bugs in the CI #23727

Merged

h-vetinari commented Nov 21, 2018

View reviewed changes

h-vetinari mentioned this pull request Nov 26, 2018

TST: Run tests using a database explicitly #23928

Closed

TomAugspurger approved these changes Nov 29, 2018

View reviewed changes

h-vetinari changed the title ~~TST/CLN: fix sys1:ResourceWarning due to open sockets~~ TST/CLN/CI/DEP: use boto-fixture consistently, enable boto tests on travis-37; fix uncatchable ResourceWarning Nov 30, 2018

h-vetinari closed this Dec 3, 2018

h-vetinari reopened this Dec 3, 2018

h-vetinari added 4 commits December 6, 2018 17:54

Merge remote-tracking branch 'upstream/master' into fix_resource_warn

488d767

Merge remote-tracking branch 'upstream/master' into fix_resource_warn

186b215

Skip old botocore; don't skip on moto; undo env vars in finally

7682581

Try fix for moto import

5979389

h-vetinari added 2 commits December 9, 2018 15:12

Merge remote-tracking branch 'upstream/master' into fix_resource_warn

5ce301d

Install moto through pip

49ff454

h-vetinari commented Dec 9, 2018

View reviewed changes

jreback requested changes Dec 10, 2018

View reviewed changes

h-vetinari added 2 commits December 10, 2018 18:13

Merge remote-tracking branch 'upstream/master' into fix_resource_warn

0b58728

Add context manager for environment variables

eb1f65b

Merge remote-tracking branch 'upstream/master' into fix_resource_warn

ae0821d

jrbourbeau mentioned this pull request Dec 11, 2018

boto3 appears to be out of sync with moto; pin boto3 to earlier version dask/dask#4276

Merged

2 tasks

jreback added the CI Continuous Integration label Dec 13, 2018

jreback requested changes Dec 13, 2018

View reviewed changes

ci/deps/travis-36.yaml Show resolved Hide resolved

jreback added this to the 0.24.0 milestone Dec 13, 2018

h-vetinari added 2 commits December 14, 2018 23:48

Merge remote-tracking branch 'upstream/master' into fix_resource_warn

abb5d6a

Add dependencies in environment.yml

f660e40

h-vetinari changed the title ~~CI/DEP: increase boto coverage; add skip for uncatchable ResourceWarning~~ CI for boto: fix errors; add coverage; add skip for uncatchable ResourceWarning Dec 14, 2018

jreback approved these changes Dec 14, 2018

View reviewed changes

also change requirements-dev.txt

b532696

jreback merged commit c128f7f into pandas-dev:master Dec 15, 2018

h-vetinari deleted the fix_resource_warn branch December 15, 2018 19:04

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

CI for boto: fix errors; add coverage; add skip for uncatchable Resou…

fe6cc3a

…rceWarning (pandas-dev#23731)

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

CI for boto: fix errors; add coverage; add skip for uncatchable Resou…

69aada2

…rceWarning (pandas-dev#23731)

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

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

Conversation

h-vetinari commented Nov 16, 2018 • edited by jreback Loading

pep8speaks commented Nov 16, 2018

codecov bot commented Nov 16, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

h-vetinari commented Nov 16, 2018 • edited Loading

h-vetinari commented Nov 17, 2018 • edited Loading

h-vetinari commented Nov 17, 2018

h-vetinari commented Nov 17, 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

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

h-vetinari commented Nov 21, 2018

h-vetinari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h-vetinari commented Nov 29, 2018

h-vetinari commented Nov 29, 2018

TomAugspurger commented Nov 29, 2018

h-vetinari commented Nov 29, 2018 via email • edited Loading

h-vetinari commented Nov 29, 2018 • edited Loading

jreback commented Dec 2, 2018

h-vetinari commented Dec 2, 2018

h-vetinari commented Dec 3, 2018 • edited Loading

h-vetinari commented Dec 7, 2018

h-vetinari 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

h-vetinari commented Dec 10, 2018

h-vetinari commented Dec 12, 2018

jreback left a comment

Choose a reason for hiding this comment

jreback commented Dec 14, 2018

h-vetinari commented Dec 15, 2018

jreback commented Dec 15, 2018

h-vetinari commented Nov 16, 2018 •

edited by jreback

Loading

codecov bot commented Nov 16, 2018 •

edited

Loading

h-vetinari commented Nov 16, 2018 •

edited

Loading

h-vetinari commented Nov 17, 2018 •

edited

Loading

h-vetinari commented Nov 29, 2018 via email •

edited

Loading

h-vetinari commented Nov 29, 2018 •

edited

Loading

h-vetinari commented Dec 3, 2018 •

edited

Loading