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

Layers that are never used in ByteLatentTransformer class #28

Open
isayoften opened this issue Jan 19, 2025 · 7 comments
Open

Layers that are never used in ByteLatentTransformer class #28

isayoften opened this issue Jan 19, 2025 · 7 comments

Comments

@isayoften
Copy link

Hello! Please check this line :

self.layers = nn.ModuleList(

Image

It seems that these layers are here by mistake. All transformerblocks are defined inside the local and global parts. There is no self.layers in the forward at all also

@isayoften
Copy link
Author

The same question about:

self.tok_embeddings = torch.nn.Embedding(args.vocab_size, args.dim)

and:
self.output = nn.Linear(args.dim, args.vocab_size, bias=False)

because we already have tok_embeds and unembeds in encoder and decoder:

self.tok_embeddings = nn.Embedding(self.vocab_size, args.dim)

self.output = nn.Linear(

@isayoften
Copy link
Author

Also, we initialise tok_embeds twice because of super() call in the decoder:

super().__init__(args)

@Vectorrent
Copy link
Contributor

There is a lot of orphaned code in this project. For example:

self.pos_embeddings = nn.Embedding(args.max_length, args.dim)

Here, positional embeddings are created when args.use_rope == False - yet, these embeddings are never actually used anywhere.

@EntilZha
Copy link
Contributor

Thanks for reports on this, we'll go through and do some cleanup. Feel free to comment on any other spots.

@Vectorrent
Copy link
Contributor

Weight-tying will not work here, because self.embeddings are not valid weights:

if args.weight_tying:

@Vectorrent
Copy link
Contributor

Vectorrent commented Jan 30, 2025

Here at line 824, n-gram embeddings are initialized, only to be invalidated just after, at line 838:

self.encoder_ngram_embedding = init_embeddings(

Honestly, lines 838-849 can probably be removed, since it seems like the init_embeddings at line 824 already implements that logic gracefully.

At line 959, h_cross branches are redundant, since they always evaluate to None:

h_cross = None
(h_encoder, h_cross), cache_encoder = self.local_encoder(
    tokens=local_encoder_tokens,
    embeds=local_encoder_embeds,
    patch_embeds=h_cross if self.cross_attn_encoder else None,
    cross_mask=cross_attn_mask_enc,
    num_patches=patch_lengths.shape[1],
    patch_ids=patch_ids,
)

@Vectorrent
Copy link
Contributor

Another one here:

self.token_embedding_projection = (

self.token_embedding_projection can be created here, but is never used anywhere.

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

No branches or pull requests

3 participants