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

Support integrating a folder as files for a representation #1113

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

Conversation

BigRoy
Copy link
Collaborator

@BigRoy BigRoy commented Jan 30, 2025

Changelog Description

Support integrating a folder (directory) as files for a representation

Additional info

By allowing this, we can for example integrate BorisFX Silhouette workfiles (for which its project's are folders instead of files)

Fix ynput/ayon-silhouette#7

With these changes it is possible to integrate the workfile, register the representation, etc.

However, there may be follow up issues:

  • Integration works
  • Copy & Open from Published in Workfiles tool
  • Loader UI and tools works
  • Hero Publishing integration
    • Hero publishing on subsequent publishing (over the original one)
  • Site Sync (may need changes for this?)
  • Tray > Browser > Push to library project (may need changes for this?)
  • Tray > Browser > Delivery tool

The publisher folder will have an extension

Note: In Silhouette we're 'rather lucky' because the workfile project folders happen to have an extension .sfx and hence the 'integrated' folder will also have the extension and it matches 🥳 - however, I'm quite sure that the logic here still requires the integrated folder to also get an extension (due to the templating system). Which is technically fine, but something to be aware of. You likely couldn't turn it into a published folder named myfile/ because it'll definitely try and put some extension to it (which is what the love of representations has been about all this time)

Testing notes:

  1. Publish and integrate folders as representations, e.g. a Workfile from Silhouette integration

@BigRoy BigRoy added the type: enhancement Improvement of existing functionality or minor addition label Jan 30, 2025
@BigRoy BigRoy self-assigned this Jan 30, 2025
@ynbot ynbot added the size/XS label Jan 30, 2025
@BigRoy BigRoy added the sponsored This is directly sponsored by a client or community member label Jan 30, 2025
Copy link

@philippe-ynput philippe-ynput left a comment

Choose a reason for hiding this comment

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

Looks good to me, but I thought the plan was to zip the directory. Is it happening somewhere else ?

@BigRoy
Copy link
Collaborator Author

BigRoy commented Jan 30, 2025

Looks good to me, but I thought the plan was to zip the directory. Is it happening somewhere else ?

The discussion was whether that'd be good or not. See the issue description here: ynput/ayon-silhouette#7 (comment)

Zipping it could work for publishing, etc. but we also allow e.g. "rendering from the published file" which default deadline render plugins for e.g. Silhouette wouldn't support if for example the file was a .zip - unless we'd be able to put some clever logic somewhere so that it happens even before the farm job starts (without conflicts between machines, etc.)

That issue also exists with Harmony - we need a custom deadline rendering plug-in for Toonboon Harmony just to support rendering from .zip (which just unpacks it at start)

Ideas are welcome - exactly why I've tried to raise this topic a few times.

@iLLiCiTiT
Copy link
Member

NOTE: Representation paths must contain paths to files, not folders, otherwise you'll break site-sync.

@BigRoy
Copy link
Collaborator Author

BigRoy commented Jan 31, 2025

NOTE: Representation paths must contain paths to files, not folders, otherwise you'll break site-sync.

Yup - as mentioned in the PR description I didn't test Site Sync myself but I did assume that would be the case (and I had checked with @kalisp about that too).

Anyway, this is at a point where we should make the decision what way outweighs the pros and cons:

  1. Allow directory to be a representation.
  2. Zip the workfile on publish.

My biggest issue with point 2) is deadline rendering from a published workfile, in essence it'd mean that we'd need to customize the deadline render plug-in for the host application so that it can render from a .zip (OR we somehow point it also to an unpacked state on submission - but where would that then live?). The issue of needing to maintain a custom deadline addon due to this also exists in Toonboom Harmony. If the published workfiles there were also just folders we may not need that custom addon. If instead a prelaunch hook on the farm would unzip it for the render then you'd face the issue that most likely each worker will try to do that, at the same time, etc.

