Add a keyword-only spec argument to types.ModuleType

[brettcannon]

BPO	20383
Nosy	@brettcannon, @rhettinger, @ncoghlan, @mitsuhiko, @ericsnowcurrently
Files	module_from_spec.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/brettcannon'
closed_at = <Date 2014-05-30.18:57:20.024>
created_at = <Date 2014-01-24.17:11:47.874>
labels = ['library']
title = 'Add a keyword-only spec argument to types.ModuleType'
updated_at = <Date 2014-05-31.01:24:37.704>
user = 'https://github.com/brettcannon'

bugs.python.org fields:

activity = <Date 2014-05-31.01:24:37.704>
actor = 'ncoghlan'
assignee = 'brett.cannon'
closed = True
closed_date = <Date 2014-05-30.18:57:20.024>
closer = 'brett.cannon'
components = ['Library (Lib)']
creation = <Date 2014-01-24.17:11:47.874>
creator = 'brett.cannon'
dependencies = []
files = ['35286']
hgrepos = []
issue_num = 20383
keywords = ['patch']
message_count = 28.0
messages = ['209100', '215554', '215629', '215648', '215649', '215653', '215662', '217925', '218759', '218981', '219043', '219046', '219135', '219156', '219168', '219170', '219173', '219224', '219229', '219388', '219389', '219397', '219399', '219402', '219411', '219412', '219418', '219434']
nosy_count = 7.0
nosy_names = ['brett.cannon', 'rhettinger', 'ncoghlan', 'aronacher', 'Arfrever', 'python-dev', 'eric.snow']
pr_nums = []
priority = 'low'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue20383'
versions = ['Python 3.5']

[brettcannon]

Would allow for the name attribute to be optional since it can be grabbed from the spec. Since having module.__spec__ set is now expected we should help ensure that by supporting it in the constructor.

[brettcannon]

I envision making this happen would also allow for importlib to be updated to rely on __spec__ when possible, with the idea that sometime in the future we can deprecate pulling attributes from a module directly and shift to always working from __spec__.

[ericsnowcurrently]

Sounds good to me.

[rhettinger]

Would allow for the name attribute to be optional
...
with the idea that sometime in the future we can
deprecate pulling attributes from a module directly

Is there any chance that this would ever happen?
The __name__ attribute has been around almost forever
is widely relied on. Deprecating it would be a very
disruptive event for nearly zero benefit.

[ericsnowcurrently]

I made roughly the same point in the current import-sig thread that relates here:

https://mail.python.org/pipermail/import-sig/2014-April/000805.html

Basically, I agree we should be careful with both __name__ and __file__.

[ncoghlan]

No, the attribute level arguments won't go away - __name__ deliberately differs from __spec__.name in some cases (notably in __main__), __path__ may be manipulated after the module is loaded, and __name and __file__ are both used too heavily within module code for it to be worth the hassle of deprecating them in favour of something else.

I think Brett's push to simplify things as much as possible is good though - that's the main brake on creeping API complexity in the overall import system as we try to make the internals easier to comprehend and manipulate.

[brettcannon]

I can dream about getting rid of the attributes, but I doubt it would happen any time soon, if at all. But we do need to make it easier to set __spec__ on a new module than it currently is to help promote its use.

[brettcannon]

My current thinking on this it to introduce in importlib.util:

  def module_from_spec(spec, module=None):
    """Create/initialize a module based on the provided spec.

If a module is provided then spec.loader.create_module()
will not be consulted.
"""

This serves two purposes. One is that it abstracts the loader.create_module() dance out so that's no longer a worry. But more crucially it also means that if you have the function create the module for you then it will be returned with all of its attributes set without having to worry about forgetting that step. The module argument is just for convenience in those instances where you truly only want to override the module creation dance for some reason and really just want the attribute setting bit.

[brettcannon]

