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

Explicit cleanup for instances #1090

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

sjt-rvx
Copy link
Contributor

@sjt-rvx sjt-rvx commented Jan 16, 2025

When we render with Gaffer, to get an exr with the aov's as channels instead of their own separate exr files, we render the aovs separately (since that's what Gaffer does) and then combine them, this requires deletion of these temporary aov folders. The existing explicit_cleanup.py plugin operates on contexts only and that's not good enough since then we'd collect e.g. 4 aov folder paths to delete for the various render layers, and then the first publish job would just delete them all regardless of if they are finished enough. So I needed to have some explicit cleanup for instances.

So I added this to the explicit_cleanup.py plugin. Maybe it would be better to have it's dedicated plugin. Or perhaps the clean-up-farm plugin which I am unsure about how is used.

This then propagates the 'cleanupFullPaths' key on the instances on to the farm metadata json files.

@ynbot ynbot added the size/XS label Jan 16, 2025
@BigRoy BigRoy requested review from kalisp and iLLiCiTiT January 16, 2025 14:57
@@ -28,6 +28,13 @@ def process(self, context):
self._remove_full_paths(cleanup_full_paths)
self._remove_empty_dirs(cleanup_empty_dirs)

for instance in context:
instance_cleanup_full_paths = instance.data.get("cleanupFullPaths")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't this directly clean it up - even BEFORE it hits the farm - in the local publish session? (likely less of a problem because it didn't create data yet) but I wonder if that's a potential unintentional side effect of this?

Because now this is in the instance.data now and also in the instance data when the publish job runs on the farm. My gut feeling says that you're trying to say: "when the farm job finished, then remove these files please" - but now it also deletes them now (if they happened to already be there)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are absolutely right. That is indeed the side-effect and it could bite you hard .

And you are correct in my desired effect. I re-read cleanup_farm.py and it does exactly what I need. I thought I had read through it the other day and it did something wildly different then 😁 .

So this is wholly unnecessary. I'll probably make another merge request to add 'gaffer' to the allowed hosts in that plugin. This PR can be deleted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants