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.

Sign up for GitHub

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

Jump to bottom

Extension plugins #1040

Merged
merged 31 commits into from
Jun 8, 2021
Merged

Extension plugins #1040

merged 31 commits into from
Jun 8, 2021

Conversation

davidlatwe
Copy link
Contributor

@davidlatwe davidlatwe commented Mar 12, 2021 •
edited
Loading

Feature Propose

Allowing people registering their own sub-command, forwarding function, configs.

What's Added (updated)

  • New type plugin "Command"
  • New plugin loading mechanism : scan sys.path for module that has rezplugins sub-module
  • Warning messages for plugin name clashing related issue

Why scan modules for plugins ?

The new command type plugin enables registering custom sub-command, which may need to access additional module to do it's job. Hence we have to install the module which is required by the sub-command into Rez's venv, so that it could be imported in production installed Rez session which is ignoring PYTHONPATH. With these in mind, installing custom rezplugins as a sub-module would be the most desirable way. Now, how to let Rez register plugin that has been installed into lib/site-packages ? Instead of registering full path into rezconfig.plugin_path, simply scan modules from sys.path is most friendly.

Note

When name clashing happen, plugins loaded by the new loading mechanism will take precedence over plugins that are registered in rezconfig.plugin_path.

Please have a visit to my example plugin and have a look how this works 👉🏼 https://github.com/davidlatwe/rezbefoo (outdated)

@davidlatwe davidlatwe mentioned this pull request Mar 12, 2021
Closed
3 tasks
@nerdvegas
Copy link
Contributor

Hey David, I really like this. Some thoughts:

  • "Application" isn't the right terminology I think, "extension" would be better. The main reason I say this is because rez is actually missing a construct that I think of as an "app instance." Ie, somewhere to store package repo cache states, current config and so on (which is global state presently, and unfortunately). So I'd rather keep the term 'app' spare for if/when we make a change like this.
  • "plugin_module" is too vague. With the above in mind I would simply go with "extensions".
  • In a production install, you're going to want to install this into the rez venv. Even though doing that is straightforward (just pip install into the venv) it requires knowledge of rez internals (ie that it has its own venv in the first place). With that in mind, we should probably have a rez-install-ext tool which does this.

Thoughts?

@instinct-vfx
Copy link
Contributor

Hey,

i also really like this. I do agree with Allan's points. Also i wonder, as discussed before things like rez-pip might (should?) become separate projects, would this be the mechanism to install them?

@nerdvegas
Copy link
Contributor

nerdvegas commented Mar 15, 2021 via email

@instinct-vfx
Copy link
Contributor

Great stuff. I am totally in love with the idea of a more modular Rez and an ecosystem of extensions that you can use if you have a use case (think other package managers like nuget, chocolate or vcpkg or the likes on windows)!

@davidlatwe what do you think about the changes Allan suggested?

@nerdvegas
Copy link
Contributor

nerdvegas commented Mar 15, 2021 via email

@nerdvegas
Copy link
Contributor

Just by chance I ran into this today, illustrating my point about an Application class: #1033

@davidlatwe
Copy link
Contributor Author

Thanks @nerdvegas and @instinct-vfx !!

About naming, I agree we should avoid using the term "Application" for this, but not so sure about using "Extension" for that plugin type instead. Renaming the config entry "plugin_module" into "extensions" make sense to me as well, since the plugin manager can load all types of plugin from there. With this in mind, naming the new type of plugin to "Extension" makes me think the config "extensions" can only use to load that specific plugin type. Maybe "Supplement" or "Add-on" would be better name for new plugin type ? I will push the commit that reflect the points above first, and rename again if my point make sense to you guys as well.

About additional "rez-install-ext" tool, I was hoping that if we could just use venv's pip to do this, or with rez-python -m pip, hence the PR #1039 and #1041. Since it would be better if rez and it's extensions can use same installation method/standard. Still, what will "rez-install-ext" do ? Like, checking target environment if is a valid Rez production install before running pip ?

@nerdvegas
Copy link
Contributor

Hey I don't quite follow... do you mean that you're thinking ahead to where all plugins are loaded this way, and that 'plugin_modules' would encompass all plugin types, not just extensions? In any case that brings up another point.. it'd be great if that config entry were not necessary at all tbh. It's an odd extra step - with most tools, installing a new plugin is all you need to do, the system then knows about it. It'd be good if we could do the same.

