Skip to content

server: exit failure if --embedding is set with an incoherent --ubatch-size #6263

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

Open
phymbert opened this issue Mar 23, 2024 · 5 comments
Open
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed server/webui

Comments

@phymbert
Copy link
Collaborator

phymbert commented Mar 23, 2024

Context

there is no advantage to increase n_batch above n_ubatch with embeddings models with pooling, because the entire batch must fit in a physical batch (ie. n_ubatch). n_batch is always >= n_ubatch.

Proposition

Exit failure if --embedding is set and --ubatch-size != --batch-size in the server example. Probably also in the retrieval example in #6193.

Aldo probably KV bert.context_size must be taken into account.

@phymbert phymbert added enhancement New feature or request server/webui labels Mar 23, 2024
@phymbert phymbert self-assigned this Mar 23, 2024
@phymbert phymbert changed the title server: exit failure if --embedding is started with a wrong --ubatch-size server: exit failure if --embedding is set with an incoherent --ubatch-size Mar 23, 2024
@phymbert phymbert removed their assignment Mar 23, 2024
@phymbert phymbert added help wanted Extra attention is needed good first issue Good for newcomers labels Mar 23, 2024
@ngxson
Copy link
Collaborator

ngxson commented Mar 24, 2024

Exit failure if --embedding is set and --ubatch-size != --batch-size in the server example

Yeah I think it's ok to do so.

Aldo probably KV bert.context_size must be taken into account.

I think we should only warn the user if n_ctx_train > n_batch (just like in embedding.cpp).

If n_ctx_train < n_batch then we don't care, because users with low RAM may want to use a smaller n_batch. Only when n_tokens > n_batch then we throw an error.

@mscheong01
Copy link
Collaborator

mscheong01 commented Mar 24, 2024

For retrieval example, we assigned the n_batch value to n_ubatch.

Exit failure if --embedding is set and --ubatch-size != --batch-size in the server example.

It might be a little confusing to users bc the default value of n_batch and n_ubatch are different(users who don't specify --batch-size or --ubatch-size will 100% encounter this failure).

@phymbert
Copy link
Collaborator Author

That's the idea of this ticket, user must configure good values for the embedding model.

@mscheong01
Copy link
Collaborator

That sounds right 👍 , we just need to ensure that the error message properly guides the users to provide the same values for both --batch-size and --ubatch-size options.

@mscheong01
Copy link
Collaborator

Adding this info in the README under the --embeddings parameter would be nice too.

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

No branches or pull requests

3 participants