Simplify Iterator #264

mottosso · 2016-04-07T06:57:57Z

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 · 2016-04-07T06:58:36Z

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 · 2016-04-07T06:58:47Z

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

tokejepsen · 2016-04-08T06:54:27Z

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 · 2016-04-08T07:02:09Z

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 · 2016-04-08T07:18:38Z

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 · 2016-04-08T08:14:32Z

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 · 2016-04-08T08:19:12Z

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 · 2016-04-08T09:17:14Z

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 · 2016-04-08T09:22:29Z

Or should I send a PR to your repo?

mottosso · 2016-04-08T09:24:24Z

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 · 2016-04-08T09:26:09Z

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 · 2016-04-08T09:30:12Z

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 · 2016-04-08T09:35:52Z

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 · 2016-04-08T09:38:28Z

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

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

mottosso · 2016-04-08T09:39:48Z

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 · 2016-04-08T09:42:02Z

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 · 2016-04-08T09:43:43Z

Cool. Would it not be as simple as?

It could be, yes!

- Instances have a unique id generated with uuid.uuid4()

mottosso · 2016-04-26T17:03:09Z

Continuing from #267 here.

mottosso · 2016-04-26T17:07:33Z

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

…gins of same name.

coveralls · 2016-04-27T05:23:42Z

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

coveralls · 2016-04-27T06:07:40Z

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

tokejepsen · 2016-04-27T07:53:09Z

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

mottosso · 2016-04-27T08:06:45Z

How do you mean?

tokejepsen · 2016-04-27T08:12:36Z

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 · 2016-04-27T08:50:35Z

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 · 2016-04-27T08:55:54Z

I'm looking at the link you posted;

pyblish-base/tests/pre11/test_plugins.py

Line 54 in 62d767f

assert_equals(len(plugins), 1)

But the question is, do we want to?

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

mottosso · 2016-04-27T08:57:58Z

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 · 2016-04-27T09:04:34Z

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 · 2016-04-27T09:05:52Z

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 · 2016-04-27T09:20:25Z

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

Would we wanna namespace at that point?

tokejepsen · 2016-04-27T09:22:29Z

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.

Now they both have a common superclass and act more similarly

coveralls · 2016-04-27T09:45:06Z

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

BigRoy · 2016-04-27T12:05:39Z

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 · 2016-04-27T12:10:47Z

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 · 2016-04-27T13:24:02Z

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 · 2016-04-27T13:24:58Z

This should also be faster

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

mottosso · 2016-04-27T15:06:35Z

But the question is, do we want to?

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

mottosso added 2 commits April 7, 2016 07:10

Simplify logic iterator

966b04b

Simplify iterator

aecae8f

mottosso added the enhancement label Apr 7, 2016

mottosso mentioned this pull request Apr 7, 2016

ContextPlugin running when instance of family not present #250

Open

BigRoy and others added 9 commits April 26, 2016 14:59

Implement pyblish#266

54d2a52

- Instances have a unique id generated with uuid.uuid4()

Fix pyblish#268

f2cecf2

Ensure instance uuid is string representation

af70d41

Add test for pyblish#268

3605ae6

Add count to test to ensure it actually runs through correctly

9cca61a

Move uid to superclass and write-protect members

fa477d7

Repair tests, one thing remains. Decision time!

3c12aca

Merge util.py

4643f75

Repair util.py

0e8bde2

mottosso mentioned this pull request Apr 26, 2016

Implement #266 #267

Closed

mottosso added 3 commits April 26, 2016 19:54

Make Plugin.id into a classproperty; otherwise it can't work

e666dba

Persist ids

b28f9f5

Maintain backwards compatibility, but keep door open for multiple plu…

c5d5b82

…gins of same name.

Add additional test for id persistence across discover()

d919738

Unify Context and Instance

88478e9

Now they both have a common superclass and act more similarly

mottosso merged commit 990bfbd into pyblish:master Apr 27, 2016

Simplify Iterator #264

Simplify Iterator #264

Conversation

mottosso commented Apr 7, 2016

mottosso commented Apr 7, 2016

coveralls commented Apr 7, 2016

tokejepsen commented Apr 8, 2016

mottosso commented Apr 8, 2016

tokejepsen commented Apr 8, 2016

mottosso commented Apr 8, 2016

mottosso commented Apr 8, 2016

tokejepsen commented Apr 8, 2016

tokejepsen commented Apr 8, 2016

mottosso commented Apr 8, 2016

tokejepsen commented Apr 8, 2016

mottosso commented Apr 8, 2016

tokejepsen commented Apr 8, 2016

mottosso commented Apr 8, 2016

mottosso commented Apr 8, 2016

tokejepsen commented Apr 8, 2016

mottosso commented Apr 8, 2016

mottosso commented Apr 26, 2016

mottosso commented Apr 26, 2016

coveralls commented Apr 27, 2016 • edited Loading

coveralls commented Apr 27, 2016 • edited Loading

tokejepsen commented Apr 27, 2016

mottosso commented Apr 27, 2016

tokejepsen commented Apr 27, 2016

mottosso commented Apr 27, 2016

tokejepsen commented Apr 27, 2016

mottosso commented Apr 27, 2016

tokejepsen commented Apr 27, 2016

mottosso commented Apr 27, 2016

tokejepsen commented Apr 27, 2016

tokejepsen commented Apr 27, 2016

coveralls commented Apr 27, 2016 • edited Loading

BigRoy commented Apr 27, 2016

BigRoy commented Apr 27, 2016 • edited Loading

mottosso commented Apr 27, 2016

mottosso commented Apr 27, 2016

mottosso commented Apr 27, 2016 • edited Loading

coveralls commented Apr 27, 2016 •

edited

Loading

coveralls commented Apr 27, 2016 •

edited

Loading

coveralls commented Apr 27, 2016 •

edited

Loading

BigRoy commented Apr 27, 2016 •

edited

Loading

mottosso commented Apr 27, 2016 •

edited

Loading