Wrt rez-install-ext. I would not even want the user to have to run rez-python -m pip foo to install an extension, imo this is already too much detail. Providing rez-install-ext (or rez-install-plugin let's say) is future proofing - there may be other things we need to do as part of that process. The fact that pip is used as part of plugin installation at all should be of no interest to the user.

What if then, we start thinking about a new plugin architecture at the same time; and we think about how to ditch the config setting. We could have this:

  • rez-install-plugin is used to install plugins
  • it pip installs them into rez's venv
  • we introduce a new type of plugin called an "extension". This type of plugin provides a cli tool
  • as well as supporting "rez foo" extension form, we also support "rez-foo". Rez-install-plugin takes care of this
  • we don't have to update a config setting; rez-install-plugin tracks this itself
  • initially we don't support plugin installation in pip-based rez installs perhaps? Or perhaps we can, but the initial emphasis would be on getting this to work for production installs.
  • this all has to work in combination with old-style (ie existing) plugins.

?

ps - one way we might track installed plugins is to iterate over all visible modules and expect certain standard attributes (rez_plugin_type, rez_plugin_name for eg) that identify them as rez plugins.

@davidlatwe
Copy link
Contributor Author

do you mean that you're thinking ahead to where all plugins are loaded this way, and that 'plugin_modules' would encompass all plugin types, not just extensions?

No, not thinking ahead, it already does that. Since the current "extension" layout like what I have in my example rezbefoo, the rezplugins is a sub-module of rezbefoo, so whatever plugin type it contains, will get loaded from the "plugin_modules".

we don't have to update a config setting; rez-install-plugin tracks this itself

That would be awesome :D

as well as supporting "rez foo" extension form, we also support "rez-foo". Rez-install-plugin takes care of this

I'll have a few more tests on #1039 since I thought this might still a bit related, e.g. to use entry_points or not in the setup.py.

@nerdvegas
Copy link
Contributor

nerdvegas commented Mar 16, 2021 via email

@davidlatwe
Copy link
Contributor Author

davidlatwe commented Mar 16, 2021 •
edited
Loading

No worries ! And yes, old-style plugin that found via "plugin_path" can live with new that found via "plugin_modules" (renamed into "extensions" at this point), and all listed by rez -i 🥳

Here's the pseudo code of current plugin loading implementation:

# in plugin_managers.py
def extend_path(path, name):
    """Extend a package's path."""
    def append_if_valid(dir_):
        # do some check before appending
        path.append(subdir)

    for dir_ in config.plugin_path:  # old-style
        append_if_valid(dir_)

    for name in config.extensions:  # new-style
        # try load the module
        module_path = sys.modules[name].__path__
        for dir_ in module_path:
            append_if_valid(dir_)

@nerdvegas
Copy link
Contributor

nerdvegas commented Mar 16, 2021 via email

@davidlatwe
Copy link
Contributor Author

I reckon dropping the config setting is a must (because old-style plugins don't require doing this, so neither should new-style ones!)

Sorry, just to be sure. :P
Are you saying that to drop new config setting "plugin_module" and replace it with module iteration approach or maybe other, as long as no additional setting required ?

@nerdvegas
Copy link
Contributor

nerdvegas commented Mar 16, 2021 via email

@davidlatwe
Copy link
Contributor Author

because that's already the case for old-style plugins.

:O
I didn't know about this, I thought the only way to load them is from "pluing_path" config setting !

Anyway, I'll get started :D

@nerdvegas
Copy link
Contributor

nerdvegas commented Mar 16, 2021 via email

@davidlatwe davidlatwe changed the title Application plugins Extension plugins Mar 16, 2021
@davidlatwe
Copy link
Contributor Author

I feel like the problem is a bit of an edge case and that requiring a MAIN
in every plugin root is overkill.

Ah no, we won't need that MAIN file in every plugin root, only the main rezplugins module. Anyway, will re-visit this in another PR. :)

Test failed on latest commit. Looks like there's something wrong with the test data, seems the material that required for the new tests were not being installed and leads to failed test. Will investigate this later.

@davidlatwe
Copy link
Contributor Author

Okay, that's it.
Requested changes have been addressed, tests added. And there's an example extension which contains a simple functional command type plugin that can be installed, but owning to we haven't implement the extension installing tool, I only left a brief pip install instruction in the README.

@davidlatwe
Copy link
Contributor Author

Top comment of this PR has been updated to reflect changes.

@nerdvegas
Copy link
Contributor

Hey David, cheers! I will get to this, but bear in mind I have an FMX talk coming up and it may be derailed by that for a bit as I'll have to spend time over a few weeks prepping for it.

A couple of thoughts (no need to act on these now! It's so I don't forget):

  • It may be good to update existing (builtin) commands so they are also Command- based, simply for sake of consistency.
  • When we have a rez-extensions tool, it might be good to have it update some command list stored in the rez installation. This would allow us to skip the module scan, which might be good if this takes appreciable time to perform. But if it's quick then I probably wouldn't bother.
  • We should consider renaming Command to CLICommand to avoid confusion, as there are "package commands" already.

@davidlatwe
Merge branch 'master' into application-plugins
88b509d 
# Conflicts:
#	src/rez/data/tests/extensions/bar/__init__.py
#	src/rez/data/tests/extensions/bar/rezplugins/__init__.py
#	src/rez/data/tests/extensions/bar/rezplugins/package_repository/__init__.py
#	src/rez/data/tests/extensions/bar/rezplugins/package_repository/memory.py
#	src/rez/data/tests/extensions/foo/__init__.py
#	src/rez/data/tests/extensions/foo/rezplugins/__init__.py
#	src/rez/data/tests/extensions/foo/rezplugins/package_repository/__init__.py
#	src/rez/data/tests/extensions/foo/rezplugins/package_repository/cloud.py
#	src/rez/data/tests/extensions/non-mod/rezplugins/__init__.py
#	src/rez/data/tests/extensions/non-mod/rezplugins/package_repository/__init__.py
#	src/rez/data/tests/extensions/non-mod/rezplugins/package_repository/memory.py
@davidlatwe
update test
c624f0e
@davidlatwe
fix test data path
1368513
@maxnbk
Copy link
Contributor

maxnbk commented Mar 30, 2021

I just want to jump in to say, awesome work here so far @davidlatwe .

@davidlatwe davidlatwe mentioned this pull request Mar 31, 2021
Closed
@davidlatwe davidlatwe mentioned this pull request Apr 21, 2021
Merged
@nerdvegas
Copy link
Contributor

Hey I'm just looking at this now and I see an issue. Currently, the plugin manager searches all packages in sys.path and will assume its found a plugin type if it finds a subdir in the package with matching name (eg "build_system"). There are two problems with this though:

  • it's a little broad - there could be a package present that has nothing to do with rez, that has a submodule with a name like this;
  • it's searching too many paths. For example, it will search a path like /home/ajohns/rez/2.90.0/local/lib/python2.7/site-packages/coverage 7 times (one for each plugin type) - it'd be good to discard this path just once instead.

To that end, how about we mandate that a __rez__.py file must be present in the package root? That way we can discard packages earlier, and avoid potentially picking up modules that aren't rez plugins at all.

@davidlatwe
Copy link
Contributor Author

I have thought about this earlier before, and was considering using python package's entry_points to register them as rez's python plugin.

But adding __rez__.py seems much simpler. :)

@instinct-vfx
Copy link
Contributor

Reading Allan's comment i was actually going to suggest using entry_points. They seem very well suited for this task and are specifically designed to support plugin-like discovery mechanisms. I would actually prefer that route.

@davidlatwe
Copy link
Contributor Author

davidlatwe commented Jun 1, 2021 •
edited
Loading

But with entry_points, one would need to actually install it as a python package, which...
which is required right ? Because in Rez production install, PYTHONPATH won't work so you have to install it.

Oh, but entry_points may not work if that Rez is as a rez package (no correct entry_points to read). 🤔

@instinct-vfx
Copy link
Contributor

Could you elaborate on the last one? Why would it not work if Rez is a rez package? PYTHONPATH is a good point hm.

@davidlatwe
Copy link
Contributor Author

Sorry, that was wrong. It doesn't matter if Rez is as a rez package or not, but which Python is being used. Because that will affect where the python package metadata will be looked for entry points, correct ?

But if it's using __rez__.py then as long as it can be found in sys.path, it can be loaded.

@nerdvegas
Copy link
Contributor

nerdvegas commented Jun 1, 2021 via email

@instinct-vfx
Copy link
Contributor

That makes sense.

@davidlatwe
Copy link
Contributor Author

Just re-read the comment and,

Currently, the plugin manager searches all packages in sys.path and will assume its found a plugin type if it finds a subdir in the package with matching name (eg "build_system").

I found that isn't entirely true, there still need rezplugins namespace on top of build_system for rez to include the module. But the current implementation indeed wasting search time on packages that are obviously has nothing to do with rez.

So, I will just adding rezplugins folder existence check for now, since I think it works the same as __rez__.py.

@nerdvegas
Copy link
Contributor

nerdvegas commented Jun 7, 2021 via email

@nerdvegas nerdvegas merged commit 8ae826c into AcademySoftwareFoundation:master Jun 8, 2021
@davidlatwe davidlatwe deleted the application-plugins branch June 8, 2021 04:53
@cuckon cuckon mentioned this pull request Jul 3, 2021
Open
@davidlatwe davidlatwe mentioned this pull request Jul 26, 2021
Merged
@Jawabiscuit Jawabiscuit mentioned this pull request May 16, 2023
Open
39 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nerdvegas nerdvegas nerdvegas left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@davidlatwe @nerdvegas @instinct-vfx @maxnbk