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#23731jreback merged 13 commits intopandas-dev:masterfrom
Conversation
|
Hello @h-vetinari! Thanks for submitting the PR.
|
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
pandas/io/excel.py
Outdated
| raise ValueError('Must explicitly set engine if not passing in' | ||
| ' buffer or path for io.') | ||
|
|
||
| if should_close: |
There was a problem hiding this comment.
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
a4b6912 to
0de6e8e
Compare
|
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:
The only thing that had an effect (but still didn't work) was 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 |
|
I'm trying to cover more boto by adding it to the |
|
OK, good news is that the ResourceWarnings are gone from the The errors now in the |
pandas/tests/io/conftest.py
Outdated
| pytest.importorskip('s3fs') | ||
| boto3 = pytest.importorskip('boto3') | ||
| botocore = pytest.importorskip('botocore') | ||
| if (LooseVersion(botocore.__version__) < LooseVersion("1.11.0") |
There was a problem hiding this comment.
just make the minimum in the tests 1.11 then u don’t need this at all
There was a problem hiding this comment.
You mean add it to the travis-36 dependencies? That would currently fail due to #23754
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| conn = boto3.resource("s3", region_name="us-east-1") | ||
| bucket = conn.create_bucket(Bucket="pandas-test") | ||
|
|
||
| with tm.ensure_clean() as path: |
There was a problem hiding this comment.
this was mocked before, why are you now non-mocking it? this causes permission issues.
There was a problem hiding this comment.
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.
| 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): |
There was a problem hiding this comment.
same why are you not mocking this?
There was a problem hiding this comment.
@jreback, same as above, the mocking is done in the s3_resource fixture that I added to the sig
|
@jreback |
h-vetinari
left a comment
There was a problem hiding this comment.
@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.
pandas/tests/io/conftest.py
Outdated
| pytest.importorskip('s3fs') | ||
| boto3 = pytest.importorskip('boto3') | ||
| botocore = pytest.importorskip('botocore') | ||
| if (LooseVersion(botocore.__version__) < LooseVersion("1.11.0") |
There was a problem hiding this comment.
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.
|
@TomAugspurger Would you mind opining on #23731 (comment)? :) |
|
@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 |
|
What do you mean by "another" CI job? Can we take an existing one and pin moto, boto, and botocore to known versions? |
|
That's of course what I meant... Sorry for the confusing choice of words.
|
Yes, there seems to be an error with the newer moto that will hopefully be fixed soon.
The warning unfortunately cannot be caught by
It's actually an indirect (optional) dependency through I added the newer moto to to the On the other hand, I'm forcing EDIT: clarification about shifting |
|
can you rebase |
|
Failure in azure is unrelated. |
|
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). |
|
@TomAugspurger |
h-vetinari
left a comment
There was a problem hiding this comment.
@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.
| - pip: | ||
| - brotlipy | ||
| - coverage | ||
| - moto |
There was a problem hiding this comment.
conda pulls in moto 1.1.1, which is way too old.
| 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") |
There was a problem hiding this comment.
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).
pandas/tests/io/conftest.py
Outdated
| pytest.importorskip('s3fs') | ||
| boto3 = pytest.importorskip('boto3') | ||
|
|
||
| # temporary workaround as moto fails for botocore >= 1.11 otherwise |
pandas/tests/io/conftest.py
Outdated
| pytest.skip("failure to use s3 resource") | ||
| finally: | ||
| s3.stop() | ||
| os.environ.setdefault("AWS_ACCESS_KEY_ID", None) |
There was a problem hiding this comment.
this is not correct, you need to reset it to what it was before. maybe just use an environment context manager here
|
@jreback |
|
@jreback |
jreback
left a comment
There was a problem hiding this comment.
looks good. 1 small addition, ping on green.
|
lgtm. ping on green. |
|
@jreback Green |
|
thanks @h-vetinari |
closes #23680
closes #23754
git diff upstream/master -u -- "*.py" | flake8 --diffEDIT2: The warning has been identified as being caused by a vendored
requestsfrombotocore<1.11, which is solved by raising the minimum version to1.11for the only CI job (travis-36) that is testingboto.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_IDandAWS_SECRET_ACCESS_KEYto any dummy value fixes the issue (taken from getmoto/moto#1952).I'm also adding the
bototests to thetravis-37job, just to have some more coverage in general (and thetravis-37is by far the fastest job right now).EDIT: The warning has been identified as being caused by a vendoredrequestsfrombotocore<1.11. Unfortunately, it's not possible to (just) increase the minimum version, asbotocore>=1.11currently 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 thetravis-37build withbotocore>=1.11, which will start working once #23754 is skipped, while still testingbotoon thetravis-36job by forcingbotocore<1.11.