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

Implementation for PEP 451 (importlib.machinery.ModuleSpec) #63064

Closed
ericsnowcurrently opened this issue Aug 28, 2013 · 66 comments
Closed

Implementation for PEP 451 (importlib.machinery.ModuleSpec) #63064

ericsnowcurrently opened this issue Aug 28, 2013 · 66 comments
Assignees
Labels
deferred-blocker interpreter-core (Objects, Python, Grammar, and Parser dirs) stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@ericsnowcurrently
Copy link
Member

BPO 18864
Nosy @brettcannon, @terryjreedy, @ncoghlan, @larryhastings, @PCManticore, @ericsnowcurrently, @berkerpeksag, @phmc
Dependencies
  • bpo-19697: Document the possible values for main.spec
  • bpo-19700: Update runpy for PEP 451
  • bpo-19701: Update multiprocessing for PEP 451
  • bpo-19703: Update pydoc to PEP 451
  • bpo-19704: Update test.test_threaded_import to PEP 451
  • bpo-19705: Update test.test_namespace_pkgs to PEP 451
  • bpo-19706: Check if inspect needs updating for PEP 451
  • bpo-19707: Check if unittest.mock needs updating for PEP 451
  • bpo-19708: Check pkgutil for anything missing for PEP 451
  • bpo-19709: Check pythonrun.c is fully using PEP 451
  • bpo-19710: Make sure documentation for PEP 451 is finished
  • bpo-19711: add test for changed portions after reloading a namespace package
  • bpo-19712: Make sure there are exec_module tests for _LoaderBasics subclasses
  • bpo-19713: Deprecate various things in importlib thanks to PEP 451
  • bpo-19714: Add tests for importlib.machinery.WindowsRegistryFinder
  • bpo-21098: Address remaining PEP 451-related to-do comments.
  • bpo-21099: Switch applicable importlib tests to use PEP 451 API
  • Files
  • modulespec-initial.diff
  • import-system-reference.diff
  • modulespec-primary-changes.diff
  • issue18864-has-location-setter.diff
  • 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 = 'https://github.com/ericsnowcurrently'
    closed_at = <Date 2016-05-12.14:37:11.280>
    created_at = <Date 2013-08-28.06:56:18.238>
    labels = ['interpreter-core', 'deferred-blocker', 'type-feature', 'library']
    title = 'Implementation for PEP 451 (importlib.machinery.ModuleSpec)'
    updated_at = <Date 2016-05-12.14:37:11.278>
    user = 'https://github.com/ericsnowcurrently'

    bugs.python.org fields:

    activity = <Date 2016-05-12.14:37:11.278>
    actor = 'eric.snow'
    assignee = 'eric.snow'
    closed = True
    closed_date = <Date 2016-05-12.14:37:11.280>
    closer = 'eric.snow'
    components = ['Interpreter Core', 'Library (Lib)']
    creation = <Date 2013-08-28.06:56:18.238>
    creator = 'eric.snow'
    dependencies = ['19697', '19700', '19701', '19703', '19704', '19705', '19706', '19707', '19708', '19709', '19710', '19711', '19712', '19713', '19714', '21098', '21099']
    files = ['31959', '32381', '32457', '33073']
    hgrepos = []
    issue_num = 18864
    keywords = ['patch']
    message_count = 66.0
    messages = ['196352', '198981', '201210', '201212', '201224', '201244', '201369', '201399', '201604', '201605', '201946', '202456', '202461', '202482', '202483', '202655', '202656', '202657', '202658', '202659', '202660', '202661', '202662', '202663', '202664', '202669', '202672', '202673', '202676', '202681', '202695', '202710', '202711', '202712', '202917', '202918', '203329', '203340', '203411', '203578', '203649', '203800', '203801', '203810', '203811', '203825', '203831', '203835', '203843', '205384', '205520', '205575', '205576', '205769', '205822', '205881', '207509', '207510', '210927', '210957', '215152', '221905', '221932', '222289', '236537', '265408']
    nosy_count = 11.0
    nosy_names = ['brett.cannon', 'terry.reedy', 'ncoghlan', 'larry', 'Arfrever', 'Claudiu.Popa', 'BreamoreBoy', 'python-dev', 'eric.snow', 'berker.peksag', 'pconnell']
    pr_nums = []
    priority = 'deferred blocker'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue18864'
    versions = ['Python 3.4']

    @ericsnowcurrently
    Copy link
    Member Author

    This ticket will track the implementation for PEP-451 (ModuleSpec). I'll have a patch up in the next couple days.

    @ericsnowcurrently ericsnowcurrently self-assigned this Aug 28, 2013
    @ericsnowcurrently ericsnowcurrently added interpreter-core (Objects, Python, Grammar, and Parser dirs) stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Aug 28, 2013
    @ericsnowcurrently
    Copy link
    Member Author

    Here's an initial patch. Some key things left to do:

    • unit tests
    • documentation
    • implement exec_module() for the various importlib loaders.

    Once that's squared away there are further things that will be addressed in new tickets (or at least separate patches). This includes:

    • remove init_module_attrs() and module_to_load()
    • deprecations
    • clear a bunch of helper functions out of _bootstrap.py
    • use ModuleSpec with __main__
    • address impact on stdlib (pkgutil, pickle, etc.)

    @ericsnowcurrently
    Copy link
    Member Author

    Here's a patch just for the changes to the import page in the language reference. It's not perfect, but should represent the target pretty closely.

    @ericsnowcurrently
    Copy link
    Member Author

    Let's try that again with the proper diff. :)

    @ncoghlan
    Copy link
    Contributor

    Thanks, that looks pretty good - so much less work for loaders to do.

    After this, we may even be able to finally fix the circular import bug and the one where submodule imports succeed but the overall package import fails.

    @brettcannon
    Copy link
    Member

    I've added a review

    @ncoghlan
    Copy link
    Contributor

    Something else we may be able to fix (albeit probably not in 3.4): several of the issues noted in PEP-395 (since __main__.__spec__.name will give the real module name, even when __name__ is set to "__main__" or otherwise modified)

    @ericsnowcurrently
    Copy link
    Member Author

    Here's an update of the language reference patch based on Brett's comments.

    @ericsnowcurrently
    Copy link
    Member Author

    Here's an updated patch that implements the meat of the current PEP. Docs are still lacking and I could probably add a few more tests. Other key things left to do:

    • deprecations and removals
    • refactor importlib finders and loaders to use the new Finder/Loader APIs
    • refactor pythonrun.c to make use of __spec__
    • adjust other APIs to use __spec__ (pickle, etc.)

    @ericsnowcurrently
    Copy link
    Member Author

    This time against tip. :)

    I should also point out that this patch is on top of the path in bpo-19413.

    @ericsnowcurrently
    Copy link
    Member Author

    Here's a new patch that is mostly up to date with the PEP. I still need to work on Docs and add more tests. There are a few failing tests (due to the recent reload patch) that I need to fix when I get a minute.

    This patch also implements find_spec() on FileFinder and PathFinder. Left to do:

    • deprecations and removals
    • refactor importlib loaders to use the new Finder/Loader APIs
    • refactor pythonrun.c to make use of __spec__
    • adjust other APIs to use __spec__ (pickle, etc.)

    @ericsnowcurrently
    Copy link
    Member Author

    I've created a server-side clone for the implementation:

    http://hg.python.org/features/pep-451/

    @ericsnowcurrently
    Copy link
    Member Author

    Okay, I've updated the PEP-451 branch in the clone to include as much of the implementation as I've completed, which is the bulk of the functional changes. It's enough to pass the test suite.

    Here's what I'd like to get done before the feature freeze (in this priority order):

    1. wrap up the current functional changes in the clone;
    2. change module.__initializing__ to module.__spec__._initializing;
    3. refactor importlib loaders to use the new Finder/Loader APIs;
    4. refactor pythonrun.c to make use of __spec__;
    5. implement the deprecations and removals;
    6. adjust other APIs to use __spec__ (pickle, etc.);
    7. add rudimentary doc additions for the new APIs.

    Other things that can (but don't have to) wait until after the beta release:

    • finish doc changes;
    • fill in gaps in test coverage (there shouldn't be much due to Brett's mother of all test suites for importlib)

    I haven't had a chance yet to make any changes to Doc/reference/import.rst in response to Brett's review, but I did make the Doc/library/importlib.rst changes he recommended.

    @brettcannon
    Copy link
    Member

    I don't quite know what you mean by "current functional changes in the clone". Is this changes to import to use exec_module() and such? Just a guess since step 3 suggests there are no loaders implementing the new API currently.

    As for the tests, I'm hoping that simply refactoring some of them will allow reusing a bunch of the finder & loader tests instead of having to do it from scratch.

    @brettcannon
    Copy link
    Member

    I should clarify why I want a clarification on step 1. On Friday my current plan (barring other bugs I need to squash for b1) is to just start working on the pep-451 repo and I want a TODO list to follow in this issue so I don't have to waste time trying to figure out what to work on next. That's why I want it spelled out if we want the loaders fixed first or import, etc.

    @ericsnowcurrently
    Copy link
    Member Author

    Sorry that wasn't more clear. I committed the changes from the modulespec-primary-changes.diff patch to the PEP-451 branch in the server-side clone. Those changes are:

    Step 1
    ------

    1. added ModuleSpec class
    2. added _SpecMethods wrapper class
    3. added ModuleSpec factory functions
    4. added tests for ModuleSpec, _SpecMethods, and the factories
    5. exposed ModuleSpec in importlib.machinery
    6. exposed ModuleSpec factories in importlib.util
    7. added basic docs for ModuleSpec and the factory functions

    Step 2
    ------

    1. changed _find_module() to _find_spec()
    2. changed _find_and_load_unlocked() to use _find_spec() and _SpecMethods
    3. changed _setup() to use specs
    4. changed pydoc to recognize __spec__

    Step 3
    ------

    1. updated the import reference doc
    2. changed importlib.reload() to use specs
    3. added importlib.find_spec()
    4. changed importlib.find_loader() to wrap find_spec()
    5. updated importlib.abc to reflect the new APIs
    6. changed pkgutil to use specs
    7. changed imp to use specs
    8. fixed a bunch of broken tests to use spec

    Step 3
    ------

    1. implemented find_spec() on PathFinder
    2. implemented find_spec() on FileFinder
    3. re-implemented FileFinder.find_loader() to wrap find_spec()
    4. re-implemented PathFinder.find_module() to wrap find_spec()
    5. changed _NamespacePath to use specs

    Step 5
    ------

    1. added _module_repr function
    2. changed ModuleType.__repr__ to wrap _module_repr

    Others
    ------

    • removed _NamespaceLoader
    • added comments indicating deprecations and removals

    At this point, the test suite passes and the fundamental changes of the PEP are implemented (on the server-side clone). Here's what's left to do before the feature freeze:

    1. change module.__initializing__ to module.__spec__._initializing
    2. refactor importlib loaders to use the new Finder/Loader APIs
    3. refactor pythonrun.c to make use of specs
    4. check pkgutil for any missed changes
    5. implement the deprecations and removals
    6. adjust other APIs to use __spec__ (pickle, runpy, inspect, pydoc, others?)
    7. evaluate any impact on setuptools

    Other things that can (but don't have to) wait until after the beta release:

    • finish doc changes
    • fill in any gaps in test coverage
    • ensure new docstrings exist and are correct
    • ensure existing docstrings are still correct

    @ericsnowcurrently
    Copy link
    Member Author

    "1." was intentionally left blank <wink>

    @ericsnowcurrently
    Copy link
    Member Author

    Regarding tests, a bunch of importlib (and other?) tests make direct calls to find_module(), find_loader(), or load_module(). These are still working, as expected. However, we should probably either replace them or supplement them with equivalent tests that make use of specs directly.

    @ncoghlan
    Copy link
    Contributor

    Yeah, don't replace any tests, add new ones for the new APIs. Given the
    needs of 2/3 compatible loader implementations, the deprecations referred
    to in the PEP should also be documentation-only for 3.4.

    A more conservative approach also gives us a chance to make sure we have
    provided a full replacement for load_module - it occurred to me after the
    PEP was accepted that we may eventually need a "can_load_into(target)"
    loader API after all, since loaders may not be finder specific. That means
    that with the current API of passing the target to find_spec, I potentially
    talked us into repeating the "load_module" mistake on the finder side of
    things by asking the finder to handle more than it needed to.

    @ericsnowcurrently
    Copy link
    Member Author

    Yeah, don't replace any tests, add new ones for the new APIs.

    That's what makes the most sense to me too.

    Given the
    needs of 2/3 compatible loader implementations, the deprecations referred
    to in the PEP should also be documentation-only for 3.4.

    I'm fine with that. I seem to remember one place where an actual deprecation warning would be good, but I'll have to double-check.

    A more conservative approach also gives us a chance to make sure we have
    provided a full replacement for load_module - it occurred to me after the
    PEP was accepted that we may eventually need a "can_load_into(target)"
    loader API after all, since loaders may not be finder specific. That means
    that with the current API of passing the target to find_spec, I potentially
    talked us into repeating the "load_module" mistake on the finder side of
    things by asking the finder to handle more than it needed to.

    Nice analogy. "can_load_into()" makes sense. Do you think it's important enough to include in 3.4? I'm not sure it is, since I'd expect it to be a pretty uncommon case that can be worked around pretty easily (since the finder is fully aware of the loader it's creating). The extra loader method would help with that boilerplate operation, but...

    As we found out (and you expounded) there are a variety of reload/load-into cases that we could address more explicitly. Perhaps there's a better API that could address those needs more broadly, or maybe they're just not worth addressing specifically. Exploring all this is something that can wait, IMHO.

    @ncoghlan
    Copy link
    Contributor

    Although, a boolean query method would bring back the problem of the loader
    not reporting any details on *why* it can't load into a particular target
    module. So it may be better to have an optional loader
    "check_existing_target" API that throws a suitable exception if the target
    is unacceptable. Loaders that don't support reloading at all would just
    always raise an exception, while those that don't care would just not
    implement the method.

    That would address my concern about the lack of useful error information in
    Eric's original boolean check idea, without bothering finders with loader
    related details as the accepted PEP does (I still like the idea of passing
    a target to importlib.find_spec - I just no longer think we should be
    passing that down to the finders themselves).

    Another thing we need to check we have a test for: ensuring reloading a
    namespace module picks up new directories added since it was first loaded.

    This would all be so much easier if reloading wasn't supported in the first
    place :)

    @ncoghlan
    Copy link
    Contributor

    On 12 Nov 2013 09:36, "Eric Snow" <report@bugs.python.org> wrote:

    As we found out (and you expounded) there are a variety of
    reload/load-into cases that we could address more explicitly. Perhaps
    there's a better API that could address those needs more broadly, or maybe
    they're just not worth addressing specifically. Exploring all this is
    something that can wait, IMHO.

    Yes, that's an option, too, and probably a good one. To go down that path,
    we would drop the various "target" parameters and say loaders that need to
    check for the reloading case should continue to provide load_module without
    exec_module (at least for 3.4). runpy would use the rule that it supports
    anything that exposes exec_module without create_module.

    @ericsnowcurrently
    Copy link
    Member Author

    (I still like the idea of passing
    a target to importlib.find_spec - I just no longer think we should be
    passing that down to the finders themselves).

    Passing the target to the finders isn't just for the sake of any implicit "check_target" test, though that was the original motivator. It also allows the finder to decide between multiple loaders based on other criteria related to the target (but not necessarily the loader). I think it was a good addition to the API regardless.

    Another thing we need to check we have a test for: ensuring reloading a
    namespace module picks up new directories added since it was first loaded.

    Agreed. Furthermore, such a test is worthwhile outside the context of PEP-451. I'm tempted to say we're already covered with existing tests, but reload is goofy enough that an explicit test is worth it.

    This would all be so much easier if reloading wasn't supported in the first
    place :)

    So very true. :)

    @ncoghlan
    Copy link
    Contributor

    And coming full circle: there's no *harm* in letting finders reject loading
    into a target module, and that's orthogonal to having loaders reject it.
    It's just that loaders that want to do that will currently still need to
    implement load_module.

    That means the only thing we need to postpone is the load_module
    deprecation, since it still covers at least one advanced use case the new
    API doesn't handle.

    @ericsnowcurrently
    Copy link
    Member Author

    Sounds good. It will be worth adding a note to the load_module() docs indicating the limited cases where it is still appropriate, and encouraging the use of exec_module() instead.

    @brettcannon
    Copy link
    Member

    Semantic-affecting changes first, then fluffy bits in terms of priority. Basically go based on what is most critical to be in 3.4.

    And yes, basically can you comment out find_module/find_loader/load_module and execute the test suite (sans import-related tests) without import-related failures.

    @brettcannon
    Copy link
    Member

    No longer blocking b1, now just blocking rc1.

    @vstinner
    Copy link
    Member

    Your commit doesn't compile on Windows.

    Category None
    Changed by Eric Snow <ericsnowcurrently@gmail.com>
    Changed at Fri 22 Nov 2013 16:17:09
    Branch default
    Revision 07229c6104b16d0ab7cc63f3306157d3d2819fed
    Comments

    Implement PEP-451 (ModuleSpec).

    http://buildbot.python.org/all/builders/AMD64%20Windows7%20SP1%203.x/builds/3348/steps/compile/logs/stdio

    @berkerpeksag
    Copy link
    Member

    There is a commented-out line in Lib/importlib/_bootstrap.py:

    +# if hasattr(loader, 'get_data'):
    + if hasattr(loader, 'get_filename'):

    http://hg.python.org/cpython/rev/07229c6104b1#l6.369

    @brettcannon
    Copy link
    Member

    Removed the commented-out line in c8a84eed9155

    @ericsnowcurrently
    Copy link
    Member Author

    Failing buildbot:

    http://buildbot.python.org/all/builders/x86%20XP-4%203.x/builds/9630/steps/test/logs/stdio

    test.test_module.ModuleTests.test_module_repr_source
    test.test_pkgutil.ExtendPathTests.test_iter_importers (passed when retried)

    @brettcannon
    Copy link
    Member

    @brettcannon
    Copy link
    Member

    The test_module failure can be triggered by running test_importlib test_module. For some reason unittest's __spec__ gets set to one for the BuiltinImporter (and nothing else).

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Nov 22, 2013

    New changeset 98bab4a03120 by Brett Cannon in branch 'default':
    Issue bpo-18864: Don't try and use unittest as a testing module for
    http://hg.python.org/cpython/rev/98bab4a03120

    @brettcannon
    Copy link
    Member

    I just realized that there is no way to make ModuleSpec.has_location return True if the spec is created by simply instantiating ModuleSpec. As it stands now you **must** go through importlib.util.spec_from_file_location(), which makes it not a helper but a **required** API to use. I think this is the only thing that cannot be stated directly through ModuleSpec instantiation or public attribute assignment.

    This means either (a) the docs on ModuleSpec need to be **very** clear that you must use spec_from_file_location() to make spec.has_location be true, (b) add another keyword-only argument to ModuleSpec.__init__(), or (c) make has_location settable (e.g. be able to only set it to True/False or alternatively assigning it sets both has_location to True and origin to the assigned value).

    @ericsnowcurrently
    Copy link
    Member Author

    Good catch. My preference would be for (c) simply add a setter for the property that sets the underlying private variable to True or False.

    @brettcannon
    Copy link
    Member

    (c) it is then! I would just take the time to call bool() on the arg to coalesce it into a True/False thing (which maybe makes sense in __init__() as well).

    @brettcannon
    Copy link
    Member

    Actually, ignore the __init__() part of my last comment since it's set to False directly.

    @ericsnowcurrently
    Copy link
    Member Author

    Here's a patch for adding a setter for ModuleSpec.has_location.

    @brettcannon
    Copy link
    Member

    LGTM

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Dec 11, 2013

    New changeset e961a166dc70 by Eric Snow in branch 'default':
    Issue bpo-18864: Add a setter for ModuleSpec.has_location.
    http://hg.python.org/cpython/rev/e961a166dc70

    @ericsnowcurrently
    Copy link
    Member Author

    About the only thing left for this ticket is to finish up writing a few tests and round out some documentation.

    @ericsnowcurrently
    Copy link
    Member Author

    There are a few lingering to-do comments that will need to be addressed as part of the remaining work:

    Lib/test/test_importlib/extension/test_case_sensitivity.py:# XXX find_spec tests
    Lib/test/test_importlib/extension/test_finder.py:# XXX find_spec tests
    Lib/test/test_importlib/source/test_file_loader.py: mod = loader.load_module('_temp') # XXX
    Lib/test/test_importlib/source/test_file_loader.py: # XXX Change to use exec_module().
    Lib/test/test_importlib/test_windows.py: # XXX Need a test that finds the spec via the registry.

    @terryjreedy
    Copy link
    Member

    Failure with 3.4.0c1:

    [170/394/5] test_importlib
    test test_importlib failed -- Traceback (most recent call last):
      File "C:\Programs\Python34.32\lib\unittest\case.py", line 57, in testPartExecutor
        yield
      File "C:\Programs\Python34.32\lib\unittest\case.py", line 574, in run
        testMethod()
      File "C:\Programs\Python34.32\lib\unittest\loader.py", line 32, in testFailure
        raise exception
    ImportError: Failed to import test module: test.test_importlib.source.test_abc_loader
    Traceback (most recent call last):
      File "C:\Programs\Python34.32\lib\unittest\loader.py", line 312, in _find_tests
        module = self._get_module_from_name(name)
      File "C:\Programs\Python34.32\lib\unittest\loader.py", line 290, in _get_module_from_name
        __import__(name)
      File "C:\Programs\Python34.32\lib\test\test_importlib\source\test_abc_loader.py", line 73, in <module>
        class PyLoaderMock(abc.PyLoader):
    AttributeError: 'module' object has no attribute 'PyLoader'

    On the other hand, the test passes on a fresh debug build.

    @brettcannon
    Copy link
    Member

    Terry has admitted in other bugs he filed that he didn't use a fresh install location, so the test failure is most likely a red herring.

    @ericsnowcurrently
    Copy link
    Member Author

    I've gone ahead and moved all remaining work into separate issues. Once the 4 remaining sub-issues are resolved, this one can be closed. I've *finally* got a couple days free so I want to close this out. :)

    @BreamoreBoy
    Copy link
    Mannequin

    BreamoreBoy mannequin commented Jun 29, 2014

    Given that PEP-451 is listed in the "Finished PEPs (done, implemented in code repository)" section of the PEP index can we close this and associated issues out?

    @terryjreedy
    Copy link
    Member

    Dependencies 19711 and 21099 are still open.

    @BreamoreBoy
    Copy link
    Mannequin

    BreamoreBoy mannequin commented Jul 4, 2014

    I've asked for patch reviewss on the three dependencies outstanding, bpo-19711, bpo-19714 and bpo-21099.

    @BreamoreBoy
    Copy link
    Mannequin

    BreamoreBoy mannequin commented Feb 24, 2015

    I've asked for updates on the two remaining dependencies bpo-19711 and bpo-21099.

    @ericsnowcurrently
    Copy link
    Member Author

    The last dependencies have now been closed!

    @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
    deferred-blocker interpreter-core (Objects, Python, Grammar, and Parser dirs) stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants