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

Add 'context' key to 'result' dict #401

Merged

Conversation

MustafaJafar
Copy link
Contributor

Changelog Description

This PR adds context key to result dict inside plugin.process

Additional Info 1

This PR is very helpful for people who develop some debugging tools like https://gist.github.com/BigRoy/1972822065e38f8fae7521078e44eca2

This PR avoids the need to create custom redundant events.

Additional Info 2

Things I tried:

  • Accessing context via result["instance"].context but this only works for instance plugins.
  • Passing the context as argument to lib.emit -> lib.emit("pluginProcessed", result=result, context=context) but apparently this affects every single call so we will get an error TypeError: on_plugin_processed() got an unexpected keyword argument 'context'
  • The backward friendly solution was just to update the result key.

@BigRoy
Copy link
Member

BigRoy commented May 29, 2024

Yes - this would be great.

This would make context easily accessible from the pluginProcessed event callback which is the reason you're making this change but wasn't extremely apparent from the PR description.

@mottosso
Copy link
Member

mottosso commented Jun 2, 2024

Sorry for the delay I must have forgot to hit "Comment".

This should work fine, my only concern is with cyclic dependencies. I recall there was a reason for not including this, but can't recall exactly what. It did involve recursion and memory bloating. If you can confirm that no references to itself exist in the result that would help alleviate this issue. For example, the instance contains reference to the context, which contains reference to each instance.. I can't be sure. Can we list out the dependencies here so we know?

Copy link
Contributor Author

@MustafaJafar MustafaJafar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

memory bloating

Ouch!


Can we list out the dependencies here so we know?

I'm not sure how to do that but I did some test. I hope it's relevant.
As far as I understood we have two cases:

  • If the processed plugin is an instance plugin. in which result["instance"].context is the same as result["context"] which is duplicate data. So, it's redundant to pass the context in this case ?
  • If the processed plugin is a context plugin. in which there are no instances passed therefore result["instance"].context doesn't exist.

I tested the suggestions below using BigRoy's pyblish stepper.
Here's a portion of the log:

<<<<<<<<<< CollectWorkfile >>>>>>>>>>
result["instance"]: workfileCfx
result["instance"].context: [pyblish.plugin.Instance("reviewMain"), pyblish.plugin.Instance("workfileCfx")]
result.get("context"): None
<<<<<<<<<< CollectCurrentDate >>>>>>>>>>
result["instance"]: None
Skipping result["instance"].context
result.get("context"): [pyblish.plugin.Instance("reviewMain"), pyblish.plugin.Instance("workfileCfx")]

pyblish/plugin.py Show resolved Hide resolved
@mottosso
Copy link
Member

mottosso commented Jun 2, 2024

result["instance"].context

Mm, true, this is already a cycle, as the context then also includes the instance which includes the context etc. So it mustn't be an issue to add another then.

Then this looks fine to me, I'm happy to merge this once you are happy.

@MustafaJafar
Copy link
Contributor Author

MustafaJafar commented Jun 2, 2024

Then this looks fine to me, I'm happy to merge this once you are happy.

Great, thanks a ton! I'm really happy indeed.

Would you recommend the previous suggestions ? Or the PR as it's looks good ?
Oh, My question was already answered!

So it mustn't be an issue to add another then.

@mottosso mottosso merged commit 444cc8e into pyblish:master Jun 4, 2024
1 check passed
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 this pull request may close these issues.

3 participants