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

Using positive and negative offsets leading to some Plug-Ins being processed multiple times #127

Open
Etherkirby opened this issue Sep 7, 2022 · 7 comments

Comments

@Etherkirby
Copy link

When using a mix of Plug-Ins with positive and negative offsets, some Plug-Ins get executed twice, depending on where you continue the publishing process.

Example structure:

  • Collector1 (order=CollectorOrder)
  • Collector2 (order=CollectorOrder + 0.4)
  • Validator1 (order=ValidatorOrder - 0.4)
  • Validator2 (order=ValidatorOrder)
  • Validator3 (order=ValidatorOrder + 0.4)
  • Extractor1 (order=ExtractorOrder - 0.4)
  • Extractor2 (order=ExtractorOrder)
  • Integrator

With these example Plug-Ins, we can run into either Collector2 or Validator3 being run twice. Here's how:

  • Open Pyblish Lite UI
  • Collection will run
  • Pressing the "Play" Button now will lead to Collector2 being processed an additional time.
  • Instead, if we press the "Validate" Button first, all Validators run as expected without any Collectors being run twice.
  • If we now press the "Play" Button, Validator3 will be processed an additional time.

This problem seems to happen here, because we take the .order of the next Plug-In to run and always subtract 0.5 from it to find out where we should start. This in turn causes an issue if the next order is actually using a negative offset and will go back more than it needs to:

  • Start of Collection
  • Collector1 is processed
  • Collector2 is processed (0 + 0.4 = 0.4)
  • Press "Publish" button
  • Next Plug-In is Validator1 (1 - 0.4 = 0.6)
  • We subtract 0.5 from this to figure out where to continue (0.6 - 0.5 = 0.1)
  • Collector2 is processed again
  • Validator1 is processed
  • Validator2 is processed
  • ...
@BigRoy
Copy link
Member

BigRoy commented Sep 7, 2022

Nice breakdown of the issue!

I feel like it should instead start from round(plugin.order)?

@Etherkirby
Copy link
Author

Thanks for the fast reply!

That's a good suggestion. I'm currently working on a proposed fix where it doesn't subtract the 0.5 if it's just resuming, but just rounding up might be easier.

Will have my PR ready in a bit.

@mottosso
Copy link
Member

Been following this but haven't had myself a moment to reflect on it until now. Very well breakdown of the problem indeed, thanks for that.

Just to confirm, this is an issue only when using offsets up to 0.4?

Would this cause the same issue?

  • Collector1 (order=CollectorOrder)
  • Collector2 (order=CollectorOrder + 0.1)
  • Validator1 (order=ValidatorOrder - 0.1)
  • Validator2 (order=ValidatorOrder)
  • Validator3 (order=ValidatorOrder + 0.1)
  • Extractor1 (order=ExtractorOrder - 0.1)
  • Extractor2 (order=ExtractorOrder)
  • Integrator

One of the things I see some do, is establish their own orders to avoid plussing and minusing by hand.

# Somewhere in your library, e.g. my_orders.py
PostValidationOrder = api.ValidatorOrder - 0.1
PreValidationOrder = api.ValidatorOrder - 0.2

# And then use them in favour of the build-in orders.
import my_orders
Extractor1.order = my_orders.PreValidationOrder
Extractor2.order = my_orders.PostValidationOrder

Aside from this, I'm very cautions to change the publishing mechanism. There are a lot of edgecases here that have turned into "the way things work". People build their pipelines around those assumptions, and so changing them - even if correct - may not actually be desirable. There are many things that can be changed, but the ordering mechanic is especially sensitive and would require much more care.

If it turns out it does require this case - for example if the above "workaround" still doesn't actually solve the problem - then we'd have to cross that bridge and it'd take a bit more of a testsuite to justify breaking some pipelines.

@Etherkirby
Copy link
Author

Just to confirm, this is an issue only when using offsets up to 0.4?

This behavior appears when the difference between a Collector/Validator/Extractor <= 0.5. In your example:

  • CollectorOrder + 0.1 = 0.1
  • ValidatorOrder - 0.1 = 0.9
  • The difference between these two is > 0.5, so this works fine

The reason why we noticed this behavior was because we have certain Collectors and Validators that we want to run at the end of Collection / Validation, so we use a very high offset of 0.4 for them. This means they run twice as soon as there is a negative offset of -0.1 or more in the next step.

I totally understand that you want to be extremely cautious when it comes to these sorts of changes. There is definitely a workaround for it (adjusting the order so there is never a difference <= 0.5), so I'll leave that decision up to you.

@mottosso
Copy link
Member

so we use a very high offset of 0.4 for them

Bear in mind that 0.4 being very high is completely arbitrary. These are floating point values, and the range you pick is up to you. For example, you can consider 0.04 the extreme upper end instead of 0.4, or even 0.0004 and work within the +- of that.

With this in mind, let's keep this issue present as documentation for what happens at the Edge of Orders (sounds like the title of a good book!) but leave the implementation as-is.

@aharjunmaa
Copy link
Contributor

Hello! I just ran into this same problem, because our previous developers saw it fit to use offsets of -0.499 ... 0.499 in the plugin ordering. I overrode the Controller class with a fix, which at first was start_from=pyblish.api.ValidatorOrder (because the collectors should already have run anyway), but I then came up with start_from=round(plugin.order) in order to mimic the old functionality as much as possible. Rounding errors should not be a problem since we don't use offsets of exactly 0.5.

I came here to report this, but I see that this is a known issue since well over a year. However, the Pyblish Lite documentation still says that it is safe to use offsets of up to 0.5, which clearly is not the case:

https://learn.pyblish.com/14-cvei-iv

If this is not going to be fixed in the code, would it not make sense to update the documentation accordingly?

You can offset any plug-in from negative 0.499.. 0.2499.. to positive 0.499.. 0.2499..

@mottosso
Copy link
Member

Thanks for reporting this @aharjunmaa. We would love someone to maintain the API docs for Pyblish, they are a little finnicky to update at the moment mostly because it's been too long since I've done it myself. But they are all written in Markdown and may be uploaded anywhere for hosting if you or anyone would like to take on the challenge.

Steps

  1. Update the markdown file for that page here: https://github.com/pyblish/pyblish-by-example/blob/1.8/chapters/14-cvei-iv.md
  2. Make a new account on GitBooks (if keeping this, I think it's paid nowadays, used to be free) or anywhere like on GitHub Pages for example
  3. Make a PR here, and I'll reroute the URL to this new account
  4. Profit

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