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

Don't have importlib.abc.Loader.create_module() be optional #67203

Closed
brettcannon opened this issue Dec 8, 2014 · 27 comments
Closed

Don't have importlib.abc.Loader.create_module() be optional #67203

brettcannon opened this issue Dec 8, 2014 · 27 comments
Assignees
Labels
interpreter-core Interpreter core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@brettcannon
Copy link
Member

brettcannon commented Dec 8, 2014

BPO 23014
Nosy @brettcannon, @ncoghlan, @ericsnowcurrently, @asottile
Files
  • require_create_module.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 2015-01-09.16:40:15.884>
    created_at = <Date 2014-12-08.16:18:58.725>
    labels = ['interpreter-core', 'type-bug']
    title = "Don't have importlib.abc.Loader.create_module() be optional"
    updated_at = <Date 2019-06-28.17:35:32.150>
    user = 'https://github.com/brettcannon'

    bugs.python.org fields:

    activity = <Date 2019-06-28.17:35:32.150>
    actor = 'brett.cannon'
    assignee = 'brett.cannon'
    closed = True
    closed_date = <Date 2015-01-09.16:40:15.884>
    closer = 'brett.cannon'
    components = ['Interpreter Core']
    creation = <Date 2014-12-08.16:18:58.725>
    creator = 'brett.cannon'
    dependencies = []
    files = ['37427']
    hgrepos = []
    issue_num = 23014
    keywords = ['patch']
    message_count = 27.0
    messages = ['232313', '232334', '232380', '232402', '232431', '232432', '232479', '232511', '232549', '232562', '232699', '232700', '232741', '232745', '232789', '232790', '233764', '346576', '346656', '346657', '346660', '346662', '346673', '346675', '346780', '346782', '346841']
    nosy_count = 6.0
    nosy_names = ['brett.cannon', 'ncoghlan', 'Arfrever', 'python-dev', 'eric.snow', 'Anthony Sottile']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue23014'
    versions = ['Python 3.5', 'Python 3.6']

    @brettcannon
    Copy link
    Member Author

    brettcannon commented Dec 8, 2014

    I continue to be bothered by how we designed importlib.abc.Loader.create_module(). I don't like that it's optional and I don't like that it can return None.

    I would much rather make it so that importlib.abc.Loader.create_module() was a static method that does what importlib.util.module_from_spec() does. I would also like importlib to throw a fit if exec_module() was defined on a loader but not create_module() with a DeprecationWarning to start and then an AttributeError later (as I said, this requires exec_module() defined and does not play into the whole load_module() discussion). This should also hopefully promote people subclassing importlib.abc.Loader more as well.

    What do other people think?

    @brettcannon brettcannon added interpreter-core Interpreter core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error labels Dec 8, 2014
    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Dec 8, 2014

    I mostly like the idea, but am wary of having the base class implement it
    as a static method that subclasses are then likely to override with a
    normal instance method.

    A module level function somewhere + a base class instance method that just
    delegates to the stateless function version would feel cleaner to me.

    @brettcannon
    Copy link
    Member Author

    brettcannon commented Dec 9, 2014

    Fair enough. I have no qualms with keeping importlib.util.module_from_spec() and have Loader.create_module() just call module_from_spec().

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Dec 9, 2014

    OK, that sounds good to me, then. From a compatibility perspective, a
    porting note for 3.5 should cover it.

    @brettcannon
    Copy link
    Member Author

    brettcannon commented Dec 10, 2014

    What do you think about requiring create_module() and/or not supporting a None value return? The only reason I ask is to think through the ramifications and how long to raise a DeprecationWarning. I think I can live with having a reasonable default if create_module() isn't defined, but also supporting None seems a bit much. Or I can live with a None value to have import just do the most reasonable thing, but you should then explicitly ask for that by returning None. Or we just go full-on explicit and require create_module() and that it return the object to use.

    So assuming we go with the latter, that means people who chose to not subclass importlib.abc.Loader will have to now implement create_module(). In Python 3.5 that's easy since importlib.util.module_from_spec() now exists. But if you're supporting older versions it's a bit more work since there was only types.ModuleType() and that didn't fill in all attributes since it was missing key bits of information that is now (potentially) in the spec. That means if someone wants to be compatible from 3.4 on then it will be a bit of work to do properly.

    That means either a deprecation cycle that is long enough to outlast code that wants to work in 3.4 or we don't go as far and support either returning None *or* not defining create_module(). My gut says to either deprecate both options and make the deprecation last until Python 4 or to require create_module() but allow for the returning of None and have that last only a single release since adding a create_module() that returns None takes no effort and is fully backwards-compatible (my reasoning for the latter is that screwing up to return None when you meant a module is easier to debug than accidentally typing the method name, plus it means people calling super() won't break in a transition).

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Dec 10, 2014

    The specific reason I wanted the "return None to delegate to the default behaviour" feature was to make it easier to migrate the C extension machinery.

    With that design, a PEP-451 based C extension loader would just need to return None when there was no appropriate "Create" symbol exported from the module.

    If returning None was disallowed, it would need to instead arrange to call importlib.util.module_from_spec(). That's doable, of course, but requires loaders to reimplement behaviour provided by the standard import system, rather than being able to just say "do the default thing, whatever that happens to be". That's the kind of really easy to get wrong responsibility I appreciated PEP-451 taking *away* from custom loaders.

    However, I have no problem with making create_module() mandatory if the "return None to request the default behaviour" feature is retained. As you say, adding an implementation that returns None is both easy and remains compatible with Python 3.4.

    @brettcannon
    Copy link
    Member Author

    brettcannon commented Dec 11, 2014

    OK, then why don't we just make create_module() a required method to use exec_module(), add a DeprecationWarning for when it isn't defined, leave Loader.create_module() as-is (already returns None), and then plan to make it a requirement in Python 3.6 since the fix is so simple. That leaves the None return value for pragmatic reasons but makes it only way to specify the default instead of 2.

    @brettcannon brettcannon self-assigned this Dec 11, 2014
    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Dec 12, 2014

    Sounds good to me.

    I'm not sure what to put in the docs about calling importlib.util.module_from_spec() from create_module() implementations. Really, if a loader can use that, then it should be doing whatever it was going to do after the call in exec_module() instead.

    create_module() is intended specifically for cases where you genuinely need to do something different to create the object (as in the concept of calling in to a C level module creation hook, or the idea of allowing a __new__.py file in package directories)

    @brettcannon
    Copy link
    Member Author

    brettcannon commented Dec 12, 2014

    I don't think we really need to say anything. If people want default results, simply return None (which is handled for them by importlib.abc.Loader). The only thing changing here is that the method will now be required instead of optional.

    I'll post the patch here before I commit to make sure it's all clear.

    @brettcannon
    Copy link
    Member Author

    brettcannon commented Dec 12, 2014

    Here is a patch that makes sure that if exec_module() is defined then so is create_module(). There really isn't any benefit in the code now, but starting in Python 3.6 we can make a very clear code path delineation between spec-based loading and old-fashioned load_module() instead of the current solution we have now where create_module() is used for either path and everything is intertwined.

    @ericsnowcurrently
    Copy link
    Member

    ericsnowcurrently commented Dec 16, 2014

    Thanks for bringing this up, Brett. The goal and approach seem good to me. Did you bring this up during the PEP-451 discussions? If so, sorry I missed it. You've made a good point. And hopefully this will encourage people to subclass Loader more. :)

    @ericsnowcurrently
    Copy link
    Member

    ericsnowcurrently commented Dec 16, 2014

    At some point should we make create_module (and exec_module) always required? In other words, when should would drop the legacy loader support? I expect the answer is Python 4. :)

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Dec 16, 2014

    I'm still a fan of "never" - it's a good escape hatch that means we never have to contort the new API to deal with some of the more esoteric use cases that involved completely overloading the import process in the loader :)

    @brettcannon
    Copy link
    Member Author

    brettcannon commented Dec 16, 2014

    Yeah, we need to settle the whole load_module() thing at PyCon because I'm tired of it hanging over our heads since the code is entirely structured to yank it out and if it's going to stay I want to clean up _bootstrap.py to make the code flow easier to follow.

    @ericsnowcurrently
    Copy link
    Member

    ericsnowcurrently commented Dec 17, 2014

    @nick

    As long as those esoteric cases can be handled without much work via create_module and exec_module, we should have a goal in mind for yanking legacy load_module support. If there are valid use cases that can't be handled via PEP-451 then I'd say we should consider finding a better way. If Loader.load_module is that better way then we need to consider a better way to handle it.

    What cases do you have in mind that only load_module handles (at least easily)? I can vaguely imagine the circumstances, but nothing concrete comes to mind. It doesn't seem like the sort of situation that has come up more that a handful of times nor one the currently couldn't be addressed in another way. I'm probably missing something here and I wouldn't be surprised if we discussed this point during the PEP-451 discussions. :)

    @ericsnowcurrently
    Copy link
    Member

    ericsnowcurrently commented Dec 17, 2014

    @brett

    Discussing this at PyCon sounds good. I'm in favor of dropping load_module as soon as possible (if possible), as you might have guessed by the way I implemented the module spec code. :) I agree that we should make a decision about load_module one way or the other, as either way we can consequently clean up that code.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 9, 2015

    New changeset ab72f30bcd9f by Brett Cannon in branch 'default':
    Issue bpo-23014: Make importlib.abc.Loader.create_module() required when
    https://hg.python.org/cpython/rev/ab72f30bcd9f

    @asottile
    Copy link
    Mannequin

    asottile mannequin commented Jun 26, 2019

    Hi! just stumbled on this as I was updating pytest's import hook. I notice in PEP-451 this is still marked as optional (then again, I don't know whether PEPs are supposed to be living standards or not, or if there was a PEP which supersedes that one with better direction?)

    @brettcannon
    Copy link
    Member Author

    brettcannon commented Jun 26, 2019

    PEPs are not living documents unless they are marked as Active (which PEP-451 is not).

    @asottile
    Copy link
    Mannequin

    asottile mannequin commented Jun 26, 2019

    Where can I find up to date best practices / docs for the import machinery? I couldn't find a mention of this in docs.python.org

    @brettcannon
    Copy link
    Member Author

    brettcannon commented Jun 26, 2019

    The import machinery docs are split between the language reference and importlib itself. It really depends on what you're looking for.

    @asottile
    Copy link
    Mannequin

    asottile mannequin commented Jun 26, 2019

    Mostly looking for something that says how create_module should / shouldn't be implemented (and why return None is necessary)

    @brettcannon
    Copy link
    Member Author

    brettcannon commented Jun 26, 2019

    Mostly looking for something that says how create_module should / shouldn't be implemented

    Basically you only need to provide the method if you want to use a custom object for the module itself. So as long as the object can quack like a module object you should be fine.

    (and why return None is necessary)

    The "return None for default semantics" is there for two reasons (if I remember correctly). One, it makes it very easy to implement the default semantics. :) Two, it sends a very clear signal that nothing out of the ordinary is going on and so you can assume nothing special is expected (which is why LazyLoader requires this since it mucks with the object directly in a very specific way).

    @asottile
    Copy link
    Mannequin

    asottile mannequin commented Jun 26, 2019

    yeah I guess I'm just curious why this bug (seemingly intentionally) made the implementation diverge from PEP-451 and require an empty create_module (and where I would have found that except by searching bugs.python.org)

    Everyone I've shown this bit of code has essentially said: "*headscratch* -- that's weird?" https://github.com/pytest-dev/pytest/blob/6a2d844c5d3e1e692d5bb6fa065c0d753d1dd7d1/src/_pytest/assertion/rewrite.py#L92-L93

    why require the function at all I guess

    @brettcannon
    Copy link
    Member Author

    brettcannon commented Jun 27, 2019

    Feel free to open a new issue to propose changing it.

    @asottile
    Copy link
    Mannequin

    asottile mannequin commented Jun 27, 2019

    I suspect it would be closed since it's essentially just a revert of this issue :S

    @brettcannon
    Copy link
    Member Author

    brettcannon commented Jun 28, 2019

    Do realize the person who created and closed this issue is the one suggesting you open a new issue. ;) We can re-evaluate decisions and this one is 5 years old. Now I'm not making any promises that we will definitely change anything, but I'm not going to swoop in and immediately close it either.

    @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 Interpreter core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants