Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add types to vision_transformers.py #2036

Merged
merged 1 commit into from
Nov 18, 2023
Merged

Conversation

Laurent2916
Copy link
Contributor

Hi !

In continuation of #1989, here's a PR that adds the missing types to models/vision_transformers.py, plus a few other things.
I haven't added any docstrings, as VisionTransformer already has a docstring.


def forward(self, x):
def forward(self, x: torch.Tensor) -> torch.Tensor:
B, N, C = x.shape
qkv = self.qkv(x).reshape(B, N, 3, self.num_heads, self.head_dim).permute(2, 0, 3, 1, 4)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using einops here could improve readability, though transformers chose not to, as it may interfere with torch.compiler or onnx.export (see huggingface/transformers#25110, arogozhnikov/einops#250 and arogozhnikov/einops#274)

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Nov 17, 2023

The documentation is not available anymore as the PR was closed or merged.

@rwightman
Copy link
Collaborator

@Laurent2916 I do like the typing changes, but I do not want the rest (formatting, pos -> kwarg for obvious ones, inplace changes x +=, etc).

Few comments on formatting:

  1. I disagree strongly with the black function arg alignment, it should have an extra indent level, I really hope ruff adds an option (Formatter: Specify indentation for function parameters astral-sh/ruff#8360) as black has refused despite it being mentioned in PEP and existing in IDE formatting rules for ages (like PyCharm that I use)
  2. I've been writing Python for a long time and I have the old school single quote engrained. I'm not super attached to this one but really see the point in mass changing until I decide to adopt a formatter some day, haven't found one I like without inordinat config just yet

startswith has existested for > 20 years, I'm old. It is the remove functions that are new.

@Laurent2916
Copy link
Contributor Author

Alright I reverted most of the formatting that I applied.
You're right about startswith, I didn't read the PEP carefully.

@Laurent2916 Laurent2916 changed the title Add types and slight formatting to vision_transformers.py Add types to vision_transformers.py Nov 17, 2023
@rwightman
Copy link
Collaborator

@Laurent2916 awesome, thx, should be able to merge this later today

@rwightman rwightman merged commit 21647c0 into huggingface:main Nov 18, 2023
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants