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

importlib's spec module create algorithm is not exposed #65434

Closed
mitsuhiko opened this issue Apr 15, 2014 · 15 comments
Closed

importlib's spec module create algorithm is not exposed #65434

mitsuhiko opened this issue Apr 15, 2014 · 15 comments
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@mitsuhiko
Copy link
Member

BPO 21235
Nosy @brettcannon, @ncoghlan, @mitsuhiko, @merwok, @florentx, @ericsnowcurrently, @1st1
Dependencies
  • bpo-20383: Add a keyword-only spec argument to types.ModuleType
  • bpo-21581: Consider dropping importlib.abc.Loader.create_module()
  • 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 2016-01-15.21:35:04.314>
    created_at = <Date 2014-04-15.14:24:41.701>
    labels = ['type-bug', 'library']
    title = "importlib's spec module create algorithm is not exposed"
    updated_at = <Date 2016-01-15.21:35:04.313>
    user = 'https://github.com/mitsuhiko'

    bugs.python.org fields:

    activity = <Date 2016-01-15.21:35:04.313>
    actor = 'brett.cannon'
    assignee = 'none'
    closed = True
    closed_date = <Date 2016-01-15.21:35:04.314>
    closer = 'brett.cannon'
    components = ['Library (Lib)']
    creation = <Date 2014-04-15.14:24:41.701>
    creator = 'aronacher'
    dependencies = ['20383', '21581']
    files = []
    hgrepos = []
    issue_num = 21235
    keywords = ['3.4regression']
    message_count = 15.0
    messages = ['216295', '216296', '216303', '216307', '216313', '216314', '216324', '219044', '219136', '219158', '219160', '219169', '219223', '255904', '258331']
    nosy_count = 10.0
    nosy_names = ['brett.cannon', 'ncoghlan', 'aronacher', 'eric.araujo', 'Arfrever', 'flox', 'tshepang', 'eric.snow', 'yselivanov', 'nikicat']
    pr_nums = []
    priority = 'normal'
    resolution = None
    stage = None
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue21235'
    versions = ['Python 3.5']

    @mitsuhiko
    Copy link
    Member Author

    3.4 deprecates load_module on the loaders and now proposes to use create_module (optionally) and exec_module. Unfortunately for external callers these interfaces are not useful because you need to reimplement _SpecMethods.create and a whole bunch of other stuff for it.

    Right now a caller can get hold of _SpecMethods by importing from _frozen_importlib but that seems very unsupported and is obviously entirely not exposed.

    Is there a reason those methods are not on the ModuleSpec directly but on a private wrapper?

    Example usage:

    from types import ModuleType
    from importlib.machinery import ModuleSpec, SourceFileLoader
    from _frozen_importlib import _SpecMethods
    
    loader = SourceFileLoader('plugin_base', 'my_file.py')
    spec = ModuleSpec(name=loader.name, loader=loader)
    mod = _SpecMethods(spec).create()

    In the absence of _SpecMethods a caller needs to do this:

    from importlib.machinery import ModuleSpec, SourceFileLoader
    
    loader = SourceFileLoader('plugin_base', 'my_file.py')
    spec = ModuleSpec(name=loader.name, loader=loader)
    if not hasattr(loader, 'create_module'):
        module = types.ModuleType(loader.name)
    else:
        module = loader.create_module(spec)
    module.__loader__ = loader
    module.__spec__ = spec
    try:
        module.__package__ = spec.parent
    except AttributeError:
        pass
    loader.exec_module(module)

    (And that is not the whole logic yet)

    @mitsuhiko
    Copy link
    Member Author

    On further investigation that is not even enough yet due to the new locking mechanism. I'm not even sure if exposing _SpecMethods would be enough.

    @ericsnowcurrently
    Copy link
    Member

    I agree that this is something we need to address in 3.5. Adding this to 3.4 won't be an option since it would require a new feature. However, Loader.load_module() is only deprecated (and won't be removed in 3.X), so the current approach will still work until we provide a new approach in 3.5. The only gap is that now it is possible (even if very unlikely) that in 3.4 you would run into a loader that does not implement load_module(), which would obviously break direct calls to the method. :(

    For 3.4 such direct calls in the stdlib to loader.load_module() were replaced with this (mostly):

      spec = importlib.util.spec_from_loader(name, loader)
      # or importlib.util.spec_from_file_location(...)
      methods = _SpecMethods(spec)
      mod = methods.load()

    As you already noted, this relies on a current implementation detail of importlib._bootstrap. We'd rather not encourage use of such private API so providing a simple replacement makes sense.

    Futhermore, for 3.5 (in the "soon" timeframe) I'm planning on working on improving the import system abstractions*. My expectation is that part of that will be exposing most or all of the functionality of _SpecMethods directly. At the same time I don't want that API to be a distraction. I think accomplishing both is doable.

    So you shouldn't need to worry about create() or anything else.

    @ericsnowcurrently ericsnowcurrently added the stdlib Python modules in the Lib dir label Apr 15, 2014
    @brettcannon
    Copy link
    Member

    Are you after just the module creation/initialization code so you can call exec_module() yourself, Armin, or do you want even more of the algorithm exposed (e.g. the lock stuff)? There are not tons of importlib users to the level of wanting to re-implement import to the level of wanting to control module creation, setting sys.modules manually, etc., so we are constantly trying to figure out how far to go in exposing things without going so far as to make it impossible to make changes.

    @mitsuhiko
    Copy link
    Member Author

    I'm not sure myself what I need right now. I personally have avoided importlib/imp entirely for my code and I roll with manual module creation because it is most stable between 2.6 - 3.4 but it's getting more complicated to work because of all the new attributes (package, __spec__ etc.).

    This particular case came from SQLAlchemy I believe (I tried to help Mike Bayer transition his code).

    It's true that create() is the wrong function, load() is actually what should have been used there.

    @mitsuhiko
    Copy link
    Member Author

    Also mostly unrelated importlib now does something I have never seen an ABC do: the ABC has create_module but concrete implementations mostly have that function entirely absent. That should probably be reconsidered as it's super confusing.

    @ericsnowcurrently
    Copy link
    Member

    I've opened up bpo-21240 to address the the docs concern. Thanks for bringing it up. :)

    @florentx florentx mannequin added the type-bug An unexpected behavior, bug, or error label Apr 15, 2014
    @brettcannon
    Copy link
    Member

    Issue bpo-20383 is tracking adding an API to simply getting the proper module and having it be initialized.

    @ericsnowcurrently
    Copy link
    Member

    How about this replacement for direct use of Loader.load_module():

    # in importlib.util
    def load(spec_or_name, /, **kwargs):  # or "load_from_spec"
        if isinstance(spec_or_name, str):
            name = spec_or_name
            if not kwargs:
                raise TypeError('missing loader')
            spec = spec_from_loader(name, **kwargs)
        else:
            if kwargs:
                raise TypeError('got unexpected keyword arguments')
            spec = spec_or_name
        return _SpecMethods(spec).load()

    (See a similar proposal for new_module() in msg219135, issue bpo-20383).

    @brettcannon
    Copy link
    Member

    I think that's the wrong abstraction(it would be fine in a third-party library, though, that's trying to smooth over 3.3->3.4 transitions). Since importlib.util.find_spec() always returns a spec, then you want something more like::

      def load(spec):
        loader = spec.loader
        if hasattr(loader, 'exec_module'):
            module = importlib.util.whatever_issue_20383_leads_to()
            loader.exec_module(module)
            return module
        else:
            loader.load_module(spec.name)
            return sys.modules[spec.name]

    Since this is Python 3.5 code we are talking about you don't have to worry about the find_loader/find_module case as find_spec wraps both of those and normalizes it all to just using specs. You could even tweak it to pass the module in explicitly and in the load_module branch make sure that the module is placed in sys.modules first so it essentially becomes a reload (but as both know that's not exactly the same nor pleasant in certain cases so probably best not exposed that way =).

    If this exists in importlib.util then you make import_module() be (essentially)::

      def import_module(name, package):
        fullname = resolve_name(name, package)
        spec = find_spec(fullname)
        return load(spec)

    @brettcannon
    Copy link
    Member

    Opened issue bpo-21581 to discuss Armin's point about importlib.abc.Loader.create_module() being there but not being much use since the method is entirely optional.

    @ericsnowcurrently
    Copy link
    Member

    I'm just considering current usage:

      mod = loader.load_module(name)

    becomes:

      spec = spec_from_loader(name, loader)
      mod = load(spec)

    vs.

      mod = load(name, loader=loader)

    I guess I'm torn. I like how the former forces you to consider specs when dealing with the import system. On the other hand, I don't want it to be annoying to have to move to this brave new world (that's exactly why we toned down the deprecations). And on the third hand, perhaps it would be annoying to just a handful of people so we shouldn't sweat it.

    So if we end up simplifying, load() could look like this:

    def load(spec):  # or "load_from_spec"
        return _SpecMethods(spec).load()

    It already takes care of everything we need, including the import lock stuff.

    @brettcannon
    Copy link
    Member

    I'm not torn so let that settle your torment. =) Considering we are talking about the standard library for a language that has a mantra of "explicit is better than implicit" I think worrying about an added line to very little code since so few people muck with this stuff is enough to not worry about it.

    @brettcannon
    Copy link
    Member

    Is there anything left in this issue that hasn't been addressed in Python 3.5?

    @brettcannon
    Copy link
    Member

    Due to lack of response, I'm assuming all issues are now addressed.

    @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
    stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants