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

Infer step size for Embeddings #1647

Merged
merged 31 commits into from
Nov 12, 2024
Merged

Infer step size for Embeddings #1647

merged 31 commits into from
Nov 12, 2024

Conversation

KuuCi
Copy link
Contributor

@KuuCi KuuCi commented Nov 7, 2024

Theres two cases:

  1. For in batch negatives, total number of negatives is batch size - 1. And pos_step_size is 2.
  2. For hard negatives, each sample has a list of negatives that can be as long as you like. In this case you have to change the pos_step_size.

We can remove setting step_size and autoinfer without needing the pos_step_size. We will need to look at the first batch to determine how many hard negatives. If there are hard negatives, we will use that - 1 as the step size. Otherwise, we will defer to batch negatives, setting step_size = 2

Testing:

embedding-ft-no-pos-zXOtvz (off of this branch, removed pos_step_size keyword):
image

embedding-ft-pos-kl2fPy (off foundry main pos_step_size = 2):
image

embedding-ft-pos-neg-gMJSGx (off foundry main pos_step_size = 21)
image

embedding-ft-no-pos-neg-3GoEms (off of this branch, removed pos_step_size keyword):
image

Copy link
Contributor

@jacobfulano jacobfulano left a comment

Choose a reason for hiding this comment

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

Small comments

@mrdrozdov
Copy link

I really like this idea. I do have a couple soft suggestions:

  1. Rather than self._first_batch_seen can you simply set self.step_size = None during init? Then check if self.step_size is None instead of using first_batch_seen.

  2. It could be more tidy to just make step_size part of the batch. I think you could do something like batch["step_size"] = step_size, so that it is accessible in compute_score? That being said, I'm not familiar enough to foundry to know if this causes complications, and you might need to do something like batch["step_size"] = torch.tensor([step_size] * batch_size, dtype=torch.long). If you go this route, then you can ignore (1).

Anyway, the current approach is fine too, but wanted to share just in case.

@KuuCi KuuCi requested a review from mrdrozdov November 8, 2024 20:35
@KuuCi KuuCi changed the title Infer step size for Embeddings Infer step size and gather_in_batch_negatives for Embeddings Nov 8, 2024
@KuuCi KuuCi marked this pull request as ready for review November 9, 2024 00:26
@KuuCi KuuCi requested a review from a team as a code owner November 9, 2024 00:26
@KuuCi KuuCi requested a review from mrdrozdov November 9, 2024 00:26
Copy link

@mrdrozdov mrdrozdov left a comment

Choose a reason for hiding this comment

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

This looks mostly good to me!

A few requests for integration tests:

  • Confirm expected behavior when training data has no hard negatives.
  • Confirm when using hard negatives = 1.
  • Confirm when using hard negatives = 3 (or some number besides 1).

Maybe you've already done this? In which case I will switch to approve after you address my minor comments.

llmfoundry/models/llm_embed/modeling_llm_embed.py Outdated Show resolved Hide resolved
@KuuCi
Copy link
Contributor Author

KuuCi commented Nov 12, 2024

Tested with the following runs:

0 negatives: embedding-ft-no-pos-neg-0-7jyk87
image

1 negative: embedding-ft-no-pos-neg-1-zAeAzg
image

20 negatives: embedding-ft-no-pos-neg-20-EicOP8
image

All looks good

Copy link

@mrdrozdov mrdrozdov left a comment

Choose a reason for hiding this comment

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

:shipit:

@KuuCi KuuCi changed the title Infer step size and gather_in_batch_negatives for Embeddings Infer step size for Embeddings Nov 12, 2024
@KuuCi KuuCi merged commit 415ada2 into main Nov 12, 2024
10 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.

4 participants