🔗 Helpful links

• 🧪  See artifacts and rendered test results at hud.pytorch.org/pr/77970
• 📄  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 (0 Pending)

As of commit 67a6a93 (more details on the Dr. CI page):

I'm a bit iffy on this. Why not extend _make_subclass and then call it directly from __new__?

The chain of calls is
__new__ -> THPVariable_pynew -> base_tensor_ctor -> legacy_tensor_generic_ctor_new.

legacy_tensor_generic_ctor_new is what does the parsing of new schemas and where I added the logic here. Once it returns the new Tensor to THPVariable_pynew then THPVariable_NewWithVar does the wrapping of the new tensor to its potential Tensor subclass.

make_subclass does the parsing of schemas (_make_subclass(PyObject* cls, Tensor data, bool require_grad=False, *, bool dispatch_strides=False) , the creation of the new Tensor, and the wrapping of its new tensor to its potential Tensor subclass.

So it's really only

    if (r.toBool(1)) {
      alias.unsafeGetTensorImpl()->set_sizes_strides_policy(c10::TensorImpl::SizesStridesPolicy::CustomStrides);
    }
    if (r.toBool(2)) {
      alias.unsafeGetTensorImpl()->set_custom_device(true);
    }
    return alias;

that could be shared without a lot of refactoring, since _make_subclass both does parsing of schemas and the wrapping of tensors to its subclasses...

Unless there's some easy refactoring I'm missing or larger refactoring I should do doesn't seem like a lot to share ?

It's not about refactoring to share; it's related to the earlier question which is why you needed to use BaseTensor.__new__ as opposed to defining it directly and calling _make_subclass inside __new__, so you wouldn't exercise the __new__ codepath at all.

BTW, the underlying motivation here is that __new__ is exercisable by anyone who does a Tensor(...) call, so I'd like to avoid sprouting API surface here.

From @samdow

So typically if you pass requires_grad to a tensor constructor, that means that you want the subclass that you're creating to requires gradient, not the tensor that it's wrapping. It looks like from this #73727, it accepts a single tensor, so super().new() calls that and if the tensor requires gradient, it will wrap it like normal and won't put requires_grad on the wrapper tensor

as to why

    def __new__(cls, elem, *, requires_grad=None):
        if requires_grad is None:
            return super().__new__(cls, elem)
        else:
            return cls._make_subclass(cls, elem, requires_grad)

is required

but I updated to just call _make_subclass and it seems to work for now. Maybe something that will need to be revisited later

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

but I updated to just call _make_subclass and it seems to work for now. Maybe something that will need to be revisited later

Yeah we also saw later that the PR mentioned (which is also mentioned in the subclass zoo) is no longer relevant and now closed without being merged. To @eellison's point, it might be worth to update the subclass zoo so that it isn't too out of date, especially if someone else comes in and tries to extend BaseTensor (or use BaseTensor)

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

@pytorchbot merge this please

Hey @eellison.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.