-
Notifications
You must be signed in to change notification settings - Fork 11
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
fix: job fails if Write node has no folders in filename #14
Conversation
pyproject.toml
Outdated
"deadline == 0.20.*", | ||
"openjobio == 0.8.*", | ||
"deadline == 0.21.*", | ||
"openjd == 0.10.*", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to wait for stable closed-beta installers before we merge this dep update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I will revert these changes for the time being
Signed-off-by: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com>
Signed-off-by: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com>
Signed-off-by: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com>
Signed-off-by: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com>
Signed-off-by: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com>
Signed-off-by: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com>
Signed-off-by: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com>
Signed-off-by: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com>
Signed-off-by: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com>
Signed-off-by: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com>
Signed-off-by: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com>
Signed-off-by: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com>
8a365c0
to
92a099d
Compare
Signed-off-by: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com>
Signed-off-by: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com>
@@ -228,6 +228,7 @@ def _run_job_bundle_output_test(test_dir: str, dcc_scene_file: str, report_fh, m | |||
) | |||
contents = contents.replace(tempdir, "/normalized/job/bundle/dir") | |||
contents = contents.replace(tempdir.replace("\\", "/"), "/normalized/job/bundle/dir") | |||
contents = contents.replace(os.getcwd(), "/normalized/cwd") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect we should explicitly set the CWD to a known temporary directory different from the job bundle dir, to ensure this has predictable behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I am following the comment.
the CWD in my case was no the job bundle dir, but the path where I launch Nuke from (which happened to be the root of this repo).
This currently works because os.getcwd
does not change from the moment job attachments collects the paths to this point.
Can you clarify what is the unpredictable behavior you are concerned about?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CWD depends on how the user launched Nuke. It might contain an arbitrary set of files in it, that could be read or ignored by Nuke or scripts embedded in a Nuke scene. Creating a fresh temporary directory, and setting the CWD to it before running the tests eliminates that potential variability.
Signed-off-by: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
return nuke.root().knob("project_directory").getEvaluatedValue() | ||
project_path = nuke.root().knob("project_directory").getEvaluatedValue() | ||
if not project_path: | ||
project_path = os.getcwd() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to set this to the directory of the nuke.scriptName()
instead of the directory from which Nuke was launched?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nuke takes cwd as that path. Meaning, if we didnt define project_directory
and have a relative file path in a Write
node. The file will go to cwd.
So here we have to take the same approach, otherwise you could have project_directory
undefined and a relative path in a Read
node and assume that you are relative to nuke.scriptName
, but Nuke is realtive to cwd.
I have seen some cases where TDs set this up relative to the cwd and have artists launch the tool from a specific path (e.g. from within the "shot" path).
Signed-off-by: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com>
Signed-off-by: Esteban Papp <81431996+amznestebanpapp@users.noreply.github.com>
What was the problem/requirement? (What/Why)
When submitting a job in Nuke that has a
Write
node with a filename that does not contain a folder, the job fails.Example nk:
Error:
What was the solution? (How)
Check that there is a folder in the filename before trying to create one.
What is the impact of this change?
Despite uncommon in practice, a
Write
node does not necessarily has to contain a folder. A user could indicate an output filename without a folder.How was this change tested?
Above nk script was used to submit jobs. A job with a filename that does not contain a folder was submitted successful with these changes.
Other changes were done to be able to upload wheels (also tested).
Did you run the "Job Bundle Output Tests"? If not, why not? If so, paste the test results here.
The difference is because I am using a newer version of Nuke. No other differences found:
Was this change documented?
No
Is this a breaking change?
Yes, this is updating the dependencies to
deadline-cloud
andopenjobio
, its also changing the module name toopenjd
since that is what is being generated.