Create jiterator cache dirs recursively #74425
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
CI Flow Status⚛️ CI FlowRuleset - Version:
|
Contributor
🔗 Helpful links
💊 CI failures summary and remediationsAs of commit 9eaa946 (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Please report bugs/suggestions to the (internal) Dr. CI Users group. |
mruberry
reviewed
Mar 21, 2022
| dir.pop_back(); | ||
| } | ||
|
|
||
| return _r_mkdir(base+dir); |
Collaborator
There was a problem hiding this comment.
Why remove the trailing slashes?
Doesn't base need to end with a slash if dir is appended to it like this?
Collaborator
Author
There was a problem hiding this comment.
You are right that a forward slash is indeed needed and in the currently used code the dir provides it here.
However, it might be a better idea to either:
- allow two forward slashes in the path between
baseanddir(it should be ignored if it's not at the beginning of a path if I'm not mistaken) and remove the deletion inbase - check if
basecontains the/at the end ordirat the beginning and only keep one.
Let me know which approach sounds better.
Collaborator
There was a problem hiding this comment.
Aha, thank you. I suppose it's fine, then, and I don't know if it's interesting to try and "fix" user paths.
We should add a test that the cache is working, however. Not for this PR, necessarily, but @ngimel and I were thinking that a simple test which calls a jiterated kernel and then checks that the directory contains a file would be a good sanity check.
mruberry
approved these changes
Mar 22, 2022
Collaborator
|
@pytorchbot merge this please |
Contributor
|
Hey @ptrblck. |
Contributor
|
Had to revert change as it caused a lots of internal failures, where there are stronger Re-landing (with minimal changes) in #74592 |
Contributor
|
@pytorchbot revert this |
Collaborator
|
Reverting PR 74425 failed due to 'nodyText' |
Contributor
|
@pytorchbot revert this |
pytorchmergebot
added a commit
that referenced
this pull request
Mar 22, 2022
This reverts commit 6f05edd. Reverted #74425 on behalf of https://github.com/malfet
malfet
pushed a commit
that referenced
this pull request
Mar 23, 2022
Fixes #74415 @mruberry The change expects the base directories (`HOME/TEMP`, `XDG_CACHE_HOME`, or the user-defined `PYTORCH_KERNEL_CACHE_PATH`) to exist to avoid potentially exploiting the recursive folder creation. Let me know, if this is not a concern from your side and this PR should be simplified. Pull Request resolved: #74425 Approved by: https://github.com/mruberry
facebook-github-bot
pushed a commit
that referenced
this pull request
Mar 23, 2022
Summary: Reland of #74425 with internal compilation error fixed The change expects the base directories (`HOME/TEMP`, `XDG_CACHE_HOME`, or the user-defined `PYTORCH_KERNEL_CACHE_PATH`) to exist to avoid potentially exploiting the recursive folder creation. Pull Request resolved: #74592 Reviewed By: mruberry Differential Revision: D35066710 Pulled By: malfet fbshipit-source-id: c26aff826b0a3d6ca99286b031711698a515fbbb
pytorchmergebot
pushed a commit
that referenced
this pull request
Mar 23, 2022
Summary: Reland of #74425 with internal compilation error fixed The change expects the base directories (`HOME/TEMP`, `XDG_CACHE_HOME`, or the user-defined `PYTORCH_KERNEL_CACHE_PATH`) to exist to avoid potentially exploiting the recursive folder creation. Pull Request resolved: #74592 Reviewed By: mruberry Differential Revision: D35066710 Pulled By: malfet fbshipit-source-id: c26aff826b0a3d6ca99286b031711698a515fbbb (cherry picked from commit 99479e5)
shahofblah
pushed a commit
that referenced
this pull request
Mar 25, 2022
Fixes #74415 @mruberry The change expects the base directories (`HOME/TEMP`, `XDG_CACHE_HOME`, or the user-defined `PYTORCH_KERNEL_CACHE_PATH`) to exist to avoid potentially exploiting the recursive folder creation. Let me know, if this is not a concern from your side and this PR should be simplified. Pull Request resolved: #74425 Approved by: https://github.com/mruberry
shahofblah
pushed a commit
that referenced
this pull request
Mar 25, 2022
This reverts commit 6f05edd. Reverted #74425 on behalf of https://github.com/malfet
shahofblah
pushed a commit
that referenced
this pull request
Mar 25, 2022
Summary: Reland of #74425 with internal compilation error fixed The change expects the base directories (`HOME/TEMP`, `XDG_CACHE_HOME`, or the user-defined `PYTORCH_KERNEL_CACHE_PATH`) to exist to avoid potentially exploiting the recursive folder creation. Pull Request resolved: #74592 Reviewed By: mruberry Differential Revision: D35066710 Pulled By: malfet fbshipit-source-id: c26aff826b0a3d6ca99286b031711698a515fbbb (cherry picked from commit 99479e5)
malfet
added a commit
that referenced
this pull request
Mar 30, 2022
It caused a number of internal only compilation failures, for example see: #74425 (comment) and #74542 (comment)
malfet
added a commit
that referenced
this pull request
Mar 31, 2022
It caused a number of internal only compilation failures, for example see: #74425 (comment) and #74542 (comment)
malfet
added a commit
that referenced
this pull request
Mar 31, 2022
It caused a number of internal only compilation failures, for example see: #74425 (comment) and #74542 (comment)
malfet
added a commit
that referenced
this pull request
Mar 31, 2022
It caused a number of internal only compilation failures, for example see: #74425 (comment) and #74542 (comment)
malfet
added a commit
that referenced
this pull request
Mar 31, 2022
It caused a number of internal only compilation failures, for example see: #74425 (comment) and #74542 (comment)
malfet
added a commit
that referenced
this pull request
Mar 31, 2022
It caused a number of internal only compilation failures, for example see: #74425 (comment) and #74542 (comment)
malfet
added a commit
that referenced
this pull request
Apr 1, 2022
It caused a number of internal only compilation failures, for example see: #74425 (comment) and #74542 (comment) [ghstack-poisoned]
malfet
added a commit
that referenced
this pull request
Apr 1, 2022
It caused a number of internal only compilation failures, for example see: #74425 (comment) and #74542 (comment) [ghstack-poisoned]
malfet
added a commit
that referenced
this pull request
Apr 1, 2022
It caused a number of internal only compilation failures, for example see: #74425 (comment) and #74542 (comment) [ghstack-poisoned]
malfet
added a commit
that referenced
this pull request
Apr 1, 2022
It caused a number of internal only compilation failures, for example see: #74425 (comment) and #74542 (comment) ghstack-source-id: 70e3ddd Pull Request resolved: #75085
malfet
added a commit
that referenced
this pull request
Apr 2, 2022
It caused a number of internal only compilation failures, for example see: #74425 (comment) and #74542 (comment) [ghstack-poisoned]
malfet
added a commit
that referenced
this pull request
Apr 2, 2022
It caused a number of internal only compilation failures, for example see: #74425 (comment) and #74542 (comment) [ghstack-poisoned]
malfet
added a commit
that referenced
this pull request
Apr 2, 2022
It caused a number of internal only compilation failures, for example see: #74425 (comment) and #74542 (comment) ghstack-source-id: 8db31ff Pull Request resolved: #75085
malfet
added a commit
that referenced
this pull request
Apr 5, 2022
It caused a number of internal only compilation failures, for example see: #74425 (comment) and #74542 (comment) [ghstack-poisoned]
malfet
added a commit
that referenced
this pull request
Apr 5, 2022
It caused a number of internal only compilation failures, for example see: #74425 (comment) and #74542 (comment) [ghstack-poisoned]
malfet
added a commit
that referenced
this pull request
Apr 5, 2022
It caused a number of internal only compilation failures, for example see: #74425 (comment) and #74542 (comment) ghstack-source-id: 14889fa Pull Request resolved: #75085
pytorchmergebot
pushed a commit
that referenced
this pull request
Apr 5, 2022
It caused a number of internal only compilation failures, for example see: #74425 (comment) and #74542 (comment) Pull Request resolved: #75085 Approved by: https://github.com/ngimel, https://github.com/albanD
facebook-github-bot
pushed a commit
that referenced
this pull request
Apr 7, 2022
Summary: It caused a number of internal only compilation failures, for example see: #74425 (comment) and #74542 (comment) Pull Request resolved: #75085 Approved by: https://github.com/ngimel, https://github.com/albanD Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/90a56fc515dbac9534a1a14110f9edf089430f81 Reviewed By: b0noI Differential Revision: D35404322 Pulled By: malfet fbshipit-source-id: aaa7033d0b7cbfcc1d4b3eeff86d09eba428f068
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #74415
@mruberry
The change expects the base directories (
HOME/TEMP,XDG_CACHE_HOME, or the user-definedPYTORCH_KERNEL_CACHE_PATH) to exist to avoid potentially exploiting the recursive folder creation.Let me know, if this is not a concern from your side and this PR should be simplified.