Skip to content

Conversation

@sjt-rvx
Copy link
Contributor

@sjt-rvx sjt-rvx commented Jul 1, 2025

Changelog Description

To allow gaffer to publish to multiple contexts in one go, I wanted to be able to specify explicit cleanup directories per-instance, otherwise all the context's paths-to-cleanup would be removed once the first of the publishes would go through. Plus I needed to clean up more folders than just the stagingDir so cleanup_farm.py does not do it for me (plus, it's a context plugin).

Part of RVX's summer of pull requests

@ynbot ynbot added the size/XS label Jul 1, 2025
for _dir in explicit_cleanup_dirs:
self.log.debug("Removing explicit cleanup dir {}".format(_dir))
try:
shutil.rmtree(_dir)
Copy link
Collaborator

@BigRoy BigRoy Jul 1, 2025

Choose a reason for hiding this comment

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

Although I can see the use case - I'm a bit worried that some day, accidentally a wrong path ends up in explicit_cleanup_dirs :') and it ends up wiping a full project or asset folder. :) Or was this the one that by default errors on non-empty folders?

Additionally, I feel like it should not be part of this plug-in. As far as I can see this plug-in is focused on handling stagingDir - and explicitly does not apply to other files. This logic is better kept in either a separate plug-in, or actually part of the explicit cleanup (even though that is ContextPlugin it can iterate over the instances just fine.)

@BigRoy BigRoy added community Issues and PRs coming from the community members type: enhancement Improvement of existing functionality or minor addition labels Jul 1, 2025
@iLLiCiTiT
Copy link
Member

iLLiCiTiT commented Jul 1, 2025

Looks like duplicty of ExplicitCleanUp . Just use "cleanupFullPaths" instead of "explicit_cleanup_dirs".

@sjt-rvx
Copy link
Contributor Author

sjt-rvx commented Jul 2, 2025

Looks like duplicty of ExplicitCleanUp . Just use "cleanupFullPaths" instead of "explicit_cleanup_dirs".

ExplicitCleanup is a context plugin, what I needed was per-instance cleanup. But I agree with Roy about maybe having this inside ExplicitCleanup - and just have it also iterate over the instances being published to fish out cleanupFullPaths from there.

The issue with just having it a context plugin is the case where publishing happens to multiple shots on the farm. So if I collect the paths in the publish context - all the cleanup paths for the different shots get added to the cleanupFullPaths then render 1 finishes and cleans all paths, even for renders 2, 3 and 4. This is a result of the farm-generation-jobs being in one publish context and the actual farm-image-publish being another, so transferring the publish context data to the per-render-context data one-to-one is the thing that's getting in my way.

  • of course I can also just have my own cleanup plugin in our in-house addon - with blackjack and *****rs

@iLLiCiTiT
Copy link
Member

iLLiCiTiT commented Jul 3, 2025

I really don't understand why you can't use "cleanupFullPaths"? It is context plugin, but that has nothing to do with what it does. As far as I understand the issue is when you collect the paths? Based on what you wrote, it looks like you're filling the paths before you send the job to the farm? If that's the case then that's wrong on multiple levels, and you should have plugin adding the paths that is running during the farm publishing (so it is instance specific)... And it is OS agnostic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community Issues and PRs coming from the community members size/XS type: enhancement Improvement of existing functionality or minor addition

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants