🔗 Helpful links

• 🧪  See artifacts and rendered test results at hud.pytorch.org/pr/77396
• 📄  Preview Python docs built from this PR
• 📄  Preview C++ docs built from this PR
• ❓Need help or want to give feedback on the CI? Visit our office hours

✅ No Failures (50 Pending)

As of commit 3d103ba (more details on the Dr. CI page):

Looking over the implementation it looks like my outline was incomplete (also the outline predates policy consolidation), so there are a few more things to hit.

• After policy consolidation, CustomStrides also causes strides() to be virtualized. Strides will then hit strides_custom which will raise an error, as this is the default implementation. So if we do this, we also have to make strides overrideable by Python in the same way.
• This leads to a second problem, which is that once you flip on CustomStrides, there is literally no way to get at the underlying strides field on the TensorImpl object. Ordinarily, as a TensorImpl subclass you would have access to the protected strides_default method, but this is not an option for Python dispatch. It would be good if super().strides() did the right thing but I'm not exactly sure how to go about doing this. Maybe this is not a big deal as the point is you're opting out of the built in data.
• It would be better not to put the is_python_dispatch test directly inline in the is_contiguous method, as it is called from a lot of places and by putting it in the inline test you're forcing a bunch of extra cold code at all of the call sites, bloating icache. What is less certain is how exactly we should move it out of line. The minimal change is to place it inside the default virtual method implementation itself; this is cheap and easy to do but if a TensorImpl subclass overrides that virtual method it would be easy to forget to dispatch to Python if necessary and then this codepath would go dark (depending on the use case, though, this might not actually be a problem) If we need to guarantee this path gets hit for a subclass, no matter the TensorImpl, we could revive the non-virtual method which checks if python_dispatch before deciding what to do.

This also leaves me a question which is whether or not this is the best implementation strategy. An alternative implementation strategy would have been to add is_contiguous to native_functions.yaml, and then use manual_cpp_binding to maintain this method in TensorBase

  bool is_contiguous(at::MemoryFormat memory_format=at::MemoryFormat::Contiguous) const {
    return impl_->is_contiguous(memory_format);
  }

but have a test for "is Python subclass" here. But there are benefits to the approach in this PR: it is faster for the common case (since we only do one branch, not two) and it avoids having to deal with TorchScript related fallout from them having defined is_contiguous directly in TS primitives. So let's leave it as is.

Thanks for the review and discussion in the meeting, super helpful!

• After policy consolidation, CustomStrides also causes strides() to be virtualized. Strides will then hit strides_custom which will raise an error, as this is the default implementation. So if we do this, we also have to make strides overrideable by Python in the same way.

As discussed in the meeting, I'm currently limiting this diff to is_contiguous and the next to contiguous and punting on the others for now. Let me know if you feel strongly about supporting any of the other operations

• This leads to a second problem, which is that once you flip on CustomStrides, there is literally no way to get at the underlying strides field on the TensorImpl object. Ordinarily, as a TensorImpl subclass you would have access to the protected strides_default method, but this is not an option for Python dispatch. It would be good if super().strides() did the right thing but I'm not exactly sure how to go about doing this. Maybe this is not a big deal as the point is you're opting out of the built in data.

Good call, and as you point out, for us, not having the underlying strides field is indeed okay for us

• It would be better not to put the is_python_dispatch test directly inline in the is_contiguous method, as it is called from a lot of places and by putting it in the inline test you're forcing a bunch of extra cold code at all of the call sites, bloating icache. What is less certain is how exactly we should move it out of line. The minimal change is to place it inside the default virtual method implementation itself; this is cheap and easy to do but if a TensorImpl subclass overrides that virtual method it would be easy to forget to dispatch to Python if necessary and then this codepath would go dark (depending on the use case, though, this might not actually be a problem) If we need to guarantee this path gets hit for a subclass, no matter the TensorImpl, we could revive the non-virtual method which checks if python_dispatch before deciding what to do.

Thanks for the explanation and the learnings :) After a quick discussion with Christian, I've changed it the minimal change -- that is, putting it in the virtual method and leaving this up to the devs of tensor subclasses to add the Python dispatch if they'd like.

Please let me know if there are any other outstanding things!

By the way, look like this should have been marked as BC breaking?
Also, this failure (at least on compilation level) could have been easily avoided by adding default value to new argument

By the way, look like this should have been marked as BC breaking?

I don't think so, torchdistx is using private APIs. So this is not BC breaking.

By the way, look like this should have been marked as BC breaking?

I don't think so, torchdistx is using private APIs. So this is not BC breaking.

Hmm, are all APIs in C10 consider to be private?

@george-qi has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Hmm, are all APIs in C10 consider to be private?

That's a good question. At least our FC/BC guarantees don't apply to them. Just like we're "breaking" aten APIs all the time without warnings.