Re: Round 2 for "A ModuleSpec Type for the Import System"

On Wed, Aug 14, 2013 at 6:27 PM, PJ Eby <[email protected]> wrote:

> On Fri, Aug 9, 2013 at 6:58 PM, Eric Snow <[email protected]>
> wrote:
> > A High-Level View
> > -----------------
> >
> > ...
>
> It would be really helpful if that high-level view were actually
> included, as I'm having a lot of trouble wrapping my head around the
> rest of the spec.  For that matter, some introductory examples to
> contrast "before" and "after" for something that this changes would be
> really nice at about this point.
>

Sounds good.  As to examples, do you mean how you would replace an
implementation of load_module() with one of exec_module()?


> > Path-based attributes:
> >
> > If any of these is set, it indicates that the module is path-based.  For
> > reference, a path entry is a string for a location where the import
> > system will look for modules, e.g. the path entries in ``sys.path`` or a
> > package's ``__path__``).
> >
>
> What does "path-based" actually mean here?  On the one hand, you're
> saying that a path entry is on sys.path or a __path__, but then we're
> using an attribute called "filename".  Shouldn't it be called
> path_entry or subpath, or location or something, if it's not required
> to be a filename?  The overlap between path = sys.path and path =
> filesystem path is way too confusing here.
>

This is a really good point.  I'll clean it up.  I've already changed
"path" to "path_entries" and dropped "filename" in favor of "set_fileattr".
 Furthermore, "file location" is a good substitute for "path" when talking
about files.


> > .. XXX Would a different name be better?  ``path_location``?
>
> Yeah, definitely something other than filename.  ;-)
>
> It might also help to explain that some modules can be loaded by
> reference to a location, e.g. a filesystem path or a URL or something
> of the sort -- having the location lets you load the module, but in
> theory you could load that module under various names.  In contrast,
> non-located modules can't be loaded in this fashion: modules created
> by a meta path loader (such as builtins), or modules dynamically
> created in code.  For these, the name is the only way to access them,
> so they have an "origin" but not a "location".
>

Right.  That's the point of "origin".  It will be up to the loader whether
or not to use "origin" to determine a location, if any.

Also, bear in mind that it's not just exotic locations like URLs that
> aren't filenames.  zipimport uses pseudo-filenames that pretend a
> zipfile is a directory, by prepending the zipfile's filename to a path
> that's within the zipfile.  So, calling this "filename" is *really* a
> bad idea; it's not always a filename for even stdlib importers, let
> alone anything third-party!
>

Yeah, that has always bugged me about "__file__".  The upcoming revision of
the PEP uses the combo of "origin" and "set_fileattr" (a bool) instead of
"filename".


> > ``path``
> >
> > The list of path entries in which to search for submodules if this
> > module is a package.  Otherwise it is ``None``.
>
> This should probably be called submodule_path or
> submodule_search_locations or something, to avoid even *more*
> overloading of the word "path".  ;-)
>

I came to the same conclusion and was planning on using "path_entries".
 However perhaps something even more explicit, like
"submodule_search_locations", would be better. :)

>    If not passed in, ``path`` is set to an empty list if
>
>    ``is_package`` is true.  Then the directory from ``filename`` is
> >    appended to it, if possible.  If ``is_package`` is false, ``path``
> >    stays unset.
>
> How does this interact with namespace packages?  Does it?
>

Namespace packages won't use this method, so nothing will be populated
dynamically.


> > ``load(module=None, *, is_reload=False)``
> >
> > This method captures the current functionality of and requirements on
> > ``Loader.load_module()`` without any semantic changes, except one.
> > Reloading a module when ``exec_module()`` is available actually uses
> > ``module`` rather than ignoring it in favor of the one in
> > ``sys.modules``, as ``Loader.load_module()`` does.
>
> Interesting -- this could possibly be leveraged to implement
> multi-version imports.
>

I'm planning on splitting reload() out from load() so those semantics would
go away.  However, there may be room to still provide the same
functionality.  What would be needed for multi-version imports? (Is that
question opening a can of worms? <wink>)


> > This means that
> > ``is_reload`` could be dropped as a parameter.  However, doing so would
> > mean we could not use ``None`` to indicate that the module should be
> > pulled from ``sys.modules``.
>
> Wait, what?  That doesn't seem true to me: why not just use the module
> or pull one according to whether it's None or not?  What actual
> difference does is_reload really make here?
>

With a separate reload() this point is moot.


> > Regarding the first part of ``load()``, the following describes what
> > happens.
>
> I'm thinking maybe this should be parameterized to allow passing in a
> 'modules' dictionary other than sys.modules.  This would make
> multi-version imports or other "isolated environment" imports more
> viable, and factor out another global element of the import system.
> That way, if you implement an isolated module system, you don't have
> to duplicate or subclass ModuleSpec to perform the same loading
> functionality.
>

Cool idea, but couldn't this wait.  I could totally see this as part of PEP
406 (import engine).


> > Unfortunately, the ability to proxy does not extend to ``id()``
> > comparisons and ``isinstance()`` tests.
>
> Who does id() tests on loaders?


Which is why I'm not going to worry about it too much. :)


>  isinstance() fudging, OTOH, is quite
> doable.  See the ProxyTypes library on PyPI for an example; it's
> 2.x-only but I believe somebody has done a proof-of-concept port (due
> to some __special__ methods being different or missing in 3.x)
>

The current plan is to simply implement __subclasshook__() on the various
importlib ABCs, and perhaps other loaders, to check for methods.  Some of
the ABCs in collections.abc (like Iterator) do this.

> Finders
> > -------
> >
> > Finders will now return ModuleSpec objects when ``find_module()`` is
> > called rather than loaders.  For backward compatility, ``Modulespec``
> > objects proxy the attributes of their ``loader`` attribute.
>
> Has anybody looked at how this change affects pkgutil's (and
> setuptools') generic function-based extensions to PEP 302?  Currently,
> you can register specific loader types with these guys, but that'll
> likely break if importlib is going to start wrapping loaders without
> those tools' knowledge.
>

Good point.  I'll look into this.


>
> May I suggest adding a new finder method, find_module_spec() instead?
> Then, implement it for finders that don't support it by calling
> find_module() and wrapping the loader with a ModuleSpec.  This
> approach would be less disruptive to code that already uses
> find_module and inspects loader types to add extension protocols.
>

I consider this a last resort--i.e. if we can't find a way to make
find_module() work for us in a simple enough way.  I just cringe at the
idea of bolting on another backward-compatibility-induced method,
particularly when it's the OOTDI and the existing name is better fit for
the new functionality than old and re-purposing find_module() is within
reach.

> Adding another similar method to avoid backward-compatibility issues
> > is undersireable if avoidable.  The import APIs have suffered enough,
> > especially considering ``PathEntryFinder.find_loader()`` was just
> > added in Python 3.3.  The approach taken by this PEP should be
> > sufficient to address backward-compatibility issues for
> > ``find_module()``.
>
> I'm not sure I'm following here: are you saying that all PEP 302
> finders implemented by anyone, anywhere, must be changed *in order to
> work at all*, when this lands in a *minor version change*?
>

Existing finders and loaders will continue working as-is.  I've already got
this working in a rough implementation, so it's not that big a stretch.


> > Other Changes
> > -------------
>
> This section doesn't address impact on pkgutil, which makes
> significant use of the PEP 302 API.
>

I'll add that in.  Thanks for bringing it up.  My draft implementation is
passing all the pkgutil tests, but I wouldn't be surprised if I've missed
something here.

Anyway, thanks for the feedback.  I'll post an update to the PEP in the
next day or two.

-eric

_______________________________________________
Import-SIG mailing list
[email protected]
http://mail.python.org/mailman/listinfo/import-sig