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

Implemented option to replace embedding vectors by non-trainable 1-hot encoding vectors #48

Merged
merged 15 commits into from
Feb 6, 2024

Conversation

josephattieh
Copy link
Collaborator

No description provided.

@TimotheeMickus TimotheeMickus requested review from TimotheeMickus and removed request for TimotheeMickus January 31, 2024 12:28
@jrvc jrvc self-requested a review February 2, 2024 15:18
embeddings = [nn.Embedding(vocab, dim, padding_idx=pad) for vocab, dim, pad in emb_params]

else:
print("CREATING EMBEDDINGLESS")
Copy link
Member

Choose a reason for hiding this comment

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

being pedantic here: would like to see a more informative log output (IFF not only for debugging purposes).
perhaps print using the logger.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed this. Was there for debugging purposes.

@@ -32,8 +32,9 @@ def attention_bridge_optimizer(model, task_queue_manager, base_optimizer):
params.append(param)
if name in suboptimizers:
raise Exception(f'Trying to create second optimizer for "{name}"')
optimizer = base_optimizer(params)
suboptimizers[name] = optimizer
if len(params) != 0:
Copy link
Member

Choose a reason for hiding this comment

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

just out of curiosity: why can params now be empty? i am not sure how this optimizer could interact only with the embeddings

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This fixes a problem in which a sub-optimiser is created for the embedding layer but there are no trainable parameters there since the weights are frozen.

mammoth/opts.py Outdated
'--enable_embeddingless',
'-enable_embeddingless',
action='store_true',
help="Enable the use of embeddingless models.",
Copy link
Member

@jrvc jrvc Feb 6, 2024

Choose a reason for hiding this comment

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

Perhaps add a reference here or a tiny description of what embeddingless is here -- e.g. byte level something something implementation based on citation_name et al. (2045)

Copy link
Member

@jrvc jrvc left a comment

Choose a reason for hiding this comment

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

It looks alright imho. Minor changes needed, at the logging level.
(Reviewer confidence: 2) XD

Copy link
Member

@jrvc jrvc left a comment

Choose a reason for hiding this comment

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

thanks for the changes

@josephattieh josephattieh merged commit 632feed into main Feb 6, 2024
2 checks passed
@josephattieh josephattieh deleted the embeddingless branch February 6, 2024 10:15
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.

2 participants