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

Setup pluggy with locking plugin #965

Draft
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

soapy1
Copy link
Contributor

@soapy1 soapy1 commented Nov 7, 2024

Fixes #929

Description

This set's up conda-lock to be run as a plugin. By running the locking functionality as a plugin, opposed to a builtin action, we will be able to more easily swap out locking implementations. This is helpful for developer experimentation and providing users with another way of customize their conda-store install.

Some things that this PR adds:

  • adds pluggy as a dependency
  • adds a plugin manager for collecting and loading plugins
  • moves locking action to the conda-store lock plugin
  • adds a plugin context object for sharing required state and functionality between plugin runs
  • adds a config option to set the default lock plugin

For a more complete demo of how this approach can be extended, see main...soapy1:conda-store:plugin-demo-pluggy. For a more complete description of what is added by this demo see the conversation in GH.

Next steps

fast follow pr's

  • persist the default lock plugin selection to the settings db table (and pull the default value from there)

opportunities to extend the use of plugins

  • allow users to define their own plugins
    • (once the plugin use case is more flushed out) document how to build user plugins
  • it would be nice to move some other parts of conda-store to be plugins. For example, the storage backends, or other actions
  • try out other lock backends

Pull request checklist

  • Did you test this change locally?
  • Did you update the documentation (if required)?
  • Did you add/update relevant tests for this change (if required)?

Copy link

netlify bot commented Nov 7, 2024

Deploy Preview for conda-store canceled.

Name Link
🔨 Latest commit 3ce4a49
🔍 Latest deploy log https://app.netlify.com/sites/conda-store/deploys/672d2f2b3b1cfb00085c6a16

Copy link

netlify bot commented Nov 7, 2024

Deploy Preview for conda-store canceled.

Name Link
🔨 Latest commit ab1cdf9
🔍 Latest deploy log https://app.netlify.com/sites/conda-store/deploys/6740e89ebb836d00084ea4f5

@soapy1 soapy1 force-pushed the setup-pluggy branch 5 times, most recently from 0309540 to 8ebc697 Compare November 8, 2024 00:37
import uuid


class PluginContext:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is mostly a copy/paste from the ActionContext

self.conda_flags = conda_flags

@hookspec.hookimpl
def lock_environment(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is mostly a copy/paste from the solve_lockfile_action

settings = self.get_settings(db)

# Load lock plugin
self.load_plugin_by_name(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

currently loading the lock plugin globally by it's name. In the future, when more lock plugins are available, this should be configurable. See the pluggy demo for more examples of how this can be done.


def collect_plugins(self):
"""Registers all availble plugins with the plugin registry"""
for plugin in BUILTIN_PLUGINS:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, there is only one builtin plugin. As we add more, we'll want to register them here. And we'll probably also want to support loading user defined/installed plugins

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@soapy1 soapy1 marked this pull request as ready for review November 8, 2024 00:42
@soapy1 soapy1 added needs: discussion 💬 This item needs team-level discussion before scoping needs: review 👀 labels Nov 8, 2024
@soapy1 soapy1 requested a review from jaimergp November 8, 2024 01:14
@soapy1 soapy1 force-pushed the setup-pluggy branch 2 times, most recently from 2cc11d8 to fc992ff Compare November 11, 2024 21:48
self.conda_flags = conda_flags

@utils.run_in_tempdir
@hookspec.hookimpl
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I was expecting the implementations to live in a different abstraction level, not so close to the actual code. For reference, this is how conda does it.

Copy link
Member

@jaimergp jaimergp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @soapy1! I like what I'm seeing, but I'm wondering if we could leverage some "community knowledge" and steal what conda is doing for their hooks. They have one more layer in between the plugin and the hook, and the best part if that they have documented it veeery well. Do you think you can take a look at their setup and see what things we could benefit from? 🙏

@soapy1 soapy1 force-pushed the setup-pluggy branch 6 times, most recently from 1794bcd to d1f3549 Compare November 20, 2024 22:13
@soapy1
Copy link
Contributor Author

soapy1 commented Nov 20, 2024

@jaimergp I still have more work to do with docs + tests, but wondering if you could take a gander to see if this is more the implementation you were thinking. Thanks!

@jaimergp
Copy link
Member

Yep, I only took a quick look, but that's more aligned with what I had in mind!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs: discussion 💬 This item needs team-level discussion before scoping needs: review 👀
Projects
Status: In review 👀
Development

Successfully merging this pull request may close these issues.

[ENH] - Investigate moving to a plugin based architecture
2 participants