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

Ensure that ContextPlugins are sorted before InstancePlugins for similar order #368

Open
buddly27 opened this issue Apr 11, 2020 · 6 comments

Comments

@buddly27
Copy link
Contributor

In most cases, it seems that the convention is to use ContextPlugin for the Collection stage and InstancePlugin for the following stages. However there might be some exception, for instance the InstancePlugin could be used in the Collection stage to initialize configuration of newly created instances.

For instance:

class Collect(pyblish.api.ContextPlugin):
    """Context plugin to create dummy instances."""

    order = pyblish.api.CollectorOrder
    label = "Create dummy instances"

    def process(self, context):
        context.create_instance("Foo")
        context.create_instance("Bar")

class Construct(pyblish.api.InstancePlugin):
    """Instance plugin to initiate data for each instances."""

    order = pyblish.api.CollectorOrder + 0.1
    label = "Initiate default data"

    def process(self, instance):
        instance.data.update({...})

The order attribute is important to ensure that the InstancePlugin is always picked up after the ContextPlugin. But thinking about it more, I feel like it never really makes sense to have an InstancePlugin before a ContextPlugin, so I was wondering if there could be a more elegant solution to ensure that for plugins with the same order, ContextPlugin would always be sorted before InstancePlugin.

I thought about extending the sorting logic as follows:

def sort(plugins):
    """Sort `plugins` in-place."""
    if not isinstance(plugins, list):
        raise TypeError("plugins must be of type list")

    def _sort_by_type(plugin):
        """Return 1 if plugin operates on instances, 0 otherwise."""
        return 1 if plugin.__instanceEnabled__ else 0

    plugins.sort(key=lambda plugin: (plugin.order, _sort_by_type(plugin)))
    return plugins

What do you think?

@mottosso
Copy link
Member

It sounds like a solid convention, but it wouldn't be possible to change this logic as it would break several years of plug-ins written without it. It's also possible it doesn't fit with everyone's preference.

In this case, I would establish this convention internally, for example..

  1. Designate the lower half of each order, 0.0-0.5 to context plug-ins, and the upper to instance plug-ins
  2. Use a function for setting the order, that looks at which superclass it's being called in. If ContextPlugin, fit the value between 0.0-0.5
import pyblish.api
import mypipeline

class Collect(pyblish.api.ContextPlugin):
    """Context plugin to create dummy instances."""

    order = mypipeline.get_collector_order() # => CollectorOrder + 0.0
    label = "Create dummy instances"

class Construct(pyblish.api.InstancePlugin):
    """Instance plugin to initiate data for each instances."""

    order = mypipeline.get_collector_order(0.1) # => CollectorOrder + 0.6
    label = "Initiate default data"

Another alternative may be to establish a register_custom_sort, to provide your own sorting function for plug-ins. The important bit is that we maintain backwards compatibility, with as little added complexity as possible.

@BigRoy
Copy link
Member

BigRoy commented Apr 12, 2020

Admittedly I don't see a reason to change this. I think order being explicit and be sole sorting key keeps things simple.

We do have quite some ContextPlugins running over some instances so they are in-between of InstancePlugins too. The ordering on a single order value I consider random which is fine... if I want a specific order I'd do +0.1 or so.

@tokejepsen
Copy link
Member

I agree with @BigRoy. Better to be explicit than implicit here.

I would say though that implementing adding the ability to register your own custom sorting function, would be a nice addition.

@buddly27
Copy link
Contributor Author

buddly27 commented Apr 12, 2020

I'm not sure I understand how this could break any pipeline though, all I'm suggesting is to add a rule when there are no rules:

Before:

  • Plugins are ordered with the order attribute
  • If the order attributes are the same for several plugins, the order cannot be deterministic anymore
plugins.sort(key=lambda plugin: plugin.order)

After:

  • Plugins are ordered with the order attribute
  • If the order attributes are the same for several plugins, then order per plugin type
plugins.sort(key=lambda plugin: (plugin.order, _sort_by_type(plugin)))

Also I don't see how this would make the sorting logic less explicit..

Or am I missing something?

@mottosso
Copy link
Member

I'm not sure I understand how this could break any pipeline though, all I'm suggesting is to add a rule when there are no rules:

That's true, I see what you mean now. I think we all thought that ContextPlugin's would come before InstancePlugins no matter what.

In that case, the first step is putting this in a PR. That will trigger the automated tests on all platforms, and if those work then that'll be a good indicator nothing supported has broken.

@buddly27
Copy link
Contributor Author

Just submitted the PR, previous tests seems to work fine and I added an extra one to ensure that it behaves as expected. Let me know what you think!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants