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

Add plugin support for loaders. #3657

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

rhattersley
Copy link
Member

This suggestion uses iris.io.plugins as a namespace package. Each plugin would need to provide one or more modules that install into the same namespace package. All the plugin modules get imported when iris is imported (strictly speaking, when iris.fileformats is imported), and the contents of any FORMAT_SPECIFICATIONS iterables get added to the FORMAT_AGENT.

NB. This is just an iniitial technical suggestion (e.g. it has no documentation!) and place to host the general discussion.

@rhattersley rhattersley mentioned this pull request Feb 14, 2020
@rhattersley
Copy link
Member Author

The test failure is genuine and is caused by setup.py not installing the new namespace package - it actively filters out empty directories. 😒

@bjlittle bjlittle self-assigned this Feb 17, 2020
@bjlittle bjlittle added this to In Progress in Iris v3.0.0 via automation Feb 17, 2020
@bjlittle bjlittle added this to the v3.0.0 milestone Feb 17, 2020
@bjlittle
Copy link
Member

@rhattersley Welcome back 😄 ... where's this PR at? Good for review or still WIP?

@rhattersley
Copy link
Member Author

It needs a technical review really. e.g. Is the native-namespace mechanism something we're happy to run with? Is iris.io.plugins the right name? I'll pull out the ABF/L loader as a separate repo to provide an example of what it's like to supply a plugin.

Once the technical stuff is settled, then we can sort out documentation, what's new, ... etc.

@rhattersley
Copy link
Member Author

For an example plugin (with super-simple, modern packaging) see SciTools/iris-abf. 😁

@rhattersley
Copy link
Member Author

NB. SciTools/iris-abf#1

@rhattersley
Copy link
Member Author

FWIW, I've updated the iris-abf package to include tests, use black formatting, and apply CI via GitHub actions.

The tests are brand new tests, with a small amount of "fruitiness" to work around an absence of iris in the test environment due to the inability to do "pip install iris". Hence, I couldn't just grab the tests from here. Installing iris would effectively require conda, which would complicate iris-abf and draw it away from the wholesome poetry, PEP 517, PEP 518, GitHub-actions goodness that it's currently using. This "I can't pip install iris" chicken-and-egg problem is one of the things this PR is seeking to address, but clearly more work is required elsewhere as well ... e.g. I'm looking at you, pyke & cf-units. 😒

@pp-mo
Copy link
Member

pp-mo commented Feb 25, 2020

example plugin (with super-simple, modern packaging) see SciTools/iris-abf.

Interesting.
Can you explain what the install process for this would be :

  • ? does it require to "inject itself" into an Iris installation
  • ? how would that work
  • ? can it support a local (or "--user") install
  • ? can we automate installation / install from a channel ?
  • ? is it waiting on iris, to adopt the 'fileformats plugins directory', before we can make this work ?

@rhattersley
Copy link
Member Author

rhattersley commented Feb 28, 2020

Thanks for the questions @pp-mo - some critical thinking is exactly what this PR needs!

* ? does it require to "inject itself" into an Iris installation

Yes - normally, "pip install iris-abf" would place the abf.py module into an existing Iris installation. You can remove it again cleanly with "pip uninstall iris-abf". So far so good.... ish.

* ? can it support a local (or "--user") install

No. 😞 I had rather hoped this would Just Work™️ as allowing for a single namespace package to be provided across multiple directories is one of their main use cases! Installing locally places the abf.py module into .local/.../site-packages/iris/io/plugins/abf.py, but an import iris.io.plugins.abf fails to find the module, presumably because iris and iris.io aren't namespace packages.

I'm thinking these namespace package shennanigans are perhaps a bit too bleeding edge. It would be lot a simpler, more robust, easier to understand, predicatable, etc. etc. to just go with the plain old naming convention approach. In which case we just rename iris/io/plugins/abf.py to something like iris_io_abf.py or even just iris_abf.py.

@bjlittle bjlittle removed this from In Progress in Iris v3.0.0 Sep 8, 2020
@bjlittle bjlittle removed this from the v3.0.0 milestone Sep 8, 2020
@pp-mo
Copy link
Member

pp-mo commented Nov 10, 2020

I still think we should implement this, but probably just a simple naming strategy as discussed above, so leaving this in place as an issue / reminder.
Also... there is an outstanding problem with loader selection : see #3918

@bjlittle bjlittle mentioned this pull request Jan 20, 2022
8 tasks
@trexfeathers
Copy link
Contributor

Related: #4798

@bjlittle bjlittle requested review from bjlittle and removed request for bjlittle October 5, 2022 09:55
@bjlittle bjlittle removed their assignment Oct 5, 2022
@trexfeathers
Copy link
Contributor

Possibly other example that would be ready to try: the NAME loader

@vsherratt vsherratt mentioned this pull request Feb 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

None yet

4 participants