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

Add topic on plugin discovery #294

Merged
merged 3 commits into from Apr 13, 2017

Conversation

Projects
None yet
3 participants
@theacodes
Member

theacodes commented Apr 11, 2017

Resolves #291

@theacodes theacodes requested a review from ncoghlan Apr 11, 2017

@theacodes

This comment has been minimized.

Member

theacodes commented Apr 11, 2017

@ncoghlan first draft, definitely needs a sanity check.

name for finder, name, ispkg
in pkgutil.iter_modules(myapp.plugins.__path__)]
.. TODO:: Is there a simple way to import both of these at the same time as discovery?

This comment has been minimized.

@simonvanderveldt

simonvanderveldt Apr 11, 2017

@jonparrott What kind of info are you looking for here?
If you just want to import the modules you find it's enough to simply add:

importlib.import_module(name)

If you only want to import packages and not modules you can check that based on the ispkg value

@simonvanderveldt

This comment has been minimized.

simonvanderveldt commented Apr 11, 2017

@jonparrott one question I recently ran into is wether or not to use a subpackage as a parent for the plugins. Your examples all use a subpackage myapp.plugins, was that a deliberate choice? If so, it might be of value to mention why?

(hope it's OK if I as a random bystander ask some questions/provide some feedback)

@ncoghlan

This looks good to me, but I have a few comments and suggestions inline that I'd like @jonparrott to take a look at before merging.

=======================

If all of the plugins for your application follow the same naming convention,
you can use :func:`pkgutil.iter_modules` to discover all of the top-level

This comment has been minimized.

@ncoghlan

ncoghlan Apr 11, 2017

Member

Huh, I just learned something, as I previously thought iter_modules had the same problem as walk_packages, where the latter can cause problems by eagerly importing everything that it thinks looks like a Python package. That may be dozens or even hundreds of unwanted packages when running in the system Python on Linux.

However, it turns out iter_modules is safe from that problem - it's strictly a consequence of the recursive walk behaviour in walk_packages.

So the naming convention approach is even better than I thought - I previously thought you needed to combine it with something like http://pythonhosted.org/distlib/tutorial.html#querying-a-path-for-distributions to get the listing of what was installed locally.

This comment has been minimized.

@theacodes

theacodes Apr 11, 2017

Member

Yeah, I thought so too, happy change. :)

.. _flask: https://flask.pocoo.org
.. _Flask-SQLAlchemy: https://flask-sqlalchemy.pocoo.org/
.. _Flask-Talisman: https://pypi.python.org/pypi/flask-talisman

This comment has been minimized.

@ncoghlan

ncoghlan Apr 11, 2017

Member

Potentially worth mentioning here is that of the three approaches, the naming convention approach is the only one that can be reliably queried through the "simple" API for index servers: https://www.python.org/dev/peps/pep-0503/#specification

This is due to the fact that the simple API only exposes project names and artifact file names - it doesn't publish any of the other metadata that's available through the XML-RPC and JSON APIs.

And even if you do access those other APIs, they don't currently provide import module or entry point metadata anyway.

This comment has been minimized.

@theacodes

:doc:`Namespace packages <namespace_packages>` can be used to provide a
convention for where to place plugins and provides a way to perform discovery.
For example, if you make the package ``myapp.plugins`` a namespace package

This comment has been minimized.

@ncoghlan

ncoghlan Apr 11, 2017

Member

Missing comma at the end of the line.

This comment has been minimized.

@theacodes

theacodes Apr 11, 2017

Member

Intentional, add a then to make it clearer. :)

name for finder, name, ispkg
in pkgutil.iter_modules(myapp.plugins.__path__)]
.. TODO:: Is there a simple way to import both of these at the same time as discovery?

This comment has been minimized.

@ncoghlan

ncoghlan Apr 11, 2017

Member

Switching the comprehension to be {name: importlib.import_module(name) for ...} should make it line up with your example output below.

This comment has been minimized.

@theacodes

theacodes Apr 11, 2017

Member

So here's the rub, for a namespace packages it actually returns ['a', 'b'] not ['myapp.plugins.a', 'myapp.plugins.b']. I took a shot at something, but let me know if ya'll have a better way.

This comment has been minimized.

@ncoghlan

ncoghlan Apr 12, 2017

Member

Ah, right - because we feed in the path directly, iter_modules doesn't know what the package prefix should be unless we provide it explicitly.

Rather than passing package to import_module (that's for resolving relative references that start with a leading '.'), you'll instead want to set the package prefix appropriately:

def iter_namespace(ns_pkg):
    return pkgutil.iter_modules(ns_pkg.__path__, ns_pkg.__name__ + ".")

This comment has been minimized.

@theacodes

theacodes Apr 12, 2017

Member

@ncoghlan what do you think about the new approach I used of using the second argument to importlib.import_module? I think that's a bit cleaner than the string concatenation.

If you're good with that, let's merge. :)

This comment has been minimized.

@ncoghlan

ncoghlan Apr 13, 2017

Member

@jonparrott importlib.import_module ignores its second argument when given module names that look absolute (i.e. no leading .). That means to get it to work, you'd have to:

  1. Specify '.' as the prefix when calling iter_modules
  2. Specify ns_pkg.__name__ as the package when calling import_module

Hence the suggestion of instead encapsulating those details in a helper function that just iterates over the given namespace package, producing absolute module names that can be passed directly to importlib.import_module or importlib.util.find_spec.

This comment has been minimized.

@ncoghlan

ncoghlan Apr 13, 2017

Member

Note that I also think iter_submodules (since it's not actually specific to namespace packages) would make a good addition to pkgutil in 3.7+, accepting an already imported package as its sole argument, rather than a list of path entries and an optional prefix. That's orthogonal to the recommendation for how folks can implement their own iter_submodules today, though.

This comment has been minimized.

@theacodes

theacodes Apr 13, 2017

Member

Gotcha. Done. :)

@theacodes

This comment has been minimized.

Member

theacodes commented Apr 11, 2017

one question I recently ran into is wether or not to use a subpackage as a parent for the plugins. Your examples all use a subpackage myapp.plugins, was that a deliberate choice? If so, it might be of value to mention why?

@simonvanderveldt good call, I added a little note about that.

(hope it's OK if I as a random bystander ask some questions/provide some feedback)

Absolutely!

@ncoghlan all comments addressed, please take another look? :)

@ncoghlan

This comment has been minimized.

Member

ncoghlan commented Apr 13, 2017

All looks good to me now, with the one exception that I'm fairly sure the current example namespace iteration code won't work unless your current directory is the myapp.plugins directory (hence making its contents importable under their unprefixed names)

@theacodes theacodes force-pushed the theacodes:plugins branch from 8a3f706 to fbdfdf4 Apr 13, 2017

@theacodes

This comment has been minimized.

Member

theacodes commented Apr 13, 2017

Fixed up the namespace sample and verified that it works. Merging. Thanks, @ncoghlan!

@theacodes theacodes merged commit 22b14f6 into pypa:master Apr 13, 2017

@theacodes theacodes deleted the theacodes:plugins branch Apr 13, 2017

ncoghlan added a commit to ncoghlan/python-packaging-user-guide that referenced this pull request Jun 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment