Simplify Iterator

[mottosso]

To help with #250, the Iterator has seen some simplification that should help in understanding how it works.

Before

def Iterator(plugins, context):
    for plugin in plugins:
        instances = instances_by_plugin(context, plugin)

        # Run once for every instance, plus once for context
        for instance in instances + [None]:
            if instance is None and plugin.__instanceEnabled__:
                continue

            yield plugin, instance

After

def Iterator(plugins, context):
    for plugin in plugins:
        instances = instances_by_plugin(context, plugin)

        if plugin.__instanceEnabled__:
            for instance in instances:
                yield plugin, instance

        else:
            yield plugin, None

[mottosso]

And here's an example of what #250 might look like in the Iterator.

def Iterator(plugins, context):
    """Primary iterator

    This is the brains of publishing. It handles logic related
    to which plug-in to process with which Instance or Context,
    in addition to stopping when necessary.

    Arguments:
        plugins (list): Plug-ins to consider
        context (list): Instances to consider

    """

    for plugin in plugins:
        instances = instances_by_plugin(context, plugin)

        if plugin.__instanceEnabled__:
            for instance in instances:
                yield plugin, instance

        else:
            families = set()
            for instance in context:
                families.add(instance.data["family"])
                families.update(instance.data.get("families", []))

            has_support = set(families) & set(plugin.families)
            has_support = has_support or "*" in plugin.families
            if has_support:
                yield plugin, None

[coveralls]

Coverage decreased (-0.008%) to 95.783% when pulling aecae8fb45a0d517c5ab77d99701c42aa157365f on mottosso:master into 62d767f4386d33e7cb18f544086ac128af02c1bc on pyblish:master.

[tokejepsen]

How do you determine whether a plugin is an InstancePlugin or ContextPlugin? I thought you say that plugin.__instanceEnabled__ and plugin.__contextEnabled__ shouldn't really be used since its being deprecated from v1.3.

[mottosso]

I thought you say that plugin.instanceEnabled and plugin.contextEnabled shouldn't really be used since its being deprecated from v1.3.

You are right, they are, these are used for backwards compatibility.

Normally I would use issubclass.

[tokejepsen]

I've had a brief look at this, but I can't understand how the iterator is used throughout pyblish. In my head its a matter of changing the instances_by_plugin method so a ContextPlugin by default get all instances because it has all families. The problematic line is here where no instances are said to be compatible with a ContextPlugin, but all instances should be. Then in theory you just need to make a difference between InstancePlugin and ContextPlugin in the iterator:

# pseudo code
for plugin in plugins:
    instances = instances_by_plugin(context, plugin)
    if issubclass(plugin, InstancePlugin):
        for instance in instances:
            yield plugin, instance

    if issubclass(plugin, ContextPlugin):
        if instances:
            yield plugin, None

Now if you modify the families property of a ContextPlugin to a specific family, we should get the behaviour we want.

[mottosso]

The problematic line is here where no instances are said to be compatible with a ContextPlugin, but all instances should be.

That's one of the lines changed in this PR.

• https://github.com/pyblish/pyblish-base/pull/264/files#diff-8a141b4b17439c8c6dc7ef52dd054bb0L185

[mottosso]

I've had a brief look at this, but I can't understand how the iterator is used throughout pyblish.

The iterator is a rather new member, but it should encapsulate any point where you need to iterate through plug-in and instance pairs. You would use it when for example building a new GUI, such that you won't have to think about these delicate matters of which plug-in processes which instances or context.

Here's how you would use it.

from pyblish import api, logic
context = api.Context()

class MyPlugin(api.ContextPlugin):
  def process(self, context):
    print("Hello")

for plugin, instance in logic.Iterator([MyPlugin], context):
  print("Processing %s with %s" % (plugin, instance))

In my head its a matter of changing the instances_by_plugin method so a ContextPlugin by default get all instances because it has all families.

I had the exact same thought, and I did quickly try this, but couldn't get it to work. Try it out for yourself, I must have missed something.

[tokejepsen]

This works for me, with the below test.

from .plugin import Validator, InstancePlugin, ContextPlugin

def Iterator(plugins, context):
    """Primary iterator

    This is the brains of publishing. It handles logic related
    to which plug-in to process with which Instance or Context,
    in addition to stopping when necessary.

    Arguments:
        plugins (list): Plug-ins to consider
        context (list): Instances to consider

    """

    for plugin in plugins:
        instances = instances_by_plugin(context, plugin)
        if issubclass(plugin, InstancePlugin):
            for instance in instances:
                yield plugin, instance

        if issubclass(plugin, ContextPlugin):
            if instances:
                yield plugin, None

import pyblish.api
from pyblish import api, logic
import pyblish.util

class RunNever(pyblish.api.ContextPlugin):
    order = pyblish.api.ValidatorOrder
    families = ["NOT_EXIST"]

    def process(self, context):
        print "I shouldn't run"

class RunAlways(pyblish.api.ContextPlugin):
    order = pyblish.api.ValidatorOrder

    def process(self, context):
        print "I should always run"

class RunOtherFamily(pyblish.api.ContextPlugin):
    order = pyblish.api.ValidatorOrder
    families = ["otherFamily"]

    def process(self, context):
        print "I should on 'otherFamily'"

context = api.Context()
for name in ("A", "B"):
    instance = context.create_instance(name)
    instance.set_data("family", "myFamily")

for name in ("C", "D"):
    instance = context.create_instance(name)
    instance.set_data("family", "otherFamily")

for plugin, instance in logic.Iterator([RunNever, RunAlways, RunOtherFamily],
                                       context):
    print("Processing %s with %s" % (plugin, instance))

I can't get it to publish correctly though, but guessing this is a step towards the solution.

[tokejepsen]

Or should I send a PR to your repo?

[mottosso]

Or should I send a PR to your repo?

No I think this is well enough presented.

You can't use issubclass though, because it won't work with old-style plug-ins. The __instanceEnabled__ attribute works for both old and new. When I said it was deprecated, I meant we shouldn't build new functionality with it, but it's already used (and required) here.

Having a closer look in a bit.

[tokejepsen]

You can't use issubclass though, because it won't work with old-style plug-ins. The instanceEnabled attribute works for both old and new. When I said it was deprecated, I meant we shouldn't build new functionality with it, but it's already used (and required) here.

I guess you couldn't convert the plugin according to the arguments input?

[mottosso]

I guess you couldn't convert the plugin according to the arguments input?

I've been considering doing that, similar to how the oldest-style plug-ins are being converted. That way we would only ever have to work with a single plug-in.

I think it might be difficult though, because the old plug-ins had support for running both as an InstancePlugin and a ContextPlugin when the arguments were context, instance. I'm not sure there is a 1-1 conversion here.

If you find a way, that would be really great.

[tokejepsen]

I think it might be difficult though, because the old plug-ins had support for running both as an InstancePlugin and a ContextPlugin when the arguments were context, instance. I'm not sure there is a 1-1 conversion here.

What was expected to happen with both context and instance as arguments?

[mottosso]

What was expected to happen with both context and instance as arguments?

That's the question, isn't it? :)

[mottosso]

The instance attribute is working as a boolean switch; if it is present, it will run once per instance, and not once with context. So regardless of any other argument, it is an InstancePlugin if it has the argument instance, and a ContextPlugin otherwise.

[tokejepsen]

The instance attribute is working as a boolean switch; if it is present, it will run once per instance, and not once with context. So regardless of any other argument, it is an InstancePlugin is it has the argument instance, and a ContextPlugin otherwise.

Cool. Would it not be as simple as?

if "instance" in args:
    InstancePlugin()
else:
    ContextPlugin()

[mottosso]

Cool. Would it not be as simple as?

It could be, yes!

[mottosso]

Continuing from #267 here.

[mottosso]

With the change to make ids for plug-ins, we've now opened up the doors of having multiple plug-ins with the same name.

Before, the name was used to choose which plug-in, in the event of two same name plug-ins, to choose from. It's been a convenience feature at best, and can be better handled now that we can do so explicitly.

But the question is, do we want to?

• Failing test

======================================================================

FAIL: Discovering plugins results in a single occurence of each plugin
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/virtualenv/python2.7.9/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/home/travis/build/pyblish/pyblish-base/tests/pre11/test_plugins.py", line 54, in test_no_duplicate_plugins
    assert_equals(len(plugins), 1)
AssertionError: 2 != 1

[coveralls]

Coverage increased (+0.03%) to 95.821% when pulling c5d5b82f6f973c88e2a10543c925ff5a17e13ea5 on mottosso:master into 62d767f4386d33e7cb18f544086ac128af02c1bc on pyblish:master.

[coveralls]

Coverage increased (+0.03%) to 95.821% when pulling d91973831dca26e124ebe5a7a1722903d58f537d on mottosso:master into 62d767f4386d33e7cb18f544086ac128af02c1bc on pyblish:master.

[tokejepsen]

Am I understanding the test correct, that it tests for what is essentially a bug, that one plugin is hidden underneath another?

[mottosso]

How do you mean?

[tokejepsen]

Sorry, I wasn't reading the code correctly. I'm guessing you are worried about the case where a user has duplicated plugins with the same name and the same code?

[mottosso]

Sorry, I wasn't reading the code correctly. I'm guessing you are worried about the case where a user has duplicated plugins with the same name and the same code?

Sorry, I'm still not getting it, could you point to which part of the code you are referring?

[tokejepsen]

I'm looking at the link you posted; https://github.com/pyblish/pyblish-base/blob/62d767f4386d33e7cb18f544086ac128af02c1bc/tests/pre11/test_plugins.py#L54

But the question is, do we want to?

Its quite possible that I don't get what you are asking.

[mottosso]

Its quite possible that I don't get what you are asking.

Ah, yes. The question is, would we ever want to have two plug-ins of the same name, even though they are different?

Such as a CollectInstances from pyblish-magenta along with a CollectInstances from pyblish-track.

Does it add more value than confusion?

[tokejepsen]

Does it add more value than confusion?

I think it would add more value. This is a bit long term, but say you have two packages developed independently but they accidentally use the same name for a plugin.

[mottosso]

This is a bit long term, but say you have two packages developed independently but they accidentally use the same name for a plugin.

Yes, that is already happening. At the moment, each package needs to have a fully unique name to interop with other packages.

But what about in the GUI? They would appear to be one and the same. :S

[tokejepsen]

But what about in the GUI? They would appear to be one and the same. :S

Would we wanna namespace at that point?

[tokejepsen]

Yes, that is already happening. At the moment, each package needs to have a fully unique name to interop with other packages.

I definitely don't see a point in having the ability to have the same name within the same package.

[coveralls]

Coverage decreased (-0.4%) to 95.427% when pulling 88478e95a8106ee580d98cba3334c4e090ba7eb6 on mottosso:master into 62d767f4386d33e7cb18f544086ac128af02c1bc on pyblish:master.

[BigRoy]

With the change to make ids for plug-ins, we've now opened up the doors of having multiple plug-ins with the same name.

Before, the name was used to choose which plug-in, in the event of two same name plug-ins, to choose from. It's been a convenience feature at best, and can be better handled now that we can do so explicitly.

But the question is, do we want to?

To be honest I think it'll add somewhat to confusion than to value. What would happen with a class explicitly registered instead of read from a path? How would you know if it is supposed to overwrite an existing registered class or not if it's allowed to keep both with the same name.

The idea of namespaces I think could work well, but I think that should maybe also be explicit? Maybe the clashes could be based namespace + id. Say:

class ValidateMesh(pyblish.api.InstancePlugin):
    namespace = "magenta"

class ValidateMesh(pyblish.api.InstancePlugin):
    namespace = "bumpy"

Yet, to be honest, even that feels somewhat confusing?

[BigRoy]

And here's an example of what #250 might look like in the Iterator.

From here.

This makes sense to me. I think this would work and is how I expect it to behave.

Or maybe like this:

def Iterator(plugins, context):
    """Primary iterator

    This is the brains of publishing. It handles logic related
    to which plug-in to process with which Instance or Context,
    in addition to stopping when necessary.

    Arguments:
        plugins (list): Plug-ins to consider
        context (list): Instances to consider

    """

    for plugin in plugins:
        instances = instances_by_plugin(context, plugin)

        if plugin.__instanceEnabled__:
            for instance in instances:
                yield plugin, instance

        else:

            requires_family = "*" not in plugin.families
            if requires_family:

                # families of all available instances together
                families = set()
                for instance in context:
                    families.add(instance.data["family"])
                    families.update(instance.data.get("families", []))

                has_support = any(plugins_by_families([plugin], families))
                if not has_support:
                    continue

            yield plugin, None

This should also be faster since it would only collect all families from all instances when required. Thinking about it some more, it could even be like this I think:

def Iterator(plugins, context):
    """Primary iterator

    This is the brains of publishing. It handles logic related
    to which plug-in to process with which Instance or Context,
    in addition to stopping when necessary.

    Arguments:
        plugins (list): Plug-ins to consider
        context (list): Instances to consider

    """

    for plugin in plugins:
        instances = instances_by_plugin(context, plugin)

        requires_family = "*" not in plugin.families
        if requires_family:

            # families of all available instances together
            families = set()
            for instance in context:
                families.add(instance.data["family"])
                families.update(instance.data.get("families", []))

            has_support = any(plugins_by_families([plugin], families))
            if not has_support:
                continue

        if plugin.__instanceEnabled__:
            for instance in instances:
                yield plugin, instance

        else:
            yield plugin, None

[mottosso]

How would you know if it is supposed to overwrite an existing registered class or not if it's allowed to keep both with the same name.

This is actually something I think should be more explicit anyway; at the moment, a plug-in with the same name as one that exists does overwrite, but it isn't obvious. In fact it doesn't even tell you about it, but more importantly I don't think it's very intuitive.

Having it register it regardless of what's already there is more explicit and less prone to surprises.

E.g.

# _registered_plugins[plugin.__name__] = plugin # From
_registered_plugins[plugin.id] = plugin # To

Maybe the clashes could be based namespace + id. Say:

I don't think it would matter, the IDs of those plug-ins would be unique anyway. And yeah, like you said, there is no natural namespace with plug-ins; apart from the module name, but not all plug-ins have modules.

[mottosso]

This should also be faster

Let's not worry about performance yet. Slow and stable is what we want.

[mottosso]

But the question is, do we want to?

We can leave this question for now; the changes maintain the current behaviour.