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
Rework compat bindings. #47863
Rework compat bindings. #47863
Conversation
[ghstack-poisoned]
💊 CI failures summary and remediationsAs of commit cf28c05 (more details on the Dr. CI page):
🕵️ 1 new failure recognized by patternsThe following CI failures do not appear to be due to upstream breakages: pytorch_bazel_build (1/1)Step: "Bazel Build" (full log | diagnosis details | 🔁 rerun)
|
# load will automatically search /usr/include, but not conda include. | ||
EXTRA_INCLUDE_PATHS.append(os.path.join(CONDA_PREFIX, "include")) | ||
|
||
|
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.
It would probably be nice to have a Note explaining at a high level what's going on here
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 understand that this is "temporary" stuff but because it interacts nontrivially with some code that isn't in PyTorch itself, it will be harder for other people to figure out how this relates to the bigger picture. A note here will help a lot.
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.
No, it's a valid point. Check out BACK_TESTING_NOTE:
and let me know if it seems reasonable.
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
# PyTorch the cost and complexity of such shims will increase. Once back | ||
# testing is no longer required (which is to say we have done enough historic | ||
# analysis and the shims no longer justify their maintenance and code | ||
# complexity costs) back testing paths will be removed. |
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.
Thanks, this note is super helpful. One extra benefit of the note is it also clues the reader in on what kinds of code changes to Timer are permissible, and what are not (changes that add more dependencies on C symbols => you need to add more backtesting support.)
[ghstack-poisoned]
Differential Revision: [D25199261](https://our.internmc.facebook.com/intern/diff/D25199261) [ghstack-poisoned]
Differential Revision: [D25199261](https://our.internmc.facebook.com/intern/diff/D25199261) [ghstack-poisoned]
Differential Revision: [D25199261](https://our.internmc.facebook.com/intern/diff/D25199261) [ghstack-poisoned]
Differential Revision: [D25199261](https://our.internmc.facebook.com/intern/diff/D25199261) [ghstack-poisoned]
Stack from ghstack:
Differential Revision: D25199261