pytorch / pytorch

Tensors and Dynamic neural networks in Python with strong GPU acceleration
https://pytorch.org
Other
83.85k stars 22.61k forks source link

c10 List API hard to use #26139

Open eellison opened 5 years ago

eellison commented 5 years ago

There isn't any way to access the values of Generic List except as a reference. When writing ops this makes you do:

auto val = pop(stack);
auto list = val.toGenericListRef();

instead of:

auto list = pop(stack).toGenericList();

If you try to do the latter:

auto input_list = pop(stack).toGenericList();
for (const auto& input : input_list) {
    auto tup = input.toTuple()
}

then you get the following error:

‘const class c10::impl::ListElementReference<c10::IValue, __gnu_cxx::__normal_iterator<c10::IValue*, std::vector<c10::IValue, std::allocator > >, c10::IValue>’ has no member named ‘toTuple’

This makes it difficult to use GenericList as a value instead of a reference.

eellison commented 5 years ago

cc @smessmer

smessmer commented 5 years ago

This is a limitation that we have because we store some List<T> as vector<T> (e.g. List<int64_t> is stored this way) and some as vector<IValue> (e.g. List<string> is stored that way) but we want the List<T> API to be the same for both.

The thing that breaks your code example is the auto in the for loop. If you specify the concrete type in the for loop, it would work fine.

I don't know a good way of fixing this unfortunately, apart from unifying the way we store lists (e.g. store all as vector<IValue> or all as vector<T>), but that would be a pretty intrusive change into how JIT works and it would potentially break performance.

Another potential solution would be to say we're fine with the API being different for List<int64_t> and List<string>, but that would mean we would not be able to change this decision (say suddenly store List<string> as vector<string> and not as vector<IValue> anymore) without breaking backwards compatibility. As it is right now, we can make this decision individually for different types and change that decision later without breaking the API.

eellison commented 5 years ago

Thanks for the explanation!

eellison commented 4 years ago

Is this now possible given that we unspecialized lists ?