Tagging @antirotor @mkolar @philippe-ynput to make the decision on whether this PR is "ok" (with a potential site-sync follow up fix to support folders too) or a hard pass (deciding to never allow a directory to be a representation's "files").

@antirotor
Copy link
Member

What about using Deadline events for unzipping it just before the render. Like onJobSubmit or similar. This would allow us to use vanilla Deadline plug-in.

I am not saying it is perfect solution but it is probably the easiest one as you don't need to rewrite integrator, site sync or Deadline plug-in.

Conceptually I don't mind having folder as a representation even though I would be very very careful about how and where it is used as you'll loose control (but that aspect is the same for zip).

Performance wise it really depends on the scenario, server, number of files in the folder and their size. So there is no clear winner.

In the representation traits I wasn't counting with them explicitly, but there is the Location trait and the Bundle trait that can be used to describe a folder.

So to wrap this rant, I would go with zips now and then open the wider discussion about it.

@pleprince
Copy link

A few thoughts:

  • zip appeals to me because dealing with a single file may be simpler than a directory.
  • a single zip file will transfer faster than a directory of stuff over a network.
  • if we go for zip, we should at least have a checksum to verify the file's integrity.
  • how do we deal with a directory's contents ? Do we need a manifest ?

@antirotor
Copy link
Member

With zips there is one caveat though and that's that they will not retain user permissions and ownership (unless you tar before). I don't think it poses some issues as we try to stay away from file permissions but for pipeline tool in various studio deployments it needs to be considered.

@mkolar
Copy link
Member

mkolar commented Feb 3, 2025

I agree that zip would be better overall and would allow to keep most other areas of the pipeline intact. The introducing extra logic to unpack and pack during open and publish doesn't seem that bad to be honest.

@mkolar
Copy link
Member

mkolar commented Feb 3, 2025

But actually. It might very well be the best to leave folders as they are (with extension though) in the work area and workfiles app and only zip for publishing. That might be the best of both worlds.

@iLLiCiTiT
Copy link
Member

iLLiCiTiT commented Feb 3, 2025

  1. Allow directory to be a representation.

End goal is that we must have file paths in representation "paths". How that is achieved is the question.

In theory integrator could just copy paste the content from source folder to destination folder, and store the file paths inside the destination folder to representation entity (similar to how resources are handled now). Question is how would load plugin know how to process such representation... Also there is no extension on the representation entity, which might be issue too.

@BigRoy
Copy link
Collaborator Author

BigRoy commented Feb 3, 2025

  1. Allow directory to be a representation.

End goal is that we must have file paths in representation "paths". How that is achieved is the question.

In theory integrator could just copy paste the content from source folder to destination folder, and store the file paths inside the destination folder to representation entity (similar to how resources are handled now). Question is how would load plugin know how to process such representation... Also there is no extension on the representation entity, which might be issue too.

Just wanted to remark that the implementation here worked fine for my use case - and if the site sync would allow transfering "folders" and hash them accordingly (without the need to e.g. track the individual files inside it in the database) then it technically still works ok?

The behavior would then be similar to how we're not tracking what the contents are of a .zip file.

@BigRoy
Copy link
Collaborator Author

BigRoy commented Feb 3, 2025

But actually. It might very well be the best to leave folders as they are (with extension though) in the work area and workfiles app and only zip for publishing. That might be the best of both worlds.

The only downside here is Farm rendering - usually a farm render setup, like e.g. deadline just renders a filepath. We can't pass it the .zip path because the farm plug-in nor silhouette knows how to open the .zip so we must either:

  • Customize the farm plug-in with special behavior for the .zip which means us needing to maintain our own version of the plug-in and ship it. (We're already doing that with e.g. toonboom harmony, only because we're dealing with .zip files)
  • Or, somehow in a pre-hook somewhere unzip it before the farm job kicks in.

Either way, if we do unzip on the farm - where do unzip to? Because one might get into the situation where e.g. many machines may try to unzip at the same time. So I guess the unzipped project then goes into %TEMP% or alike? But then we'd also need to deal differently with render paths from the file that may be 'relative to the workfile' for which we have some workarounds already if it's from the published filepath - however it wouldn't be there anymore if we were to unzip.

@iLLiCiTiT
Copy link
Member

iLLiCiTiT commented Feb 4, 2025

Just wanted to remark that the implementation here worked fine for my use case - and if the site sync would allow transfering "folders" and hash them accordingly (without the need to e.g. track the individual files inside it in the database) then it technically still works ok?

It can't support folders transfer. Directory, on any disk-like integration (disk, gdrive, any cloud), is just metadata of file/s, and does not have any information for transfer if you don't have access to the folder (which is the point of site sync), we need to know about files, their sizes, their hashes etc. So representation paths MUST contain only paths to files, NOT folders.

Like I've said, integrator should add all subfiles of given folder to representation paths, if it finds out it is a folder.

@BigRoy
Copy link
Collaborator Author

BigRoy commented Feb 4, 2025

Like I've said, integrator should add all subfiles of given folder to representation paths, if it finds out it is a folder.

If doing that - would it still be fine if the result from get_representation_path() would result in the directory instead of a file?

@iLLiCiTiT
Copy link
Member

iLLiCiTiT commented Feb 4, 2025

If doing that - would it still be fine if the result from get_representation_path() would result in the directory instead of a file?

The loader that would be loading the representation should know that he can't use get_representation_path, but some other way how to load it... But technically can use get_representation_path because it does use attrib.template to fill the path, so it is ok.

Paths on representation is not to mimic what template and context do, but to collect files that are really related to the representaion.

@BigRoy
Copy link
Collaborator Author

BigRoy commented Feb 4, 2025

The loader that would be loading the representation should know that he can't use get_representation_path, but some other way how to load it... But technically can use get_representation_path because it does use attrib.template to fill the path, so it is ok.

In that case it still wouldn't solve the published workfiles, because the Workfiles tool > Copy & Open logic wouldn't be able to differentiate on that.

@iLLiCiTiT
Copy link
Member

In that case it still wouldn't solve the published workfiles, because the Workfiles tool > Copy & Open logic wouldn't be able to differentiate on that.

Would it be fair to say that we don't support such feature?

@BigRoy
Copy link
Collaborator Author

BigRoy commented Feb 4, 2025

Would it be fair to say that we don't support such feature?

We could, but it'd just be going back to saying "we don't support representations that are folders". Which I suppose is a 'fine' but it does add complexity in other areas like Harmony and Silhoutte (and Mocha?) where we need to be dealing with zipped representations instead. And as soon as a 'workfile' becomes a ZIP that we potentially need to access from outside the applications, e.g. on Deadline render farm we get into tricky situations there. Because then suddenly Deadline render needs to know that the path it got passed is a .zip which it should unzip first before passing it to e.g. Silhouette to render. This is a situation that is currently in-place in Harmony where we're shipping (and maintaining) a custom Deadline render plug-in for Harmony solely because we have these .zip files instead of what Harmony considers their actual "workfiles" (which are directories.)

Something that would be solved if the representations would be allowed to be folders too. Moving some of the complexity to core, but simplify the implementation across multiple host integrations and addons.

It's mostly a decision to be made at this point: Do we allow a representation to be a folder?

  • If not, then we can close this PR regardless... no matter on how we intend to integrate it.
  • If yes, then we should identify how we'd want to support it and what needs changing here (like e.g. still 'registering' the individual files for the representation and the transfers, but somehow allowing still the folder itself to be a the representation path)

@iLLiCiTiT
Copy link
Member

iLLiCiTiT commented Feb 4, 2025

It's mostly a decision to be made at this point: Do we allow a representation to be a folder?

We can allow representation to be a folder, but paths (which is metadata of representation) must contain paths to all files.

How that paths thing affects what you did here? Are you accessing the paths directly in loader? Because if you've used get_representation_path (it's current implementation) it would return the folder path.

