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

Fix type checking (mypy) #130

Closed
alan-cooney opened this issue Dec 26, 2022 · 6 comments · Fixed by #516
Closed

Fix type checking (mypy) #130

alan-cooney opened this issue Dec 26, 2022 · 6 comments · Fixed by #516
Labels
good first issue Good for newcomers help wanted Extra attention is needed

Comments

@alan-cooney
Copy link
Collaborator

alan-cooney commented Dec 26, 2022

Currently mypy type checks are not passing - with a large number of easy to resolve errors.

poetry run mypy transformer_lens

Resolving these would help make the codebase more robust, and the corresponding checks in /.github/workflows/checks.yml can then be enabled (they're commented out).

https://github.com/neelnanda-io/TransformerLens/blob/72b9be00c12b5be37baaa16b57e5df5e2aff3aad/.github/workflows/checks.yml#L58

@alan-cooney alan-cooney added help wanted Extra attention is needed good first issue Good for newcomers labels Dec 26, 2022
@derpyplops
Copy link
Contributor

I tried a bunch to fix it with stub packages but mainly didn't really get anywhere

@neelnanda-io
Copy link
Collaborator

Hmm, when I run mypy, most of what I see seem to be it eg failing to deal with torchtyping?
Eg:

transformer_lens/components.py:16: error: Skipping analyzing "fancy_einsum": module is installed, but missing library stubs or py.typed marker  [import]             
transformer_lens/components.py:23: error: Skipping analyzing "torchtyping": module is installed, but missing library stubs or py.typed marker  [import]              
transformer_lens/components.py:37: error: Name "batch" is not defined  [name-defined]                                                                                
transformer_lens/components.py:37: error: Name "position" is not defined  [name-defined]                                                                             
transformer_lens/components.py:38: error: Name "batch" is not defined  [name-defined]                                                                                
transformer_lens/components.py:38: error: Name "position" is not defined  [name-defined]                                                                             
transformer_lens/components.py:38: error: Name "d_model" is not defined  [name-defined]                                                                              
transformer_lens/components.py:57: error: Name "batch" is not defined  [name-defined]                                                                                
transformer_lens/components.py:57: error: Name "position" is not defined  [name-defined]                                                                             
transformer_lens/components.py:57: error: Name "d_model" is not defined  [name-defined]                                                                              
transformer_lens/components.py:58: error: Name "batch" is not defined  [name-defined]                                                                                
transformer_lens/components.py:58: error: Name "position" is not defined  [name-defined]                                                                             
transformer_lens/components.py:58: error: Name "d_vocab_out" is not defined  [name-defined]                                                                          
transformer_lens/components.py:79: error: Name "batch" is not defined  [name-defined]                                                                                
transformer_lens/components.py:79: error: Name "position" is not defined  [name-defined]                                                                             
transformer_lens/components.py:80: error: Name "batch" is not defined  [name-defined]                                                                                
transformer_lens/components.py:80: error: Name "position" is not defined  [name-defined]                                                                             
transformer_lens/components.py:80: error: Name "d_model" is not defined  [name-defined]                                                                              
transformer_lens/components.py:118: error: Name "batch" is not defined  [name-defined]                                                                               
transformer_lens/components.py:118: error: Name "position" is not defined  [name-defined]                                                                            
transformer_lens/components.py:118: error: Name "length" is not defined  [name-defined]                                                                              
transformer_lens/components.py:119: error: Name "batch" is not defined  [name-defined]                                                                               
transformer_lens/components.py:119: error: Name "position" is not defined  [name-defined]
transformer_lens/components.py:119: error: Name "length" is not defined  [name-defined]

@dkamm
Copy link
Contributor

dkamm commented Jan 15, 2023

Hi guys - just wanted to add that it looks like this might be expected because of mypy limitations (see patrick-kidger/torchtyping#35)

Patrick's first suggestion of defining an object with the strings might be the way to go. It could help solidify the naming conventions and avoid potential confusion like using "num_components" vs "components", etc.

I tried this in components.py and it got rid of the errors

from enum import Enum
class T(Enum):
    BATCH = "batch"
    D_VOCAB = "d_vocab"
    POSITION = "position"
    D_MODEL = "d_model"

# Embed & Unembed
class Embed(nn.Module):

    ...

    def forward(
        self, tokens: TT[T.BATCH, T.POSITION]
    ) -> TT[T.BATCH, T.POSITION, T.D_MODEL]:
        # If A has shape [a, b] and B has shape [c, d], then A[:, B] has shape [a, c, d]
        # B acts as a tensor of indices into the second dimension (so >=0 and <b)
        return self.W_E[tokens, :]

@alan-cooney
Copy link
Collaborator Author

@dkamm great idea! This is a far better approach

@neelnanda-io
Copy link
Collaborator

neelnanda-io commented Jan 16, 2023 via email

@dkamm
Copy link
Contributor

dkamm commented Jan 16, 2023

Sure! Will send a PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants