Skip to content

Switch from deprecated imp to importlib #5468

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

Merged
merged 1 commit into from
Jun 24, 2019
Merged

Switch from deprecated imp to importlib #5468

merged 1 commit into from
Jun 24, 2019

Conversation

asottile
Copy link
Member

@asottile asottile commented Jun 22, 2019

Resolves #1403
Resolves #2761
Resolves #5078
Resolves #5432
Resolves #5433

@nicoddemus
Copy link
Member

Awesome, thanks! This is really great, that imp machinery was as old as it gets.

I will try to take a look ASAP, possibly tomorrow.

@asottile
Copy link
Member Author

oops this isn't ready yet, I broke package imports somehow? need to look more into that 👍

@nicoddemus
Copy link
Member

Ahh OK. 👍

@codecov
Copy link

codecov bot commented Jun 22, 2019

Codecov Report

Merging #5468 into master will increase coverage by 0.03%.
The diff coverage is 98.16%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5468      +/-   ##
==========================================
+ Coverage   96.03%   96.07%   +0.03%     
==========================================
  Files         115      115              
  Lines       25512    25458      -54     
  Branches     2480     2468      -12     
==========================================
- Hits        24501    24458      -43     
+ Misses        706      697       -9     
+ Partials      305      303       -2
Impacted Files Coverage Δ
testing/test_assertion.py 97.53% <ø> (ø) ⬆️
testing/acceptance_test.py 98.01% <100%> (+0.02%) ⬆️
src/_pytest/pytester.py 90.96% <100%> (-0.06%) ⬇️
testing/test_assertrewrite.py 84.03% <100%> (+0.24%) ⬆️
src/_pytest/main.py 96.91% <100%> (-0.03%) ⬇️
testing/test_pathlib.py 100% <100%> (ø) ⬆️
src/_pytest/pathlib.py 89.08% <100%> (+0.12%) ⬆️
src/_pytest/assertion/rewrite.py 94.83% <95.65%> (+1.34%) ⬆️

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 3d01dd3...4cd08f9. Read the comment docs.

@asottile
Copy link
Member Author

ok I think this works now

return tp == imp.PKG_DIRECTORY
_, _, modname = name.rpartition(".")
spec = self._find_dotted_spec(name)
return spec and spec.loader.is_package(modname)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is by far my least favorite part of this patch :S -- I couldn't find a better way to do this though

@asottile
Copy link
Member Author

fairly certain the flakey failures are related to this patch 🤔 -- but I'm not sure where I need to put the importlib.invalidate_caches() call

@asottile
Copy link
Member Author

with pytest-randomly installed, this fails pretty reliably (like ~75-80% of the time)

pytest testing/python/integration.py -k test_mock

still not sure what's up with it though :/

@asottile
Copy link
Member Author

oh but that reproduction isn't even valid, randomly is causing the assertion ordering to be changed 🤔

this is a tough one :S

@asottile
Copy link
Member Author

OK, I figured this out and added another test for another github issue (validated it failed on macos before my change)

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work @asottile! Please take a look at my comments and let me know what you think.

PYTEST_TAG = "{}-{}{}-PYTEST".format(impl, ver[0], ver[1])
del ver, impl

PYTEST_TAG = "{}-PYTEST".format(sys.implementation.cache_tag)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From https://docs.python.org/3/library/sys.html#sys.implementation:

If cache_tag is set to None, it indicates that module caching should be disabled.

So we need to account for that. Because of this we should also add some tests where we execute a few tests in a subprocess with bytecode caching disabled to ensure we don't regress.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems not a regression here -- would be happy to schedule this as a followup though (I believe there's already a check below for sys.dont_write_bytecode or whatever the flag is, so I suspect this being None doesn't matter (the string formatting still succeeds))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm I recall we had a py27-xdist-nobyte run or something were bytecode was disabled. I don't think we have that anymore, so we might introduce regressions by mistake.

I don't believe we need an entire tox environment for that, but we might consider one test which executes a few tests in a subprocess to ensure we don't regress.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are absolutely right 😉

def test_dont_write_bytecode(self, testdir, monkeypatch):
testdir.makepyfile(
"""
import os
def test_no_bytecode():
assert "__pycache__" in __cached__
assert not os.path.exists(__cached__)
assert not os.path.exists(os.path.dirname(__cached__))"""
)
monkeypatch.setenv("PYTHONDONTWRITEBYTECODE", "1")
assert testdir.runpytest_subprocess().ret == 0

@nicoddemus
Copy link
Member

Does this fix #2419 too?

@asottile
Copy link
Member Author

Does this fix #2419 too?

I'm not sure, it wasn't clear to me how to reproduce that one

@nicoddemus
Copy link
Member

nicoddemus commented Jun 24, 2019

I'm not sure, it wasn't clear to me how to reproduce that one

No problem, we should make a note or something to post a question to the OP if they can reproduce the issue with 5.0 (once released). 👍

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, awesome work!

@asottile
Copy link
Member Author

let me comb through the --pyargs issues, I think I fixed a few there as well

@asottile
Copy link
Member Author

note: this will not be backported as we can't support the same functionality in python2 / python3.4 due to missing importlib apis

@asottile asottile merged commit 61dcb84 into pytest-dev:master Jun 24, 2019
@asottile asottile deleted the switch_importlib_to_imp branch June 24, 2019 18:22
@falzm
Copy link

