-
Notifications
You must be signed in to change notification settings - Fork 25.7k
[LT] Merge permutation_util into master #67766
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
Conversation
Test Plan: `build/bin/test_lazy` [ghstack-poisoned]
CI Flow Status⚛️ CI FlowRuleset - Version:
You can add a comment to the PR and tag @pytorchbot with the following commands: # ciflow rerun, "ciflow/default" will always be added automatically
@pytorchbot ciflow rerun
# ciflow rerun with additional labels "-l <ciflow/label_name>", which is equivalent to adding these labels manually and trigger the rerun
@pytorchbot ciflow rerun -l ciflow/scheduled -l ciflow/slowFor more information, please take a look at the CI Flow Wiki. |
🔗 Helpful links
💊 CI failures summary and remediationsAs of commit 6ba4c18 (more details on the Dr. CI page):
ci.pytorch.org: 1 failedThis comment was automatically generated by Dr. CI (expand for details).Please report bugs/suggestions to the (internal) Dr. CI Users group. |
tools/build_variables.bzl
Outdated
| "torch/csrc/lazy/core/ir.cpp", | ||
| "torch/csrc/lazy/core/ir_metadata.cpp", | ||
| "torch/csrc/lazy/core/ir_util.cpp", | ||
| "torch/csrc/lazy/tensor/permutation_util.cpp", |
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.
@wconstab I put the new file in a new subdirectory. Want to hear what's your plan in organizing files here. Do you prefer putting everything under core? Or is core just for IR nodes?
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 don't have a grand plan; I think originally I did a little planning and came up with /core and /backend as two folders. Currently I'm not sure I like it anymore; In general I think I'd lean towards putting everything into one folder first, then reorganizing it once we have enough files to really justify a separate folder.
What do you think about deleting the 'core' folder entirely and just putting everything under torch/lazy for now? Except for a separate torch/lazy/ts folder for the backend impl?
torch/lazy (most stuff here)
torch/lazy/ts_backend/ (backend impl)
Or, do you like core and maybe
torch/lazy/core/ (everything else)
torch/lazy/backend/ (backend interface)
torch/lazy/ts_backend (backend implementation)
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 like keeping torch/lazy/core/ as a subdirectory. Let me move the new files into the same directory.
Test Plan: `build/bin/test_lazy` [ghstack-poisoned]
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.
This looks OK to me. I didn't check how it is used in our codebase, is it used for View classes? Assuming we need such a util and there isn't one in torch/c10, it seems fine as-is.
Yes, it is mostly used by the View class and view-like op nodes. |
|
@desertfire has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Test Plan: `build/bin/test_lazy` Differential Revision: [D32147676](https://our.internmc.facebook.com/intern/diff/D32147676) [ghstack-poisoned]
|
@desertfire has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@desertfire has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Test Plan: `build/bin/test_lazy` Differential Revision: [D32147676](https://our.internmc.facebook.com/intern/diff/D32147676) [ghstack-poisoned]
|
@desertfire has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Test Plan: `build/bin/test_lazy` Differential Revision: [D32147676](https://our.internmc.facebook.com/intern/diff/D32147676) [ghstack-poisoned]
|
@desertfire has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@desertfire merged this pull request in a473417. |
Stack from ghstack:
Test Plan:
build/bin/test_lazyDifferential Revision: D32147676