My question regarding supporting was related to Workfiles tool > Copy & Open. Do we have to support it? Because if yes, then we would have to do much more changes regardless, even if paths in representation would lead to folder. That's adding support for folders of published paths in workfiles tool -> different topic.

@BigRoy
Copy link
Collaborator Author

BigRoy commented Feb 4, 2025

How that paths thing affects what you did here? Are you accessing the paths directly in loader? Because if you've used get_representation_path (it's current implementation) it would return the folder path.

My question regarding supporting was related to Workfiles tool > Copy & Open. Do we have to support it? Because if yes, then we would have to do much more changes regardless, even if paths in representation would lead to folder. That's adding support for folders in published paths in workfiles tool -> different topic.

Again, as stated - with this PR this already works. It's functional.

But I'd need to investigate how it'd differ if the integrator were to register the files themselves individually (whilst keeping the representation path itself to be the folder path).

@iLLiCiTiT
Copy link
Member

iLLiCiTiT commented Feb 4, 2025

Again, as stated - with this PR this already works. It's functional.

Ok, then change integrator to use all subfiles for representation paths, if representation source path is folder (should be change in get_files_info).

@iLLiCiTiT
Copy link
Member

@antirotor have you considered this use case for representation traits?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XS sponsored This is directly sponsored by a client or community member type: enhancement Improvement of existing functionality or minor addition
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix Publishing Workfiles
7 participants