falzm commented Jul 1, 2019

Hi, unless I'm mistaken this change has been merged and released in version 5.0.0, however I'm still having the deprecation warning message using this version... Am I missing something? 🤔

=============================================================================================== test session starts ===============================================================================================
platform darwin -- Python 3.7.3, pytest-5.0.0, py-1.8.0, pluggy-0.12.0
rootdir: /Users/marc/Documents/Exoscale/git/python-exoscale
collected 1 item

tests/test_compute.py .                                                                                                                                                                                     [100%]

================================================================================================ warnings summary =================================================================================================
tests/test_compute.py::TestCompute::test_list_zones
  /Users/marc/Documents/Exoscale/git/python-exoscale/.venv/lib/python3.7/distutils/__init__.py:1: DeprecationWarning: the imp module is deprecated in favour of importlib; see the module's documentation for alternative uses
    import imp

tests/test_compute.py::TestCompute::test_list_zones
tests/test_compute.py::TestCompute::test_list_zones
  /Users/marc/Documents/Exoscale/git/python-exoscale/.venv/lib/python3.7/site-packages/botocore/vendored/requests/packages/urllib3/_collections.py:1: DeprecationWarning: Using or importing the ABCs from 'collections' instead of from 'collections.abc' is deprecated, and in 3.8 it will stop working
    from collections import Mapping, MutableMapping

-- Docs: https://docs.pytest.org/en/latest/warnings.html
====================================================================================== 1 passed, 3 warnings in 0.40 seconds =======================================================================================

@nicoddemus
Copy link
Member

Hi @falzm,

import imp is coming from distutils, not from pytest:

 /Users/marc/Documents/Exoscale/git/python-exoscale/.venv/lib/python3.7/distutils/__init__.py:1: DeprecationWarning: the imp module is deprecated in favour of importlib; see the module's documentation for alternative uses

I suggest passing -Werror:imp module in the command line and seeing who's importing distutils based on the traceback.

@falzm
Copy link

falzm commented Jul 1, 2019

Thank you @nicoddemus.

@csjmac
Copy link

csjmac commented Oct 12, 2019

Hoping this is the right place to post this...if not please help guide me to the right place?

I just stood up a brand new pyramid project using cookiecutter starter for 1.10 and this is still a thing: https://docs.pytest.org/en/latest/changelog.html#id68

My env (baz is the pyramid project folder):

platform darwin -- Python 3.7.4, pytest-5.2.1, py-1.8.0, pluggy-0.13.0 rootdir: /Users/me/code/foo/bar/baz, inifile: pytest.ini, testpaths: baz plugins: cov-2.8.1

  • MacOS Mojave 10.14.6
  • Python 3.7.4
  • Pyramid 1.10.4
  • pytest 5.2.1
  • py 1.8.0
  • pluggy 0.13.0
  • cov 2.8.1

I am executing pytest using a local setup.sh file, which includes running the tests:

python3 -m venv env
env/bin/pip install --upgrade pip setuptools
env/bin/pip install --upgrade -e ".[testing]"
(alembic stuff)
env/bin/initialize_baz_db development.ini
env/bin/pytest

The 2 default pyramid tests pass but I get a warnings summary with 3 warnings:

env/lib/python3.7/site-packages/venusian/__init__.py:1
  /Users/me/code/foo/bar/baz/env/lib/python3.7/site-packages/venusian/__init__.py:1: DeprecationWarning: the imp module is deprecated in favour of importlib; see the module's documentation for alternative uses
    import imp

According to all of the docs I found, this should not be possible because this was fixed in pytest 5.0.0 however, I can see there is a reference to a PR from Aug (~2months ago) that indicate that maybe there is still some imp in use.

Workarounds I tried (but did not fix the problem):

  • running pytest outside of my shell script
  • deleting .pytest_cache dir and trying again
  • confirming package deps
  • blowing away my venv and starting over
  • forcing the update/upgrade of any/all suspect packages

...but the results are always the same.

For the first warning, caused by venusian pip show gives me this:

⋙ env/bin/pip show venusian
Name: venusian
Version: 1.2.0
Summary: A library for deferring decorator actions
Home-page: https://pylonsproject.org
Author: Chris McDonough, Agendaless Consulting
Author-email: [email protected]
License: BSD-derived (http://www.repoze.org/LICENSE.txt)
Location: /Users/me/code/foo/bar/baz/env/lib/python3.7/site-packages
Requires: 
Required-by: pyramid

It strikes me as funny that this warning is still a thing, but, I stopped laughing a little while ago now. Perhaps I'm being a perfectionist in spending time on making this warning go away but it would be really tight to have this sorted before I dive into full TDD so I can see clean output for what is essentially 2 hello word test cases.

Thank you to everyone who read this far - you're the best.

(edits made to clarify original post, and, formatting for readability)

@asottile
Copy link
Member Author

you'll want to report that to the venusian project

@nicoddemus
Copy link
Member

Just to complement @asottile's answer, pytest is warning about imp on venusian project, not on pytest itself (which has been fixed some time ago, as you have found).

Notice that the warning location points to venusian/__init__.py:

env/lib/python3.7/site-packages/venusian/__init__.py:1
  .../site-packages/venusian/__init__.py:1: DeprecationWarning: the imp module is deprecated in favour of importlib; see the module's documentation for alternative uses
    import imp

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment