pyblish / pyblish-base

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

Emit collected, validated, extracted, integrated signals from publish #361

Closed jboisvertrodeofx closed 4 years ago

jboisvertrodeofx commented 4 years ago

Description When we call pyblish.util.publish(), the "collected", "validated", "extracted" and "integrated" signals are not emitted, despite processing plugins from these steps. In the end, only the "published" signal is emitted, and those signals are only emitted when specifically using their designated function (e.g. "collect", "validate", etc.)

I added an orders=None argument to the publish and publish_iter functions, so that we can specify which orders (e.g. CollectorOrder, ValidatorOrder, etc.) we want to process plugins for, and only the signals from these orders will be emitted. If no orders are given, all the plugins are processed and so all the signals are emitted at the appropriate moment.

To make this work, I had to tweak a lot of code in the pyblish.util module, sorry about that!

Expected results It would seem to make sense to emit those signals when executing pyblish.util.publish, so that callbacks could be executed after each step if needed.

It seems that since only recently the "validated" signal is being emitted when using pyblish-lite or pyblish-qml UIs, so emitting these signals in pyblish.util.publish would make things more consistent. I believe we should add the remaining signals to the UIs' logic as well, but that is a separate story.

Short reproducible

import pyblish.api
import pyblish.util

class Collector(pyblish.api.ContextPlugin):
    order = pyblish.api.CollectorOrder

    def process(self, context):
        pass

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

    def process(self, context):
        pass

class Extractor(pyblish.api.ContextPlugin):
    order = pyblish.api.ExtractorOrder

    def process(self, context):
        pass

class Integrator(pyblish.api.ContextPlugin):
    order = pyblish.api.IntegratorOrder

    def process(self, context):
        pass

def collected(context):
    print("collected: {0}".format(context))

def validated(context):
    print("validated: {0}".format(context))

def extracted(context):
    print("extracted: {0}".format(context))

def integrated(context):
    print("integrated: {0}".format(context))

def published(context):
    print("published: {0}".format(context))

pyblish.api.deregister_all_paths()
pyblish.api.deregister_all_plugins()
pyblish.api.deregister_all_callbacks()

pyblish.api.register_callback("collected", collected)
pyblish.api.register_callback("validated", validated)
pyblish.api.register_callback("extracted", extracted)
pyblish.api.register_callback("integrated", integrated)
pyblish.api.register_callback("published", published)

pyblish.api.register_plugin(Collector)
pyblish.api.register_plugin(Validator)
pyblish.api.register_plugin(Extractor)
pyblish.api.register_plugin(Integrator)

pyblish.util.publish()
pyblish.util.publish(orders=[pyblish.api.CollectorOrder, pyblish.api.ValidatorOrder])
mottosso commented 4 years ago

Thanks for this. Before proceeding with this PR, have you considered calling each step explicitly?

util.collect()
util.validate()
util.extract()
util.integrate()

That should trigger the signals you're looking for, would this work for you?

jboisvertrodeofx commented 4 years ago

Thanks for this. Before proceeding with this PR, have you considered calling each step explicitly?

util.collect()
util.validate()
util.extract()
util.integrate()

That should trigger the signals you're looking for, would this work for you?

Thanks for the quick reply @mottosso !

In my case, I need to execute multiple plugins at once while using the same state instance when logic.Iterator is executed, to make sure that the execution stops if there were errors while processing the plugins. Using util.collect(), util.validate(), util.extract() and util.integrate() would require adding checks in-between the steps to spot errors and stop the execution, whereas using util.publish() deals with this automatically while making use of logic.registered_test().

In my opinion, it would seem to make sense for the util.publish function to emit signals for what it is responsible of executing, while the util.collect, util.validate, util.extract and util.integrate are merely convenience functions used only to execute a specific part of what util.publish is executing.

Not sure if that makes sense, but let me know if I wasn't clear!

Thanks!

jboisvertrodeofx commented 4 years ago

I committed some tweaks to replace the new orders argument with ordermin and ordermax, so that we can specify a range of orders instead of a specific list.

Examples:

# Execute every plugin until validators (including validators)
pyblish.util.publish(context, ordermax=pyblish.api.ValidatorOrder)
# Events: "collected", "validated", "published"

# Only execute extractors
pyblish.util.publish(context, ordermin=pyblish.api.ExtractorOrder, ordermax=pyblish.api.ExtractorOrder)
# Events: "extracted", "published"

# Execute every plugin from extractors (including extractors)
pyblish.util.publish(context, ordermin=pyblish.api.ExtractorOrder)
# Events: "extracted", "integrated", "published"

# Execute only collectors
pyblish.util.collect(context)
# Events: "collected"

# Execute only validators
pyblish.util.validate(context)
# Events: "validated"
jboisvertrodeofx commented 4 years ago

I also slightly changed the behavior of the collect and integrate functions to include plugins whose order are not in the usual CVEI ranges. The collect function now includes plugins that have an order of 0.5 and lower (instead of being between -0.5 and 0.5), and the integrate function now includes plugins that have an order of 2.5 and above.

As far as I'm aware, this should not have any significant impact, as there were no convenience functions to process pre-collectors and post-integrators individually anyway, apart from using publish. I would assume that pre-collectors and post-integrators would not be officially supported anyway, but perhaps my assumption is wrong.

mottosso commented 4 years ago

Sorry for the delay on this one. Because the amount of changes has grown, I can't merge this until I or someone (ping @davidlatwe) has given it a proper lookthrough, and I won't be able to for at least another month or two.

I'm not keen on the cosmetic changes to function signatures..

def my_function(
  arg_a=1, arg_b=2, arg_c=3
):

It isn't consistent with anything else throughout this codebase. To keep things within 80 chars, it should be..

def my_function(arg_a=1,
                arg_b=2,
                arg_c=2):

Except I'm also not a fan of ordermin and ordermax. They are necessary and used elsewhere, but these names and style of argument signatures aren't found anywhere else either. Also, why a new _inrange when we've already got lib.inrange?

If you can manage to achieve the goals of the PR (i.e. to emit signals) with much less changes to the code, it would make it easier and quicker to merge. Otherwise this might take a while.

davidlatwe commented 4 years ago

I see three main changes in this PR, and here's what I think.

Picking range in util.publish

I am not a fan of adding orders, ordermin, ordermax either. Not because of the naming though, but about the complexity that they have introduced into util.publish and the new behavior also conflict the meaning of "publish".

Perhaps instead of changing util.publish, how about adding one fucntion for looking up errors in context between util.collect(), util.validate(), util.extract(), util.integrate() and stop the process ?

For example:

from pyblish import util

context = util.collect()
util.fuse(context)  # This will check context and raise error if any
context = util.validate(context)
util.fuse(context)

Emitting signals of different orders in util.publish

I like this, but I think the implementation could be simpler, like instead of using multiple if-statements to seperate plugin orders for emitting corresponding signal, maybe we could try the mechanism that has implemented in pyblish-qml's main iter here.

Embracing plugins that are ordered out side of CVEI

I like this, too. But need more time to test.


Please let me know if I missed anything. ☺️

jboisvertrodeofx commented 4 years ago

Hey Marcus and David, thanks for the feedback!

I'm not keen on the cosmetic changes to function signatures..

No problem, I can change this to your proposed indentation!

Also, why a new _inrange when we've already got lib.inrange?

My original thought was to avoid changing the existing signature of lib.inrange to support ranges that are out of the usual CVEI ranges, since the util.publish function and even the UIs currently support those. I'll have to see if we still need the new util._inrange function after figuring out the required changes from David's propositions, but perhaps we won't need it anymore.

I am not a fan of adding orders, ordermin, ordermax either. Not because of the naming though, but about the complexity that they have introduced into util.publish and the new behavior also conflict the meaning of "publish".

Perhaps instead of changing util.publish, how about adding one fucntion for looking up errors in context between util.collect(), util.validate(), util.extract(), util.integrate() and stop the process ?

Right, after some more thinking, I would also remove those new arguments from the util.publish function.

In my own case at work, I was using util.collect to execute the collectors, expecting the "collected" signal, and then using util.publish to execute all the remaining plugins, expecting the "validated", "extracted", "integrated" and "published" signals to be emitted. This is why I wanted to be able to execute the util.publish function with specific orders, as the "collected" signal would have otherwise been emitted twice, but in the end, I did something similar to what you proposed and just emitted the "published" signal myself.

I like this, but I think the implementation could be simpler, like instead of using multiple if-statements to seperate plugin orders for emitting corresponding signal, maybe we could try the mechanism that has implemented in pyblish-qml's main iter here.

Thanks, I'll look into this!

Sorry in advance if it takes me some time before I get to do those proposed changes, because we unfortunately don't need these changes anymore at work, and so I will have to do them in my free time!

jboisvertrodeofx commented 4 years ago

Small update, I didn't have time to work on this yet, but I moved my changes to a branch on our repo so that we can keep working on other changes on the main repo meanwhile if needed.

I will have to create a new PR from the new branch when I do the tweaks, since it's not possible to change the source branch of a PR.