Ct 1501 add an exclude list to the dependency scanner #304
Conversation
| continue | ||
|
|
||
| if path in leaf_path_list: | ||
| logger.info("Skipping nested scanning of '{}' - in leaf list".format(path)) |
There was a problem hiding this comment.
The log entry message doesn't match the code.
There was a problem hiding this comment.
If you continued'd here, you'd be able to remove the duplicated if path not in leaf_path_list: in this function.
There was a problem hiding this comment.
No.. because the path itself still needs to be added to the list of dependencies on line 640. I struggled on how to do this cleanly... It really needs a class-based approach. I know @hoolymama did a big refactor on the dependency scanner. Perhaps we can merge that in to take advantage of the refactor?
There was a problem hiding this comment.
Honestly, I am pretty strongly opposed to merging this into master. We discussed many of those concerns last week, and after seeing it implemented in code, those concerns are even more concrete. The level of complexity that this introduces (to an already embarrassingly complex function), is untenable. Adding two more arguments that allow us to basically say "do what I told you.....except here...here...and here", and adding 8 more if statements, as well as introducing path conditioning (normpath) to this function...doesn't pass the sniff test. This function should simply do what it's told to do...and not to try to back out of it under xyz circumstances. This will become a maintenance/comprehension/debugging nightmare.
I don't mind if we want to create one-off "solutions" for certain customers, but I don't think it should trickle into mastr.
If we indeed cannot find another way to solve the problem that you're focused on (bypassing problematic .ass file scraping), a more palatable solution might be to simply migrate all of the .ass file scraping logic into a new function. This means that the caller would need to call the collect_dependencies function and then subsequently call this new .ass scraping function. Doing this would allow these two functions to handle a much smaller scope of conditions/arguments..thereby reducing complexity, etc.
|
|
||
| # TODO: Temporary hack to work around renderman 23.3 bug. | ||
| # Record the active renderer so that we can restore it after making this cmds.file call | ||
| active_renderer = get_active_renderer() | ||
| # Note that this command will often times return filepaths with an ending "/" on it for some reason. Strip this out at the end of the function | ||
| dependencies = cmds.file(query=True, list=True, withoutCopyNumber=True) or [] | ||
| # Strip errant dependences (another part of the renderman bug above). | ||
| dependencies = [path for path in dependencies if not path.endswith('_<user')] | ||
| dependencies = [os.path.normpath(path) for path in dependencies if not path.endswith('_<user')] |
There was a problem hiding this comment.
In my tests I found that I could easily end-up with a path that had mixed forward and back slashes (on Windows). I added that as a comment.
| @@ -516,42 +526,55 @@ def collect_dependencies(node_attrs): | |||
| # directory (i.e. it doesn't have any real smarts about path resolution, etc). | |||
| # NOTE: that this command will oftentimes return filepaths with an ending "/" on | |||
| # it for some reason. Strip this out at the end of the function | |||
| path = cmds.file(plug_value, expandName=True, query=True, withoutCopyNumber=True) | |||
| path = os.path.normpath(cmds.file(plug_value, expandName=True, query=True, withoutCopyNumber=True)) | |||
There was a problem hiding this comment.
Same answer as above...
There was a problem hiding this comment.
In general, I've tried to narrow the places in our client code for where/when paths are manipulated/conditioned. The majority of the time it occurs after dependency scraping (rather than before, or during). Specifically the process_upload_filepath function is called to explode/resolve/normalize any paths that come out of the dependency scraping phase. See here.
There certainly are exceptions to this (process_upload_filepath gets called several times within the collect_dependencies function), but they occur within isolated logic leaves (not affecting other logic leaves).
I'm not saying that this will break anything. But I wouldn't be surprised if it did.
There was a problem hiding this comment.
It's needed hear so that it can be compared to values in exlude_paths and leaf_paths
There was a problem hiding this comment.
Can we not wait until the customer has used it? If they use it and need changes they can be made on this branch and another release put out quickly.
Regarding codebase: The dependency scraping code is growing organically and, in my opinion, getting harder to follow. Dep scraping is messy business anyway. I think at some point, there should be a refactor with a divide and conquer approach. I know it feels like a big job, but I think the best opportunity to chip away at it is when adding new functionality, otherwise it's an even more daunting job next time.
Regarding UX design: It worries me that users are expected to make an explicit list of files, in a file, referred to in an environment variable, which is effectively a global variable for stuff that belongs to one shot. It's also error-prone, not just for users making mistakes in text files and env var names, but also they have to know ahead what files to ignore. Anyway, for some real feedback on UX we should see how customers get on.
I'm not convinced it should be merged to master though. It will be uncomfortable for us explaining to customers how to use it alongside the config file etc.
|
@lawschlosser @hoolymama You both have similar comments, so I'll address them together. Based on customers's experience and past support problems, I do think it's important to have a mechanism to skip and exclude files from the dependency scanning and upload process. It doesn't have to be this implementation. I totally agree that this is messy and the whole thing needs a refactor. @hoolymama I thought you already started this process in the new node-based Maya submitter? Can we merge that in as a starting point? @hoolymama It's interesting... I see environment variables not as a global but something that's easily controllable per-shot - given how most studios use them. Yes... users need to know beforehand... but that's the whole point - they inject their specific knowledge about the job submission to make it more efficient. There are many places where this is possible (hashing of the uploads is another one) |
Agreed.
It's basically a set of plugins and a runner to run (some or all of) them. To add it to client tools we would need to implement the UI part of the runner in QT. In my mind though, the main reason the beta Maya submitter is not ready for more serious work, is that it lacks some scraping functionality, such as yeti and xgen. Those would be separate plugins, but I'm procrastinating learning them.
I can see that for some studios. If they have for example a cascading series of environment files, there will be a unique env for the shot. But still, those variables (or the files they point to) have to be maintained in parallel as the shot evolves. When you want to render a previous version, you need a corresponding version of the exclusions file. In general, I think there's a pattern. The scope of functionality that we provide, grows as we get feature requests, and have to deal with stuff like this. That means configuration becomes more complex and specific to the scene file. To me, it makes sense to store as much of that submitter config as possible in the scene itself so it's more portable etc. |
@hoolymama To render a previous version, the file wouldn't be required. It's an optimization. Without it, the dependency scanning could take longer or extra files might be uploaded (that aren't used) but it wouldn't change the result of the render. Keep in mind that the system doesn't prevent the submitter from directly setting these paths - so it could also be an option via the GUI. |
No description provided.