This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

Title: Don't have be optional
Type: behavior Stage: resolved
Components: Interpreter Core Versions: Python 3.6, Python 3.5
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: brett.cannon Nosy List: Anthony Sottile, Arfrever, brett.cannon, eric.snow, ncoghlan, python-dev
Priority: normal Keywords: patch

Created on 2014-12-08 16:18 by brett.cannon, last changed 2022-04-11 14:58 by admin. This issue is now closed.

File name Uploaded Description Edit
require_create_module.diff brett.cannon, 2014-12-12 17:26 review
Messages (27)
msg232313 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2014-12-08 16:18
I continue to be bothered by how we designed 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 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 more as well.

What do other people think?
msg232334 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2014-12-08 22:34
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.
msg232380 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2014-12-09 14:53
Fair enough. I have no qualms with keeping importlib.util.module_from_spec() and have Loader.create_module() just call module_from_spec().
msg232402 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2014-12-09 23:21
OK, that sounds good to me, then. From a compatibility perspective, a
porting note for 3.5 should cover it.
msg232431 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2014-12-10 15:28
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 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).
msg232432 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2014-12-10 16:01
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.
msg232479 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2014-12-11 14:40
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.
msg232511 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2014-12-12 06:29
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 file in package directories)
msg232549 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2014-12-12 14:39
I don't think we really need to say anything. If people want default results, simply return None (which is handled for them by 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.
msg232562 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2014-12-12 17:26
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.
msg232699 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2014-12-16 02:47
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. :)
msg232700 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2014-12-16 02:47
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. :)
msg232741 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2014-12-16 13:47
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 :)
msg232745 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2014-12-16 15:05
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 to make the code flow easier to follow.
msg232789 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2014-12-17 01:33

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. :)
msg232790 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2014-12-17 01:36

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.
msg233764 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-01-09 16:40
New changeset ab72f30bcd9f by Brett Cannon in branch 'default':
Issue #23014: Make required when
msg346576 - (view) Author: Anthony Sottile (Anthony Sottile) * Date: 2019-06-26 01:08
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?)
msg346656 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2019-06-26 18:31
PEPs are not living documents unless they are marked as Active (which PEP 451 is not).
msg346657 - (view) Author: Anthony Sottile (Anthony Sottile) * Date: 2019-06-26 18:46
Where can I find up to date best practices / docs for the import machinery? I couldn't find a mention of this in
msg346660 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2019-06-26 18:55
The import machinery docs are split between the language reference and importlib itself. It really depends on what you're looking for.
msg346662 - (view) Author: Anthony Sottile (Anthony Sottile) * Date: 2019-06-26 18:57
Mostly looking for something that says how `create_module` should / shouldn't be implemented (and why `return None` is necessary)
msg346673 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2019-06-26 20:00
> 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).
msg346675 - (view) Author: Anthony Sottile (Anthony Sottile) * Date: 2019-06-26 20:04
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

Everyone I've shown this bit of code has essentially said: "*headscratch* -- that's weird?"

why require the function at all I guess
msg346780 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2019-06-27 21:58
Feel free to open a new issue to propose changing it.
msg346782 - (view) Author: Anthony Sottile (Anthony Sottile) * Date: 2019-06-27 22:04
I suspect it would be closed since it's essentially just a revert of this issue :S
msg346841 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2019-06-28 17:35
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.
Date User Action Args
2022-04-11 14:58:10adminsetgithub: 67203
2019-06-28 17:35:32brett.cannonsetmessages: + msg346841
2019-06-27 22:04:25Anthony Sottilesetmessages: + msg346782
2019-06-27 21:58:44brett.cannonsetmessages: + msg346780
2019-06-26 20:04:02Anthony Sottilesetmessages: + msg346675
2019-06-26 20:00:11brett.cannonsetmessages: + msg346673
2019-06-26 18:57:22Anthony Sottilesetmessages: + msg346662
2019-06-26 18:55:24brett.cannonsetmessages: + msg346660
2019-06-26 18:46:56Anthony Sottilesetmessages: + msg346657
2019-06-26 18:31:25brett.cannonsetmessages: + msg346656
2019-06-26 01:08:21Anthony Sottilesetnosy: + Anthony Sottile
messages: + msg346576
2015-01-09 16:40:27python-devsetnosy: + python-dev
messages: + msg233764
2015-01-09 16:40:15brett.cannonsetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2014-12-17 01:36:43eric.snowsetmessages: + msg232790
2014-12-17 01:33:51eric.snowsetmessages: + msg232789
2014-12-16 15:05:24brett.cannonsetmessages: + msg232745
2014-12-16 13:47:21ncoghlansetmessages: + msg232741
2014-12-16 02:47:53eric.snowsetmessages: + msg232700
2014-12-16 02:47:44eric.snowsetmessages: + msg232699
2014-12-12 17:30:24brett.cannonlinkissue21581 dependencies
2014-12-12 17:27:18brett.cannonlinkissue23013 dependencies
2014-12-12 17:26:53brett.cannonsetfiles: + require_create_module.diff
keywords: + patch
messages: + msg232562

stage: test needed -> patch review
2014-12-12 14:39:19brett.cannonsetmessages: + msg232549
2014-12-12 06:29:43ncoghlansetmessages: + msg232511
2014-12-11 14:40:17brett.cannonsetassignee: brett.cannon
messages: + msg232479
2014-12-10 16:01:37ncoghlansetmessages: + msg232432
2014-12-10 15:28:01brett.cannonsetmessages: + msg232431
2014-12-10 06:46:55Arfreversetnosy: + Arfrever
2014-12-09 23:21:35ncoghlansetmessages: + msg232402
2014-12-09 14:53:08brett.cannonsetmessages: + msg232380
2014-12-08 22:34:19ncoghlansetmessages: + msg232334
2014-12-08 16:18:58brett.cannoncreate