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

Update batch_size error message. #659

Merged
merged 1 commit into from
Oct 7, 2024
Merged

Conversation

jimburtoft
Copy link
Contributor

The previous message mentions max-input-length, even though we don't do any checks for that.

I only see this message when I compiled the model with a different batch size (or set the wrong batch size in MAX_BATCH_SIZE), so I want to add specific instructions to fix that failure case. I can't think of any other way it could happen.

I think this message also make sense in the SageMaker environment.

Things to think about:

  1. Any length limits on the error messages? I figure more descriptive is better. If it does get truncated it still makes sense.
  2. Does it make sense to be more strict with the batch size check? Currently, if you compiled with a smaller batch size, it would still pass this check. That is OK for a compiled batch size of one, but does it need to be mod configured batch size or anything?
  3. Did you really want to add some kind of a check on max-input-length / model.max_length and this was a forward looking message?

Fixes # (issue)

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you make sure to update the documentation with your changes?
  • Did you write any new necessary tests?

The previous message mentions max-input-length, even though we don't do any checks for that.  

I only see this message when I compiled the model with a different batch size (or set the wrong batch size in MAX_BATCH_SIZE), so I want to add specific instructions to fix that failure case.  I can't think of any other way it could happen.

I think this message also make sense in the SageMaker environment.

Things to think about:
1. Any length limits on the error messages?  I figure more descriptive is better.  If it does get truncated it still makes sense.
2. Does it make sense to be more strict with the batch size check?   Currently, if you compiled with a smaller batch size, it would still pass this check.  That is OK for a compiled batch size of one, but does it need to be mod configured batch size or anything?
3. Did you really want to add some kind of a check on max-input-length / model.max_length and this was a forward looking message?
@jimburtoft
Copy link
Contributor Author

@dacorvo for review.

Copy link
Collaborator

@dacorvo dacorvo left a comment

Choose a reason for hiding this comment

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

LGTM, thanks !

@dacorvo
Copy link
Collaborator

dacorvo commented Jul 18, 2024

This message comes from the old days when TGI did not accept the max_batch_size parameter and we need to use indirect means to control it.
I think all parameters could be checked when we load the model: maybe in another pull-request.

@HuggingFaceDocBuilderDev

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread. Thank you!

@jimburtoft
Copy link
Contributor Author

Hey @dacorvo , looks like this never got merged. Maybe add "checking all parameters" to a todo. The goal of this PR is to just give a specific helpful error message in this particular case.

@HuggingFaceDocBuilderDev

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread. Thank you!

@jimburtoft
Copy link
Contributor Author

Hey @dacorvo, just pinging no this error message update. Looks like you approved it but it is blocked by a failing check? I double checked my quotes.

@dacorvo dacorvo merged commit 018296c into huggingface:main Oct 7, 2024
1 check failed
@dacorvo
Copy link
Collaborator

dacorvo commented Oct 7, 2024

@jimburtoft the error is because your credentials are not sufficient for some of the tests.

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.

3 participants