-
Notifications
You must be signed in to change notification settings - Fork 38
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
Reduce job history dir length #561
base: mainline
Are you sure you want to change the base?
Reduce job history dir length #561
Conversation
f8a5944
to
a69e61a
Compare
# max path length - manifest file name | ||
# 256 - len("\manifests\d2b2c3102af5a862db950a2e30255429_input") | ||
# = 207 | ||
max_job_name_prefix = 207 - len(os.path.abspath(os.path.join(month_dir, job_dir_prefix))) |
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 path of \manifests\d2b2c3102af5a862db950a2e30255429_input
may become longer in the near future. Can we have more reserved characters so the structure does not change? Say 160 or 180 instead of "207" ?
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 can always shorten this more later if it becomes an issue :)
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.
Discussion on how to handle long paths?
9dc394d
to
9332598
Compare
9332598
to
5c5b2d2
Compare
5c5b2d2
to
a11cce8
Compare
# 256 - len("\manifests\d2b2c3102af5a862db950a2e30255429_input") | ||
# = 207 |
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.
nit: MAX_PATH is actually 260: https://learn.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation?tabs=registry
D:\some 256-character path string<NUL>
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.
While the official MAX_PATH is 260, there may be some DCC constraints that limit this to 256 characters, so I decided to go with that. This also gives us a few characters of wiggle room.
max_job_name_prefix = min( | ||
207 - len(os.path.abspath(os.path.join(month_dir, job_dir_prefix))), max_job_name_prefix | ||
) |
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.
Have we considered what happens if this ends up being 0 or a negative number?
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.
Added error handling for this case!
Do we have a stack trace or anything of the experience when being unable to create a history directory? Was it the job bundle history creation that failed? Perhaps for a quick fix, we could just be catching/warning on that. |
The error message:
In Cinema 4D, the console has the stack trace
|
4e5847d
a11cce8
to
4e5847d
Compare
014575d
to
1413539
Compare
Signed-off-by: Joel Wong <[email protected]>
…ory dir Signed-off-by: Joel Wong <[email protected]>
1413539
to
62d42bf
Compare
Quality Gate passedIssues Measures |
if max_job_name_prefix < 1: | ||
raise RuntimeError( | ||
"Job history directory is too long. Please update your 'settings.job_history_dir' to a shorter path." | ||
) |
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.
Sorry for the delay, unfortunately due to the customer experience impact we can't do this change.
A valid windows submission that works with long paths will now fail to submit because we won't create the job history dir (to be used as input to the create job bundle callback for where to create the submission files).
In my mind, there's 2 part of this at play:
- job bundle history directory right now is a required portion of submission or export
- in my opinion, it is an internal implementation detail that we must create a history entry to submit, rather than populate it after submission with best effort.
- using the library in a runtime that doesn't support long paths will have submission issues based off the job bundle directory location + job name length
I think short-term we can make the second issue less likely and do the truncation but only if it would fail otherwise. We can't have previously working setups failing due to this. If we can successfully create the history directory and a file whose path matches the longest path therein, then no truncation should happen.
Longer-term I think we should absolutely remove the truncation and:
- use a local temp folder to allow submission and then move it to the history folder after submission, and/or
- leverage
\\?\
for this specific case of the internal implementation detail of having a history directory and how we populate it.
Let me know your thoughts
Related to: aws-deadline/deadline-cloud-for-cinema-4d#105
What was the problem/requirement? (What/Why)
When submitting jobs from DCCs which don't have long path support, the job history bundle will not be able to be created if the path is too long due to a long job name.
What was the solution? (How)
Truncated the job history dir for the submitted bundle so that the job history bundle, including asset manifest, can be created.
What is the impact of this change?
Previously failing submissions caused by long paths in the job history dir will succeed for DCCs which don't support long paths.
How was this change tested?
hatch build && hatch shell
Confirmed that paths were truncated when submitting a job with a long name but the job as still able to submit successfully.
Edited my
settings.job_history
dir to a very long directory and got the expected errorWas this change documented?
No, N/A
Does this PR introduce new dependencies?
Is this a breaking change?
No
Does this change impact security?
No
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.