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

Created on 2014-12-08 16:18 by brett.cannon, last changed 2015-01-09 16:40 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
require_create_module.diff brett.cannon, 2014-12-12 17:26 review
Messages (17)
msg232313 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2014-12-08 16:18
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?
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 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).
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 __new__.py 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 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.
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 _bootstrap.py to make the code flow easier to follow.
msg232789 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2014-12-17 01:33
@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. :)
msg232790 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2014-12-17 01:36
@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.
msg233764 - (view) Author: Roundup Robot (python-dev) Date: 2015-01-09 16:40
New changeset ab72f30bcd9f by Brett Cannon in branch 'default':
Issue #23014: Make importlib.abc.Loader.create_module() required when
https://hg.python.org/cpython/rev/ab72f30bcd9f
History
Date User Action Args
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