Before this change, I think when creating a PyObjectHolder, the passed py obj ref count +2 and then -1. This change makes it only +1 one time.

Since you are making this data wrapper take care of acquiring GIL on destruction.

I wonder can it also acquires GIL on creation?

So that you don't have to do it in upper level, as in #37871.

    case TypeKind::PyObjectType:
      // convert a py::handle to the IValue that holds the py::object
      py::object py_obj;
      {
        py::gil_scoped_acquire ag;
        py_obj = obj.cast<py::object>();
      }}
      return c10::ivalue::ConcretePyObjectHolder::create(
          std::move(py_obj));

good point! I will make the change if this looks good to @wanchaol too.

can we move the gil acquisition to the constructor as well (i.e. move the logic from toIValue to here)?

I guess it's to change the constructor takes py::handle, and cast it to py::object with GIL

Yes :)

Actually, I would prefer adding a new constructor that takes a py::handle, and move the logic to there. We should still keep the constructor that takes py::object without acquire gil for C++ usage.