Here is an implementation of importlib.util.module_from_spec(). With this it makes PEP-451 conceptually work like:

  spec = importlib.util.find_spec(name)
  module = importlib.util.module_from_spec(spec)
  spec.loader.exec_module(module)

About the only other thing I can think of that people might still want is something like importlib.util.load(spec) so that they don't even need to care about whether load_module() or exec_module() is defined, but that can be a separate issue if it turns out people actually want something like that.

[ericsnowcurrently]

@brett: Did those last two messages (and the patch) get on the wrong issue. The issue of module_from_spec() or the like seems somewhat orthogonal to a spec argument to ModuleType. Perhaps you meant issue bpo-21436 or bpo-21235?

Otherwise, I have a few comments. :)

[brettcannon]

Nope, I commented where I meant to. I wanted a way to promote people to **always** create modules with properly initialized attributes while also dealing with the module creation dance at the same time. Otherwise it will require expanding the API of types.ModuleType() to accommodate specs while also needing to expose a function to do the proper thing to get a module for a loader and **still** have a function to set up a module properly based on what was potentially returned from create_module(). Rolling it all into a single function that just gets you a module ready for use seems like the most practical solution from an API perspective.

[ncoghlan]

I'd ask "Why not a class method?", but I already know the answer (types.ModuleType is implemented in C, so it would be unnecessarily painful to implement it that way).

Given that, the utility function approach sounds good to me.

[ericsnowcurrently]

Okay, I didn't read closely enough. :) It may be worth updating the title.

FWIW, the name "module_from_spec" confused me at first because my brain interpreted that as "load_from_spec". Keeping the name and purpose more focused might be helpful. I have comments below to that effect.

If your proposed change still makes sense, could we keep it simpler for now? Something like this:

# in importlib.util
def module_from_spec(spec, module=None):
    """..."""
    methods = _bootstrap._SpecMethods(spec)
    if module is None:
        return methods.create()
    else:
        methods.init_module_attrs(methods)
        return module

Keeping the 2 methods on _SpecMethods is helpful for the PEP-406 (ImportEngine) superseder that I still want to get back to for 3.5. :)

----------------------------------

This serves two purposes. One is that it abstracts the
loader.create_module() dance out so that's no longer a worry.

I'm not sure what dance you mean here and what the worry is.

But more crucially it also means that if you have the function
create the module for you then it will be returned with all of
its attributes set without having to worry about forgetting that
step.

So we would discourage calling ModuleType directly and encourage the use of a function in importlib.util that is equivalent to _SpecMethods.create(). That sounds good to me. The use case for creating module objects directly is a pretty advanced one, but having a public API for that would still be good.

From this point of view I'd expect it to just look like this:

def new_module(spec):
    return _SpecMethods(spec).create()

or given what I expect is the common use case currently:

def new_module(name, loader):
    spec = spec_from_loader(name, loader)
    return _SpecMethods(spec).create()

or together:

def new_module(spec_or_name, /, loader=None):
    if isinstance(spec_or_name, str):
        name = spec_or_name
        if loader is None:
            raise TypeError('missing loader')
        spec = spec_from_loader(name, loader)
    else:
        if loader is not None:
            raise TypeError('got unexpected keyword argument "loader"')
        spec = spec_or_name
    return _SpecMethods(spec).create()

To kill 2 birds with 1 stone, you could even make that the new signature of ModuleType(), which would just do the equivalent under the hood. That way people keep using the same API that they already are (no need to communicate a new one to them), but they still get the appropriate attributes set properly.

The module argument is just for convenience in those
instances where you truly only want to override the module
creation dance for some reason and really just want the
attribute setting bit.

Perhaps it would be better to have a separate function for that (equivalent to just _SpecMethods.init_module_attrs()). However, isn't that an even more uncommon use case outside of the import system itself?

About the only other thing I can think of that people might
still want is something like importlib.util.load(spec)

I agree it's somewhat orthogonal. I'll address comments to issue bpo-21235.