@ezyang can I get a review on this easy PR?

I would also say that the semantics are slightly weird. The output variable depends on the data of the input variable, and so there's no reason not to differentiate through this...

@apaszke I would guess the copy constructed tensor should not be made differentiable through copy op here. Please correct me if I am wrong, I think the expected behavior should be the same as:

source = torch.randn(10, requires_grad=True)
copy = torch.tensor(source.data)

To figure out a good solution to this problem, we must ask ourselves:

1. How is tensor() with a Tensor argument implemented today, and
2. Why does this implementation "do the wrong thing" with respect to requires_grad?

We can see that tensor() dispatches to tensor_ctor, which dispatches to internal_new_from_data. Here is the important lines in the function:

  if (THPVariable_Check(data)) {
      auto var = reinterpret_cast<THPVariable*>(data)->cdata;
      auto type_inference_device_type = device_opt.has_value() ? device_opt->type()
                                                               : torch::getDeviceType(var.type());
      // infer the scalar type and device type; it's not expected to infer the layout since these constructors
      // are defined per-layout-type (e.g. tensor vs sparse_coo_tensor).
      const auto& type_inference_type = torch::getVariableType(var.type().scalarType(),
                                                       *torch::getLayout(type.backend()),
                                                       type_inference_device_type);
      const auto& type_to_use = type_inference ? type_inference_type : type;
      return copy_variables ? new_with_tensor_copy(type_to_use, var, device_index) :
                              new_with_type_conversion(type_to_use, var, device_index);
  }

In the bodies of new_with_tensor_copy and new_with_type_conversion you can see this calls copy or toType on Variable, which means you get the normal gradient behavior as if you had called copy() or toType() on them, which is why you incorrectly get requires_grad at the end.

Based on this, this suggests two solutions:

1. Keep calling copy/toType, but then detach the tensor afterwards, so that you set requires_grad = false and delete the grad_fn. Effectively, you are saying that tensor(t) is equivalent to t.clone().detach(). This is the "do the wrong thing first, and then fixup after yourself" strategy.
2. Create a new operator on VariableType, new_with_tensor, which does the right thing from the get go and doesn't actually ever create a grad fn or set requires grad on the output. This is the "do the right thing from the beginning" strategy, but it is more complicated to implement.

I think both approaches are reasonable. If you want to do (2), you'll need to write a custom VariableType method implementation, based on the fact that detach is hardcoded, like this:

Tensor VariableType::detach(const Tensor & self) const {
  profiler::RecordFunction profiler("detach");
  torch::jit::Node* node = nullptr;
  if (jit::tracer::isTracing()) {
    auto& graph = jit::tracer::getTracingState()->graph;
    node = graph->create(jit::aten::detach, /*outputs=*/0);
    jit::tracer::recordSourceLocation(node);
    jit::tracer::addInputs(node, "self", self);
    graph->appendNode(node);

  }
  // <NON_GENERATED_CODE>
  auto result = as_variable_ref(const_cast<Tensor&>(self)).detach();
  // </NON_GENERATED_CODE>
  if (jit::tracer::isTracing()) {
    jit::tracer::addOutput(node, result);
  }
  return result;
}

You'll have something similar, except that it calls copy/toType as necessary. But I think detach after the fact is also a very reasonable strategy, and much simpler.

@ezyang I am picking the (2) approach here: clone() and detach() when copy construct a new tensor.

@ezyang Thanks a lot for the clarifications! Sorry I missed the default case. I will make changes so that:

source = torch.randn(3, 3, requires_grad=True)
copy = torch.tensor(source) # requires_grad=False

In general, we should discourage people from passing tensor to torch.tensor, in favor of explicitly saying x.clone().detach() or x.clone().detach().requires_grad_(True). @apaszke suggested a warning might be appropriate in this case.

@ezyang Thanks a lot for the comments! I will update the PR to make copy construct from a tensor to be leaf variable as always, and set requires_grad wrt to input args. Also raise python warnings inside this path, and update the docs altogether.