search

pytorch / torchtune

A Native-PyTorch Library for LLM Fine-tuning
https://pytorch.org/torchtune/main/
BSD 3-Clause "New" or "Revised" License
3.93k stars 354 forks source link

Mighty Nit: Standardize `torch.Tensor` typing #1420

Closed SalmanMohammadi closed 1 week ago

SalmanMohammadi commented 2 weeks ago

Sometimes we typehint torch.Tensor, sometimes it's Tensor. Wouldn't it be great if it all looked the same?

varshinimuthukumar1 commented 2 weeks ago

@SalmanMohammadi Can I work on this?

SalmanMohammadi commented 2 weeks ago

Hey @varshinimuthukumar1! Thanks for asking. Absolutely. Feel free to put up a PR and ping me on it. : )

Let's come to some consensus about which we should use.

cc @RdoubleA @ebsmothers @felipemello1 @pbontrager choose your fighter

🚀 = torch.Tensor 👀 = Tensor

pbontrager commented 2 weeks ago

Why make typing take up more space than it already does?

RdoubleA commented 2 weeks ago

using Tensor requires adding an extra import line from torch import Tensor, you already have torch imported in almost all files just stick with it :)

felipemello1 commented 2 weeks ago

i think it also bothers me a bit when i see "Tensor". Ok, what type of Tensor? Is it a torch.Tensor? :D

varshinimuthukumar1 commented 2 weeks ago

Thanks for assigning this issue to me. I will put up a PR!

SalmanMohammadi commented 2 weeks ago

Not to throw a wrench in the works here but pytorch also uses Tensor...

https://pytorch.org/docs/stable/generated/torch.nn.utils.rnn.pad_sequence.html

I see both sides, mild preference for torch.Tensor

felipemello1 commented 2 weeks ago

I just dont know if the time spent on it is worth on it haha. Since @varshinimuthukumar1 is willing to contribute, maybe the time would be better spent in other tickets with "good first issue" and "community help wanted"

varshinimuthukumar1 commented 2 weeks ago

I took up this issue to get a first-feel of the code base, and contribute further.

If it is not needed, I will drop this issue, and start elsewhere :D

felipemello1 commented 2 weeks ago

I think it would be better, from a priority perspective. The torch.Tensor vs Tensor is cosmetic, but there are plenty of open issues that would have an impact by creating new features or solving small issues that affect users.

Thanks for contributing @varshinimuthukumar1 ! Feel free to share your thoughts and ask questions as you get more familiar with torchtune :)