Skip to content

fix: Avoid BadRequestError due to invalid max_tokens #3667

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: Avoid BadRequestError due to invalid max_tokens #3667

wants to merge 2 commits into from

Conversation

luis5tb
Copy link
Contributor

@luis5tb luis5tb commented Oct 3, 2025 •
edited
Loading

This patch ensures if max tokens is not defined, then is set to None instead of 0 when calling openai_chat_completion. This way some providers (like gemini) that cannot handle the max_tokens = 0 will not fail

Issue: #3666

@meta-cla meta-cla bot added the CLA Signed This label is managed by the Meta Open Source bot. label Oct 3, 2025
@luis5tb luis5tb changed the title Fix BadRequestError due to unvalid max_tokens fix: Avoid BadRequestError due to invalid max_tokens Oct 3, 2025
mattf
mattf requested changes Oct 3, 2025
View reviewed changes
Copy link
Collaborator

@mattf mattf left a comment

Choose a reason for hiding this comment

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

hold on this pending discord discussion

@luis5tb luis5tb force-pushed the max_tokens branch 4 times, most recently from 43fb189 to ea42cf6 Compare October 5, 2025 08:35
mattf
mattf approved these changes Oct 10, 2025
View reviewed changes
Copy link
Collaborator

@mattf mattf left a comment

Choose a reason for hiding this comment

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

good find!

@leseb
Copy link
Collaborator

leseb commented Oct 15, 2025

@luis5tb see CI failure:

Run './scripts/integration-tests.sh --inference-mode record-if-missing' with required API keys to generate.

Thanks!

@luis5tb
Copy link
Contributor Author

luis5tb commented Oct 15, 2025

The error in the unittest is weird

@luis5tb see CI failure:

Run './scripts/integration-tests.sh --inference-mode record-if-missing' with required API keys to generate.

Thanks!

I trigger the recording workflow and now is green

@leseb
Copy link
Collaborator

leseb commented Oct 15, 2025

@luis5tb the amount of changes files seems off?

mattf
mattf requested changes Oct 15, 2025
View reviewed changes
Copy link
Collaborator

@mattf mattf left a comment

Choose a reason for hiding this comment

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

please use one of -

  • record-if-missing, or
  • record + git restore + git add

the restore will remove all the minor changes like call id and token counts

@luis5tb
Copy link
Contributor Author

luis5tb commented Oct 15, 2025 •
edited
Loading

please use one of -

  • record-if-missing, or
  • record + git restore + git add

the restore will remove all the minor changes like call id and token counts

I used the remote recording option (scripts/github/schedule-record-workflow.sh), there is no option for record-if-missing there. From what I see, using that option the "record" is hardcoded: https://github.com/llamastack/llama-stack/blob/main/.github/workflows/record-integration-tests.yml#L70

@luis5tb luis5tb force-pushed the max_tokens branch 2 times, most recently from edf6e86 to 4c68892 Compare October 15, 2025 15:07
@luis5tb
Fix BadRequestError due to unvalid max_tokens
7b8e203 
This patch ensures if max tokens is not defined it is set to None.
This avoid some providers to fail, as they don't have protection for
it being set to 0

Issue: llamastack#3666
@luis5tb
Recording update from CI (suite: base)
3cba12d 
Removed the modification, just adding the new content
@luis5tb
Copy link
Contributor Author

luis5tb commented Oct 16, 2025

please use one of -

  • record-if-missing, or
  • record + git restore + git add

the restore will remove all the minor changes like call id and token counts

I used the remote recording option (scripts/github/schedule-record-workflow.sh), there is no option for record-if-missing there. From what I see, using that option the "record" is hardcoded: https://github.com/llamastack/llama-stack/blob/main/.github/workflows/record-integration-tests.yml#L70

To try to workaround this I created it with remote, fetch the commit, reset it and only add the new files (similar to the record + git restore + git add). It is still adding a lot of files though

@luis5tb luis5tb requested a review from mattf October 16, 2025 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ashwinb ashwinb Awaiting requested review from ashwinb ashwinb is a code owner

@yanxi0830 yanxi0830 Awaiting requested review from yanxi0830 yanxi0830 is a code owner

@hardikjshah hardikjshah Awaiting requested review from hardikjshah hardikjshah is a code owner

@raghotham raghotham Awaiting requested review from raghotham raghotham is a code owner

@ehhuang ehhuang Awaiting requested review from ehhuang ehhuang is a code owner

@terrytangyuan terrytangyuan Awaiting requested review from terrytangyuan terrytangyuan is a code owner

@leseb leseb Awaiting requested review from leseb leseb is a code owner

@bbrowning bbrowning Awaiting requested review from bbrowning bbrowning is a code owner

@reluctantfuturist reluctantfuturist Awaiting requested review from reluctantfuturist reluctantfuturist is a code owner

@slekkala1 slekkala1 Awaiting requested review from slekkala1 slekkala1 is a code owner

@franciscojavierarceo franciscojavierarceo Awaiting requested review from franciscojavierarceo franciscojavierarceo is a code owner

@mattf mattf Awaiting requested review from mattf mattf is a code owner

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

CLA Signed This label is managed by the Meta Open Source bot.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@luis5tb @leseb @mattf