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

Making the token count check do both max_tokens and context_window ch… #1008

Merged
merged 1 commit into from
Sep 2, 2024

Conversation

jamesrichards4
Copy link
Contributor

@jamesrichards4 jamesrichards4 commented Sep 2, 2024

We were previously only check max_tokens when the request total tokens exceeded the context window. To avoid confusing situations with the interaction of these env vars both are now always checked so that an error is thrown when the total tokens exceeds the max regardless of whether it's more or less than the context window

Changes proposed in this pull request

A new decision node returns a request handler name from 'max_exceeded'/'context_exceeded'/'pass' rather than two boolean checks in serial. This allows moving to the correct next node in all situationss

Guidance to review

Ignore the reformatting in django-app. Must have missed a Ruff pass on a previous PR

Things to check

  • I have added any new ENV vars in all deployed environments
  • I have tested any code added or changed
  • I have run integration tests

…ecks to ensure an error is thrown even when max_tokens is below context window

if total_tokens > max_tokens_allowed:
return "max_exceeded"
elif total_tokens > token_budget_remaining_in_context:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
elif total_tokens > token_budget_remaining_in_context:
if total_tokens > token_budget_remaining_in_context:

return "max_exceeded"
elif total_tokens > token_budget_remaining_in_context:
return "context_exceeded"
else:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
else:

Copy link
Collaborator

@gecBurton gecBurton left a comment

Choose a reason for hiding this comment

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

some nitpicking added, feel free to ignore

@jamesrichards4 jamesrichards4 merged commit 55f61a4 into main Sep 2, 2024
18 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.

2 participants