Consolidate customization contiguous/sizes policy into unified policy #77036
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
Prior to this PR, we had a mish-mash of ways of getting unconventional sizes/strides behavior: - In OSS (but not in fbcode), some methods are virtual and you can override them directly - There is a is_contiguous policy which is a bitfield tag that lets you toggle is_contiguous to error or hit a virtual method is_contiguous_custom if it is set. Ordinarily is_contiguous() is virtual and you can just override it, but this works EVEN IF is_contiguous() is non-virtual (e.g., in fbcode) - There is also a sizes policy which is the same idea but for sizes This PR unifies these mechanisms, and in doing so, eliminates the maybe virtual/not-virtualness of the methods in question. The primary downside of this change is that it is BC-breaking (but the BC break is very easy to fix!) The new scheme works like this: we have three levels of policy for sizes/strides (order matters). - The Default policy is a conventional dense tensor, where we use all of the built-in fields to directly represent the sizes/strides/numel/contiguity of the tensor, and it is possible to bypass virtual call entirely. - The CustomStrides policy represent tensors which have a custom notion of strides (most typically, that they don't support them), shunting strides() and is_contiguous() to virtual methods strides_custom() and is_contiguous_custom(). This INCLUDES handling for contiguity, since they typically go hand-in-hand (although the situation is murky with batched tensors). The default implementations of these functions raise errors saying the tensor doesn't support them. - The CustomSizes policy represent tensors which have a custom notion of sizes (the two notable examples are nested tensor, which doesn't have a representation of sizes in the conventional form, and XLA/LTC tensor, which synchronizes its sizes with an underlying compiler backend). This shunts sizes(), numel() and dim() (along with everything from strides) to _custom() variants. There is no special policy for erroring; instead, we just do a vcall and expect the virtual method to raise an exception (the performance hit from the vcall doesn't matter because you're about to raise a C++ exception anyway). The default implementations of all overridable functions are available at _default() which is helpful in some situations when you just want to do a "sync" and then run the conventional semantics. This PR could be extended further in two ways but I did not do them due to time constraints: - Ideally, all TENSORIMPL_MAYBE_VIRTUAL would be eliminated from TensorImpl, by using the same policy trick. - set_size and set_stride are still virtual; it's not entirely clear the same trick should be used here though as these methods are deprecated. Signed-off-by: Edward Z. Yang <ezyang@fb.com> [ghstack-poisoned]
🔗 Helpful links
❌ 2 New FailuresAs of commit ba86a68 (more details on the Dr. CI page): Expand to see more
🕵️ 2 new failures recognized by patternsThe following CI failures do not appear to be due to upstream breakages
|
…fied policy" Prior to this PR, we had a mish-mash of ways of getting unconventional sizes/strides behavior: - In OSS (but not in fbcode), some methods are virtual and you can override them directly - There is a is_contiguous policy which is a bitfield tag that lets you toggle is_contiguous to error or hit a virtual method is_contiguous_custom if it is set. Ordinarily is_contiguous() is virtual and you can just override it, but this works EVEN IF is_contiguous() is non-virtual (e.g., in fbcode) - There is also a sizes policy which is the same idea but for sizes This PR unifies these mechanisms, and in doing so, eliminates the maybe virtual/not-virtualness of the methods in question. The primary downside of this change is that it is BC-breaking (but the BC break is very easy to fix!) The new scheme works like this: we have three levels of policy for sizes/strides (order matters). - The Default policy is a conventional dense tensor, where we use all of the built-in fields to directly represent the sizes/strides/numel/contiguity of the tensor, and it is possible to bypass virtual call entirely. - The CustomStrides policy represent tensors which have a custom notion of strides (most typically, that they don't support them), shunting strides() and is_contiguous() to virtual methods strides_custom() and is_contiguous_custom(). This INCLUDES handling for contiguity, since they typically go hand-in-hand (although the situation is murky with batched tensors). The default implementations of these functions raise errors saying the tensor doesn't support them. - The CustomSizes policy represent tensors which have a custom notion of sizes (the two notable examples are nested tensor, which doesn't have a representation of sizes in the conventional form, and XLA/LTC tensor, which synchronizes its sizes with an underlying compiler backend). This shunts sizes(), numel() and dim() (along with everything from strides) to _custom() variants. There is no special policy for erroring; instead, we just do a vcall and expect the virtual method to raise an exception (the performance hit from the vcall doesn't matter because you're about to raise a C++ exception anyway). The default implementations of all overridable functions are available at _default() which is helpful in some situations when you just want to do a "sync" and then run the conventional semantics. This PR could be extended further in two ways but I did not do them due to time constraints: - Ideally, all TENSORIMPL_MAYBE_VIRTUAL would be eliminated from TensorImpl, by using the same policy trick. - set_size and set_stride are still virtual; it's not entirely clear the same trick should be used here though as these methods are deprecated. Signed-off-by: Edward Z. Yang <ezyangfb.com> [ghstack-poisoned]
…fied policy" Prior to this PR, we had a mish-mash of ways of getting unconventional sizes/strides behavior: - In OSS (but not in fbcode), some methods are virtual and you can override them directly - There is a is_contiguous policy which is a bitfield tag that lets you toggle is_contiguous to error or hit a virtual method is_contiguous_custom if it is set. Ordinarily is_contiguous() is virtual and you can just override it, but this works EVEN IF is_contiguous() is non-virtual (e.g., in fbcode) - There is also a sizes policy which is the same idea but for sizes This PR unifies these mechanisms, and in doing so, eliminates the maybe virtual/not-virtualness of the methods in question. The primary downside of this change is that it is BC-breaking (but the BC break is very easy to fix!) The new scheme works like this: we have three levels of policy for sizes/strides (order matters). - The Default policy is a conventional dense tensor, where we use all of the built-in fields to directly represent the sizes/strides/numel/contiguity of the tensor, and it is possible to bypass virtual call entirely. - The CustomStrides policy represent tensors which have a custom notion of strides (most typically, that they don't support them), shunting strides() and is_contiguous() to virtual methods strides_custom() and is_contiguous_custom(). This INCLUDES handling for contiguity, since they typically go hand-in-hand (although the situation is murky with batched tensors). The default implementations of these functions raise errors saying the tensor doesn't support them. - The CustomSizes policy represent tensors which have a custom notion of sizes (the two notable examples are nested tensor, which doesn't have a representation of sizes in the conventional form, and XLA/LTC tensor, which synchronizes its sizes with an underlying compiler backend). This shunts sizes(), numel() and dim() (along with everything from strides) to _custom() variants. There is no special policy for erroring; instead, we just do a vcall and expect the virtual method to raise an exception (the performance hit from the vcall doesn't matter because you're about to raise a C++ exception anyway). The default implementations of all overridable functions are available at _default() which is helpful in some situations when you just want to do a "sync" and then run the conventional semantics. This PR could be extended further in two ways but I did not do them due to time constraints: - Ideally, all TENSORIMPL_MAYBE_VIRTUAL would be eliminated from TensorImpl, by using the same policy trick. - set_size and set_stride are still virtual; it's not entirely clear the same trick should be used here though as these methods are deprecated. Signed-off-by: Edward Z. Yang <ezyangfb.com> [ghstack-poisoned]
…fied policy" Prior to this PR, we had a mish-mash of ways of getting unconventional sizes/strides behavior: - In OSS (but not in fbcode), some methods are virtual and you can override them directly - There is a is_contiguous policy which is a bitfield tag that lets you toggle is_contiguous to error or hit a virtual method is_contiguous_custom if it is set. Ordinarily is_contiguous() is virtual and you can just override it, but this works EVEN IF is_contiguous() is non-virtual (e.g., in fbcode) - There is also a sizes policy which is the same idea but for sizes This PR unifies these mechanisms, and in doing so, eliminates the maybe virtual/not-virtualness of the methods in question. The primary downside of this change is that it is BC-breaking (but the BC break is very easy to fix!) The new scheme works like this: we have three levels of policy for sizes/strides (order matters). - The Default policy is a conventional dense tensor, where we use all of the built-in fields to directly represent the sizes/strides/numel/contiguity of the tensor, and it is possible to bypass virtual call entirely. - The CustomStrides policy represent tensors which have a custom notion of strides (most typically, that they don't support them), shunting strides() and is_contiguous() to virtual methods strides_custom() and is_contiguous_custom(). This INCLUDES handling for contiguity, since they typically go hand-in-hand (although the situation is murky with batched tensors). The default implementations of these functions raise errors saying the tensor doesn't support them. - The CustomSizes policy represent tensors which have a custom notion of sizes (the two notable examples are nested tensor, which doesn't have a representation of sizes in the conventional form, and XLA/LTC tensor, which synchronizes its sizes with an underlying compiler backend). This shunts sizes(), numel() and dim() (along with everything from strides) to _custom() variants. There is no special policy for erroring; instead, we just do a vcall and expect the virtual method to raise an exception (the performance hit from the vcall doesn't matter because you're about to raise a C++ exception anyway). The default implementations of all overridable functions are available at _default() which is helpful in some situations when you just want to do a "sync" and then run the conventional semantics. This PR could be extended further in two ways but I did not do them due to time constraints: - Ideally, all TENSORIMPL_MAYBE_VIRTUAL would be eliminated from TensorImpl, by using the same policy trick. - set_size and set_stride are still virtual; it's not entirely clear the same trick should be used here though as these methods are deprecated. Signed-off-by: Edward Z. Yang <ezyangfb.com> [ghstack-poisoned]
…fied policy" Prior to this PR, we had a mish-mash of ways of getting unconventional sizes/strides behavior: - In OSS (but not in fbcode), some methods are virtual and you can override them directly - There is a is_contiguous policy which is a bitfield tag that lets you toggle is_contiguous to error or hit a virtual method is_contiguous_custom if it is set. Ordinarily is_contiguous() is virtual and you can just override it, but this works EVEN IF is_contiguous() is non-virtual (e.g., in fbcode) - There is also a sizes policy which is the same idea but for sizes This PR unifies these mechanisms, and in doing so, eliminates the maybe virtual/not-virtualness of the methods in question. The primary downside of this change is that it is BC-breaking (but the BC break is very easy to fix!) The new scheme works like this: we have three levels of policy for sizes/strides (order matters). - The Default policy is a conventional dense tensor, where we use all of the built-in fields to directly represent the sizes/strides/numel/contiguity of the tensor, and it is possible to bypass virtual call entirely. - The CustomStrides policy represent tensors which have a custom notion of strides (most typically, that they don't support them), shunting strides() and is_contiguous() to virtual methods strides_custom() and is_contiguous_custom(). This INCLUDES handling for contiguity, since they typically go hand-in-hand (although the situation is murky with batched tensors). The default implementations of these functions raise errors saying the tensor doesn't support them. - The CustomSizes policy represent tensors which have a custom notion of sizes (the two notable examples are nested tensor, which doesn't have a representation of sizes in the conventional form, and XLA/LTC tensor, which synchronizes its sizes with an underlying compiler backend). This shunts sizes(), numel() and dim() (along with everything from strides) to _custom() variants. There is no special policy for erroring; instead, we just do a vcall and expect the virtual method to raise an exception (the performance hit from the vcall doesn't matter because you're about to raise a C++ exception anyway). The default implementations of all overridable functions are available at _default() which is helpful in some situations when you just want to do a "sync" and then run the conventional semantics. This PR could be extended further in two ways but I did not do them due to time constraints: - Ideally, all TENSORIMPL_MAYBE_VIRTUAL would be eliminated from TensorImpl, by using the same policy trick. - set_size and set_stride are still virtual; it's not entirely clear the same trick should be used here though as these methods are deprecated. Signed-off-by: Edward Z. Yang <ezyangfb.com> [ghstack-poisoned]
ezyang
added a commit
to pytorch/xla
that referenced
this pull request
May 8, 2022
Companion PR for pytorch/pytorch#77036 Signed-off-by: Edward Z. Yang <ezyang@fb.com>
|
Companion XLA PR: pytorch/xla#3555 |
ezyang
added a commit
to pytorch/functorch
that referenced
this pull request
May 8, 2022
Companion PR for pytorch/pytorch#77036 Signed-off-by: Edward Z. Yang <ezyang@fb.com>
This was referenced May 8, 2022
…fied policy" Prior to this PR, we had a mish-mash of ways of getting unconventional sizes/strides behavior: - In OSS (but not in fbcode), some methods are virtual and you can override them directly - There is a is_contiguous policy which is a bitfield tag that lets you toggle is_contiguous to error or hit a virtual method is_contiguous_custom if it is set. Ordinarily is_contiguous() is virtual and you can just override it, but this works EVEN IF is_contiguous() is non-virtual (e.g., in fbcode) - There is also a sizes policy which is the same idea but for sizes This PR unifies these mechanisms, and in doing so, eliminates the maybe virtual/not-virtualness of the methods in question. The primary downside of this change is that it is BC-breaking (but the BC break is very easy to fix!) The new scheme works like this: we have three levels of policy for sizes/strides (order matters). - The Default policy is a conventional dense tensor, where we use all of the built-in fields to directly represent the sizes/strides/numel/contiguity of the tensor, and it is possible to bypass virtual call entirely. - The CustomStrides policy represent tensors which have a custom notion of strides (most typically, that they don't support them), shunting strides() and is_contiguous() to virtual methods strides_custom() and is_contiguous_custom(). This INCLUDES handling for contiguity, since they typically go hand-in-hand (although the situation is murky with batched tensors). The default implementations of these functions raise errors saying the tensor doesn't support them. - The CustomSizes policy represent tensors which have a custom notion of sizes (the two notable examples are nested tensor, which doesn't have a representation of sizes in the conventional form, and XLA/LTC tensor, which synchronizes its sizes with an underlying compiler backend). This shunts sizes(), numel() and dim() (along with everything from strides) to _custom() variants. There is no special policy for erroring; instead, we just do a vcall and expect the virtual method to raise an exception (the performance hit from the vcall doesn't matter because you're about to raise a C++ exception anyway). The default implementations of all overridable functions are available at _default() which is helpful in some situations when you just want to do a "sync" and then run the conventional semantics. This PR could be extended further in two ways but I did not do them due to time constraints: - Ideally, all TENSORIMPL_MAYBE_VIRTUAL would be eliminated from TensorImpl, by using the same policy trick. - set_size and set_stride are still virtual; it's not entirely clear the same trick should be used here though as these methods are deprecated. Signed-off-by: Edward Z. Yang <ezyangfb.com> [ghstack-poisoned]
ezyang
added a commit
that referenced
this pull request
May 8, 2022
Prior to this PR, we had a mish-mash of ways of getting unconventional sizes/strides behavior: - In OSS (but not in fbcode), some methods are virtual and you can override them directly - There is a is_contiguous policy which is a bitfield tag that lets you toggle is_contiguous to error or hit a virtual method is_contiguous_custom if it is set. Ordinarily is_contiguous() is virtual and you can just override it, but this works EVEN IF is_contiguous() is non-virtual (e.g., in fbcode) - There is also a sizes policy which is the same idea but for sizes This PR unifies these mechanisms, and in doing so, eliminates the maybe virtual/not-virtualness of the methods in question. The primary downside of this change is that it is BC-breaking (but the BC break is very easy to fix!) The new scheme works like this: we have three levels of policy for sizes/strides (order matters). - The Default policy is a conventional dense tensor, where we use all of the built-in fields to directly represent the sizes/strides/numel/contiguity of the tensor, and it is possible to bypass virtual call entirely. - The CustomStrides policy represent tensors which have a custom notion of strides (most typically, that they don't support them), shunting strides() and is_contiguous() to virtual methods strides_custom() and is_contiguous_custom(). This INCLUDES handling for contiguity, since they typically go hand-in-hand (although the situation is murky with batched tensors). The default implementations of these functions raise errors saying the tensor doesn't support them. - The CustomSizes policy represent tensors which have a custom notion of sizes (the two notable examples are nested tensor, which doesn't have a representation of sizes in the conventional form, and XLA/LTC tensor, which synchronizes its sizes with an underlying compiler backend). This shunts sizes(), numel() and dim() (along with everything from strides) to _custom() variants. There is no special policy for erroring; instead, we just do a vcall and expect the virtual method to raise an exception (the performance hit from the vcall doesn't matter because you're about to raise a C++ exception anyway). The default implementations of all overridable functions are available at _default() which is helpful in some situations when you just want to do a "sync" and then run the conventional semantics. This PR could be extended further in two ways but I did not do them due to time constraints: - Ideally, all TENSORIMPL_MAYBE_VIRTUAL would be eliminated from TensorImpl, by using the same policy trick. - set_size and set_stride are still virtual; it's not entirely clear the same trick should be used here though as these methods are deprecated. Signed-off-by: Edward Z. Yang <ezyangfb.com> ghstack-source-id: af02861 Pull Request resolved: #77036
|
Can you add shunting for |
…fied policy" Prior to this PR, we had a mish-mash of ways of getting unconventional sizes/strides behavior: - In OSS (but not in fbcode), some methods are virtual and you can override them directly - There is a is_contiguous policy which is a bitfield tag that lets you toggle is_contiguous to error or hit a virtual method is_contiguous_custom if it is set. Ordinarily is_contiguous() is virtual and you can just override it, but this works EVEN IF is_contiguous() is non-virtual (e.g., in fbcode) - There is also a sizes policy which is the same idea but for sizes This PR unifies these mechanisms, and in doing so, eliminates the maybe virtual/not-virtualness of the methods in question. The primary downside of this change is that it is BC-breaking (but the BC break is very easy to fix!) The new scheme works like this: we have three levels of policy for sizes/strides (order matters). - The Default policy is a conventional dense tensor, where we use all of the built-in fields to directly represent the sizes/strides/numel/contiguity of the tensor, and it is possible to bypass virtual call entirely. - The CustomStrides policy represent tensors which have a custom notion of strides (most typically, that they don't support them), shunting strides() and is_contiguous() to virtual methods strides_custom() and is_contiguous_custom(). This INCLUDES handling for contiguity, since they typically go hand-in-hand (although the situation is murky with batched tensors). The default implementations of these functions raise errors saying the tensor doesn't support them. - The CustomSizes policy represent tensors which have a custom notion of sizes (the two notable examples are nested tensor, which doesn't have a representation of sizes in the conventional form, and XLA/LTC tensor, which synchronizes its sizes with an underlying compiler backend). This shunts sizes(), numel() and dim() (along with everything from strides) to _custom() variants. There is no special policy for erroring; instead, we just do a vcall and expect the virtual method to raise an exception (the performance hit from the vcall doesn't matter because you're about to raise a C++ exception anyway). The default implementations of all overridable functions are available at _default() which is helpful in some situations when you just want to do a "sync" and then run the conventional semantics. This PR could be extended further in two ways but I did not do them due to time constraints: - Ideally, all TENSORIMPL_MAYBE_VIRTUAL would be eliminated from TensorImpl, by using the same policy trick. - set_size and set_stride are still virtual; it's not entirely clear the same trick should be used here though as these methods are deprecated. Signed-off-by: Edward Z. Yang <ezyangfb.com> [ghstack-poisoned]
ezyang
added a commit
that referenced
this pull request
May 8, 2022
Prior to this PR, we had a mish-mash of ways of getting unconventional sizes/strides behavior: - In OSS (but not in fbcode), some methods are virtual and you can override them directly - There is a is_contiguous policy which is a bitfield tag that lets you toggle is_contiguous to error or hit a virtual method is_contiguous_custom if it is set. Ordinarily is_contiguous() is virtual and you can just override it, but this works EVEN IF is_contiguous() is non-virtual (e.g., in fbcode) - There is also a sizes policy which is the same idea but for sizes This PR unifies these mechanisms, and in doing so, eliminates the maybe virtual/not-virtualness of the methods in question. The primary downside of this change is that it is BC-breaking (but the BC break is very easy to fix!) The new scheme works like this: we have three levels of policy for sizes/strides (order matters). - The Default policy is a conventional dense tensor, where we use all of the built-in fields to directly represent the sizes/strides/numel/contiguity of the tensor, and it is possible to bypass virtual call entirely. - The CustomStrides policy represent tensors which have a custom notion of strides (most typically, that they don't support them), shunting strides() and is_contiguous() to virtual methods strides_custom() and is_contiguous_custom(). This INCLUDES handling for contiguity, since they typically go hand-in-hand (although the situation is murky with batched tensors). The default implementations of these functions raise errors saying the tensor doesn't support them. - The CustomSizes policy represent tensors which have a custom notion of sizes (the two notable examples are nested tensor, which doesn't have a representation of sizes in the conventional form, and XLA/LTC tensor, which synchronizes its sizes with an underlying compiler backend). This shunts sizes(), numel() and dim() (along with everything from strides) to _custom() variants. There is no special policy for erroring; instead, we just do a vcall and expect the virtual method to raise an exception (the performance hit from the vcall doesn't matter because you're about to raise a C++ exception anyway). The default implementations of all overridable functions are available at _default() which is helpful in some situations when you just want to do a "sync" and then run the conventional semantics. This PR could be extended further in two ways but I did not do them due to time constraints: - Ideally, all TENSORIMPL_MAYBE_VIRTUAL would be eliminated from TensorImpl, by using the same policy trick. - set_size and set_stride are still virtual; it's not entirely clear the same trick should be used here though as these methods are deprecated. Signed-off-by: Edward Z. Yang <ezyangfb.com> ghstack-source-id: ba43663 Pull Request resolved: #77036
|
@ngimel This seems reasonable but not obviously semantics preserving (so I may have to fix up code/tests; the current PR is supposed to be semantics preserving). I'll give it a try in a follow up after I get this PR green. |
|
Companion functorch PR: pytorch/functorch#785 |
…fied policy" Prior to this PR, we had a mish-mash of ways of getting unconventional sizes/strides behavior: - In OSS (but not in fbcode), some methods are virtual and you can override them directly - There is a is_contiguous policy which is a bitfield tag that lets you toggle is_contiguous to error or hit a virtual method is_contiguous_custom if it is set. Ordinarily is_contiguous() is virtual and you can just override it, but this works EVEN IF is_contiguous() is non-virtual (e.g., in fbcode) - There is also a sizes policy which is the same idea but for sizes This PR unifies these mechanisms, and in doing so, eliminates the maybe virtual/not-virtualness of the methods in question. The primary downside of this change is that it is BC-breaking (but the BC break is very easy to fix!) The new scheme works like this: we have three levels of policy for sizes/strides (order matters). - The Default policy is a conventional dense tensor, where we use all of the built-in fields to directly represent the sizes/strides/numel/contiguity of the tensor, and it is possible to bypass virtual call entirely. - The CustomStrides policy represent tensors which have a custom notion of strides (most typically, that they don't support them), shunting strides() and is_contiguous() to virtual methods strides_custom() and is_contiguous_custom(). This INCLUDES handling for contiguity, since they typically go hand-in-hand (although the situation is murky with batched tensors). The default implementations of these functions raise errors saying the tensor doesn't support them. - The CustomSizes policy represent tensors which have a custom notion of sizes (the two notable examples are nested tensor, which doesn't have a representation of sizes in the conventional form, and XLA/LTC tensor, which synchronizes its sizes with an underlying compiler backend). This shunts sizes(), numel() and dim() (along with everything from strides) to _custom() variants. There is no special policy for erroring; instead, we just do a vcall and expect the virtual method to raise an exception (the performance hit from the vcall doesn't matter because you're about to raise a C++ exception anyway). The default implementations of all overridable functions are available at _default() which is helpful in some situations when you just want to do a "sync" and then run the conventional semantics. This PR could be extended further in two ways but I did not do them due to time constraints: - Ideally, all TENSORIMPL_MAYBE_VIRTUAL would be eliminated from TensorImpl, by using the same policy trick. - set_size and set_stride are still virtual; it's not entirely clear the same trick should be used here though as these methods are deprecated. Signed-off-by: Edward Z. Yang <ezyangfb.com> [ghstack-poisoned]
…fied policy" Prior to this PR, we had a mish-mash of ways of getting unconventional sizes/strides behavior: - In OSS (but not in fbcode), some methods are virtual and you can override them directly - There is a is_contiguous policy which is a bitfield tag that lets you toggle is_contiguous to error or hit a virtual method is_contiguous_custom if it is set. Ordinarily is_contiguous() is virtual and you can just override it, but this works EVEN IF is_contiguous() is non-virtual (e.g., in fbcode) - There is also a sizes policy which is the same idea but for sizes This PR unifies these mechanisms, and in doing so, eliminates the maybe virtual/not-virtualness of the methods in question. The primary downside of this change is that it is BC-breaking (but the BC break is very easy to fix!) The new scheme works like this: we have three levels of policy for sizes/strides (order matters). - The Default policy is a conventional dense tensor, where we use all of the built-in fields to directly represent the sizes/strides/numel/contiguity of the tensor, and it is possible to bypass virtual call entirely. - The CustomStrides policy represent tensors which have a custom notion of strides (most typically, that they don't support them), shunting strides() and is_contiguous() to virtual methods strides_custom() and is_contiguous_custom(). This INCLUDES handling for contiguity, since they typically go hand-in-hand (although the situation is murky with batched tensors). The default implementations of these functions raise errors saying the tensor doesn't support them. - The CustomSizes policy represent tensors which have a custom notion of sizes (the two notable examples are nested tensor, which doesn't have a representation of sizes in the conventional form, and XLA/LTC tensor, which synchronizes its sizes with an underlying compiler backend). This shunts sizes(), numel() and dim() (along with everything from strides) to _custom() variants. There is no special policy for erroring; instead, we just do a vcall and expect the virtual method to raise an exception (the performance hit from the vcall doesn't matter because you're about to raise a C++ exception anyway). The default implementations of all overridable functions are available at _default() which is helpful in some situations when you just want to do a "sync" and then run the conventional semantics. This PR could be extended further in two ways but I did not do them due to time constraints: - Ideally, all TENSORIMPL_MAYBE_VIRTUAL would be eliminated from TensorImpl, by using the same policy trick. - set_size and set_stride are still virtual; it's not entirely clear the same trick should be used here though as these methods are deprecated. Signed-off-by: Edward Z. Yang <ezyangfb.com> [ghstack-poisoned]
…fied policy" Prior to this PR, we had a mish-mash of ways of getting unconventional sizes/strides behavior: - In OSS (but not in fbcode), some methods are virtual and you can override them directly - There is a is_contiguous policy which is a bitfield tag that lets you toggle is_contiguous to error or hit a virtual method is_contiguous_custom if it is set. Ordinarily is_contiguous() is virtual and you can just override it, but this works EVEN IF is_contiguous() is non-virtual (e.g., in fbcode) - There is also a sizes policy which is the same idea but for sizes This PR unifies these mechanisms, and in doing so, eliminates the maybe virtual/not-virtualness of the methods in question. The primary downside of this change is that it is BC-breaking (but the BC break is very easy to fix!) The new scheme works like this: we have three levels of policy for sizes/strides (order matters). - The Default policy is a conventional dense tensor, where we use all of the built-in fields to directly represent the sizes/strides/numel/contiguity of the tensor, and it is possible to bypass virtual call entirely. - The CustomStrides policy represent tensors which have a custom notion of strides (most typically, that they don't support them), shunting strides() and is_contiguous() to virtual methods strides_custom() and is_contiguous_custom(). This INCLUDES handling for contiguity, since they typically go hand-in-hand (although the situation is murky with batched tensors). The default implementations of these functions raise errors saying the tensor doesn't support them. - The CustomSizes policy represent tensors which have a custom notion of sizes (the two notable examples are nested tensor, which doesn't have a representation of sizes in the conventional form, and XLA/LTC tensor, which synchronizes its sizes with an underlying compiler backend). This shunts sizes(), numel() and dim() (along with everything from strides) to _custom() variants. There is no special policy for erroring; instead, we just do a vcall and expect the virtual method to raise an exception (the performance hit from the vcall doesn't matter because you're about to raise a C++ exception anyway). The default implementations of all overridable functions are available at _default() which is helpful in some situations when you just want to do a "sync" and then run the conventional semantics. This PR could be extended further in two ways but I did not do them due to time constraints: - Ideally, all TENSORIMPL_MAYBE_VIRTUAL would be eliminated from TensorImpl, by using the same policy trick. - set_size and set_stride are still virtual; it's not entirely clear the same trick should be used here though as these methods are deprecated. Signed-off-by: Edward Z. Yang <ezyangfb.com> [ghstack-poisoned]
ezyang
added a commit
that referenced
this pull request
May 8, 2022
Prior to this PR, we had a mish-mash of ways of getting unconventional sizes/strides behavior: - In OSS (but not in fbcode), some methods are virtual and you can override them directly - There is a is_contiguous policy which is a bitfield tag that lets you toggle is_contiguous to error or hit a virtual method is_contiguous_custom if it is set. Ordinarily is_contiguous() is virtual and you can just override it, but this works EVEN IF is_contiguous() is non-virtual (e.g., in fbcode) - There is also a sizes policy which is the same idea but for sizes This PR unifies these mechanisms, and in doing so, eliminates the maybe virtual/not-virtualness of the methods in question. The primary downside of this change is that it is BC-breaking (but the BC break is very easy to fix!) The new scheme works like this: we have three levels of policy for sizes/strides (order matters). - The Default policy is a conventional dense tensor, where we use all of the built-in fields to directly represent the sizes/strides/numel/contiguity of the tensor, and it is possible to bypass virtual call entirely. - The CustomStrides policy represent tensors which have a custom notion of strides (most typically, that they don't support them), shunting strides() and is_contiguous() to virtual methods strides_custom() and is_contiguous_custom(). This INCLUDES handling for contiguity, since they typically go hand-in-hand (although the situation is murky with batched tensors). The default implementations of these functions raise errors saying the tensor doesn't support them. - The CustomSizes policy represent tensors which have a custom notion of sizes (the two notable examples are nested tensor, which doesn't have a representation of sizes in the conventional form, and XLA/LTC tensor, which synchronizes its sizes with an underlying compiler backend). This shunts sizes(), numel() and dim() (along with everything from strides) to _custom() variants. There is no special policy for erroring; instead, we just do a vcall and expect the virtual method to raise an exception (the performance hit from the vcall doesn't matter because you're about to raise a C++ exception anyway). The default implementations of all overridable functions are available at _default() which is helpful in some situations when you just want to do a "sync" and then run the conventional semantics. This PR could be extended further in two ways but I did not do them due to time constraints: - Ideally, all TENSORIMPL_MAYBE_VIRTUAL would be eliminated from TensorImpl, by using the same policy trick. - set_size and set_stride are still virtual; it's not entirely clear the same trick should be used here though as these methods are deprecated. Signed-off-by: Edward Z. Yang <ezyangfb.com> ghstack-source-id: 492943c Pull Request resolved: #77036
|
@pytorchbot merge this |
|
Hey @ezyang. |
ezyang
added a commit
to pytorch/xla
that referenced
this pull request
May 11, 2022
* Update for consolidated contiguous/sizes policy Companion PR for pytorch/pytorch#77036 Signed-off-by: Edward Z. Yang <ezyang@fb.com> * fixup Signed-off-by: Edward Z. Yang <ezyang@fb.com> * Delete .torch_pin
ezyang
added a commit
to pytorch/functorch
that referenced
this pull request
May 11, 2022
Companion PR for pytorch/pytorch#77036 Signed-off-by: Edward Z. Yang <ezyang@fb.com>
ezyang
added
release notes: composability
|
@ezyang I suspect this broke metal builds that are run periodically, see: https://hud.pytorch.org/minihud?name_filter=periodic%20/%20ios-12-5-1-arm64-metal%20/%20build mind looking? |
bdhirsh
added a commit
that referenced
this pull request
May 12, 2022
`FunctionalTensorWrapper` has been sneakily relying on `shallow_copy_from` to mirror metadata from the inner tensor onto the wrapper. The new changes from #77036 actually break this logic because we blindly mirror the contiguity policy from the inner tensor to the wrapper. But `FunctionalTensorWrapper` should always have the default policy - it has valid sizes and strides! I originally wrote it this way to avoid having to alter `TensorImpl.h` just for functionalization, but now we have to; `shallow_copy_from()` copies the metadata and then immediately calls `refresh_sizes()`, which fails on `FunctionalTensorWrapper` if it was just given a non-default policy (because we don't have a `sizes_custom()` method override defined). Instead, I tried to split out the copying methods in `TensorImpl.h` into a base method that copies all metadata relevant to wrappers. And I updated `shallow_copy_from` to use that, so the metadata copying isn't duplicated anywhere. Companion functorch patch: pytorch/functorch#800 [ghstack-poisoned]
facebook-github-bot
pushed a commit
that referenced
this pull request
May 13, 2022
…#77036) Summary: Prior to this PR, we had a mish-mash of ways of getting unconventional sizes/strides behavior: - In OSS (but not in fbcode), some methods are virtual and you can override them directly - There is a is_contiguous policy which is a bitfield tag that lets you toggle is_contiguous to error or hit a virtual method is_contiguous_custom if it is set. Ordinarily is_contiguous() is virtual and you can just override it, but this works EVEN IF is_contiguous() is non-virtual (e.g., in fbcode) - There is also a sizes policy which is the same idea but for sizes This PR unifies these mechanisms, and in doing so, eliminates the maybe virtual/not-virtualness of the methods in question. The primary downside of this change is that it is BC-breaking (but the BC break is very easy to fix!) The new scheme works like this: we have three levels of policy for sizes/strides (order matters). - The Default policy is a conventional dense tensor, where we use all of the built-in fields to directly represent the sizes/strides/numel/contiguity of the tensor, and it is possible to bypass virtual call entirely. - The CustomStrides policy represent tensors which have a custom notion of strides (most typically, that they don't support them), shunting strides() and is_contiguous() to virtual methods strides_custom() and is_contiguous_custom(). This INCLUDES handling for contiguity, since they typically go hand-in-hand (although the situation is murky with batched tensors). The default implementations of these functions raise errors saying the tensor doesn't support them. - The CustomSizes policy represent tensors which have a custom notion of sizes (the two notable examples are nested tensor, which doesn't have a representation of sizes in the conventional form, and XLA/LTC tensor, which synchronizes its sizes with an underlying compiler backend). This shunts sizes(), numel() and dim() (along with everything from strides) to _custom() variants. There is no special policy for erroring; instead, we just do a vcall and expect the virtual method to raise an exception (the performance hit from the vcall doesn't matter because you're about to raise a C++ exception anyway). The default implementations of all overridable functions are available at _default() which is helpful in some situations when you just want to do a "sync" and then run the conventional semantics. This PR could be extended further in two ways but I did not do them due to time constraints: - Ideally, all TENSORIMPL_MAYBE_VIRTUAL would be eliminated from TensorImpl, by using the same policy trick. - set_size and set_stride are still virtual; it's not entirely clear the same trick should be used here though as these methods are deprecated. Signed-off-by: Edward Z. Yang <ezyangfb.com> Pull Request resolved: #77036 Approved by: https://github.com/bdhirsh Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/2896f81dd4f51062f4bf5338e31d15cc3c599eb7 Reviewed By: atalman Differential Revision: D36322300 Pulled By: ezyang fbshipit-source-id: 6723db4f277837467a0f1fe65e34b9c11c615178
4 tasks
zou3519
pushed a commit
to zou3519/pytorch
that referenced
this pull request
Jul 20, 2022
…functorch#785) Companion PR for pytorch#77036 Signed-off-by: Edward Z. Yang <ezyang@fb.com>
bigfootjon
pushed a commit
that referenced
this pull request
Jul 21, 2022
…functorch#785) Companion PR for #77036 Signed-off-by: Edward Z. Yang <ezyang@fb.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 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.
Stack from ghstack (oldest at bottom):
Prior to this PR, we had a mish-mash of ways of getting unconventional
sizes/strides behavior:
In OSS (but not in fbcode), some methods are virtual and you
can override them directly
There is a is_contiguous policy which is a bitfield tag that lets
you toggle is_contiguous to error or hit a virtual method
is_contiguous_custom if it is set. Ordinarily is_contiguous()
is virtual and you can just override it, but this works EVEN IF
is_contiguous() is non-virtual (e.g., in fbcode)
There is also a sizes policy which is the same idea but for sizes
This PR unifies these mechanisms, and in doing so, eliminates the
maybe virtual/not-virtualness of the methods in question. The primary
downside of this change is that it is BC-breaking (but the BC break is
very easy to fix!)
The new scheme works like this: we have three levels of policy for
sizes/strides (order matters).
The Default policy is a conventional dense tensor, where we use
all of the built-in fields to directly represent the
sizes/strides/numel/contiguity of the tensor, and it is possible
to bypass virtual call entirely.
The CustomStrides policy represent tensors which have a custom
notion of strides (most typically, that they don't support them),
shunting strides() and is_contiguous() to virtual methods
strides_custom() and is_contiguous_custom(). This INCLUDES handling
for contiguity, since they typically go hand-in-hand (although
the situation is murky with batched tensors). The default
implementations of these functions raise errors saying the tensor
doesn't support them.
The CustomSizes policy represent tensors which have a custom
notion of sizes (the two notable examples are nested tensor, which
doesn't have a representation of sizes in the conventional form, and
XLA/LTC tensor, which synchronizes its sizes with an underlying
compiler backend). This shunts sizes(), numel() and dim() (along
with everything from strides) to _custom() variants.
There is no special policy for erroring; instead, we just do a vcall
and expect the virtual method to raise an exception (the performance
hit from the vcall doesn't matter because you're about to raise a C++
exception anyway). The default implementations of all overridable
functions are available at _default() which is helpful in some
situations when you just want to do a "sync" and then run the
conventional semantics.
This PR could be extended further in two ways but I did not do them
due to time constraints:
Ideally, all TENSORIMPL_MAYBE_VIRTUAL would be eliminated from
TensorImpl, by using the same policy trick.
set_size and set_stride are still virtual; it's not entirely clear
the same trick should be used here though as these methods are
deprecated.
Signed-off-by: Edward Z. Yang ezyang@fb.com