Skip to content
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

Raise ImportWarning when loader.load_module() is used #70319

Closed
brettcannon opened this issue Jan 15, 2016 · 14 comments
Closed

Raise ImportWarning when loader.load_module() is used #70319

brettcannon opened this issue Jan 15, 2016 · 14 comments
Labels
interpreter-core type-bug

Comments

@brettcannon
Copy link
Member

@brettcannon brettcannon commented Jan 15, 2016

BPO 26131
Nosy @brettcannon, @ncoghlan, @vstinner, @ericsnowcurrently, @webknjaz, @MichaelAnckaert, @brandtbucher, @nanjekyejoannah
PRs
  • #22905
  • #23469
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2020-12-07.21:27:33.787>
    created_at = <Date 2016-01-15.21:44:04.899>
    labels = ['interpreter-core', 'type-bug']
    title = 'Raise ImportWarning when loader.load_module() is used'
    updated_at = <Date 2020-12-14.13:06:12.676>
    user = 'https://github.com/brettcannon'

    bugs.python.org fields:

    activity = <Date 2020-12-14.13:06:12.676>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2020-12-07.21:27:33.787>
    closer = 'brett.cannon'
    components = ['Interpreter Core']
    creation = <Date 2016-01-15.21:44:04.899>
    creator = 'brett.cannon'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 26131
    keywords = ['patch']
    message_count = 14.0
    messages = ['258333', '347561', '347580', '349284', '349286', '349287', '349295', '379389', '382548', '382796', '382799', '382801', '382874', '382986']
    nosy_count = 8.0
    nosy_names = ['brett.cannon', 'ncoghlan', 'vstinner', 'eric.snow', 'webknjaz', 'michaelanckaert', 'brandtbucher', 'nanjekyejoannah']
    pr_nums = ['22905', '23469']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue26131'
    versions = ['Python 3.6']

    @brettcannon
    Copy link
    Member Author

    @brettcannon brettcannon commented Jan 15, 2016

    Since loader.load_module() is documented as deprecated, we should consider raising an ImportWarning when it is used. That way when Python 2.7 support ends we can either remove it or have one more release where the various ImportWarnings turn into DeprecationWarnings and we finally clean up the import semantics to only be spec-based.

    @brettcannon brettcannon added interpreter-core type-bug labels Jan 15, 2016
    @nanjekyejoannah
    Copy link
    Contributor

    @nanjekyejoannah nanjekyejoannah commented Jul 9, 2019

    @brett Since this was opened like years ago. Before I work on it, I was wondering If it is still relevant.

    @brettcannon
    Copy link
    Member Author

    @brettcannon brettcannon commented Jul 9, 2019

    @joannah no clue if this ever happened. Could you check the code if DeprecationWarning is being raised yet?

    @MichaelAnckaert
    Copy link
    Mannequin

    @MichaelAnckaert MichaelAnckaert mannequin commented Aug 9, 2019

    I checked out the source (Lib/imp.py:219) and only see the docstring marking this method als Deprecated. No exceptions are being raised.

    I would like to work on this issue.

    @MichaelAnckaert
    Copy link
    Mannequin

    @MichaelAnckaert MichaelAnckaert mannequin commented Aug 9, 2019

    Clarification: the imp module shows a DeprecationWarning when imported. Was this what was meant?

    @nanjekyejoannah
    Copy link
    Contributor

    @nanjekyejoannah nanjekyejoannah commented Aug 9, 2019

    Feel free to open a PR.

    @brettcannon
    Copy link
    Member Author

    @brettcannon brettcannon commented Aug 9, 2019

    If you look at https://github.com/python/cpython/blob/master/Lib/importlib/_bootstrap.py and https://github.com/python/cpython/blob/master/Lib/importlib/_bootstrap_external.py you will notice a bunch of comments near all the calls to load_module(). Those are the places that should be somehow triggering a DeprecationWarning.

    I think there's a compatibility shim function which would need to raise an exception so that anyone who has implemented load_module() as the only way to handle loading get a warning (but I think that requires certain loaders to have appropriate support which hopefully they have already but someone needs to double-check). After that every implementation of load_module() should have a warning.

    @brettcannon
    Copy link
    Member Author

    @brettcannon brettcannon commented Oct 23, 2020

    A PR is now up. I ended up deprecating the load_module() methods in importlib itself and then raise ImportWarning in the import system itself when falling back to load_module().

    @brettcannon
    Copy link
    Member Author

    @brettcannon brettcannon commented Dec 4, 2020

    New changeset 2de5097 by Brett Cannon in branch 'master':
    bpo-26131: Deprecate usage of load_module() (GH-23469)
    2de5097

    @brandtbucher
    Copy link
    Member

    @brandtbucher brandtbucher commented Dec 9, 2020

    I'm seeing the following test failure locally on master (doesn't seem to be showing up in CI though). I'm not too familiar with this stuff, but it looks related to this change:

    ======================================================================
    FAIL: test_all (test.test___all__.AllTest) (module='distutils.command')
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "/home/bucher/src/cpython/Lib/test/test___all__.py", line 55, in check_all
        self.assertEqual(keys, all_set, "in module {}".format(modname))
      File "/home/bucher/src/cpython/Lib/contextlib.py", line 140, in __exit__
        next(self.gen)
      File "/home/bucher/src/cpython/Lib/test/support/warnings_helper.py", line 177, in _filterwarnings
        raise AssertionError("unhandled warning %s" % reraise[0])
    AssertionError: unhandled warning {message : ImportWarning('VendorImporter.exec_module() not found; falling back to load_module()'), category : 'ImportWarning', filename : '<frozen importlib._bootstrap>', lineno : 681, line : None}

    Should we be ignoring the new warning in this test as well? I'm still not sure why I seem to be the only one seeing it.

    @brettcannon
    Copy link
    Member Author

    @brettcannon brettcannon commented Dec 9, 2020

    What is "VendorImporter" (see the message of the ImportWarning)? That's not in the stdlib, so it looks like your system is injecting something via some .pth file or environment variable that doesn't define exec_module().

    @brandtbucher
    Copy link
    Member

    @brandtbucher brandtbucher commented Dec 9, 2020

    Yep, looks like that was it. Thanks!

    @webknjaz
    Copy link
    Mannequin

    @webknjaz webknjaz mannequin commented Dec 11, 2020

    What is "VendorImporter"

    @brett.cannon VendorImporter is something comping from pkg_resources. Because of an entangled traceback and the fact that it dynamically injects a vendored copy of six, it's hard to spot. Here's an upstream issue I filed about this warning: pypa/setuptools#2481.

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Dec 14, 2020

    I wrote benjaminp/six#343 to fix the six module. I would appreciate a review.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    interpreter-core type-bug
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants