pyblish / pyblish-base

Pyblish base library - see https://github.com/pyblish/pyblish for details.
Other
123 stars 60 forks source link

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

Open buddly27 opened 4 years ago

buddly27 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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.sort(key=lambda plugin: plugin.order)

After:

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 commented 4 years ago

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 commented 4 years ago

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!