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

Chat/Commands: Increase output token limit for PLG #3797

Merged
merged 15 commits into from
Apr 17, 2024

Conversation

umpox
Copy link
Contributor

@umpox umpox commented Apr 15, 2024

Description

closes #3648
closes #3795
closes https://github.com/sourcegraph/cody-issues/issues/14
closes #3779
closes #3786
closes #3784
closes #3721
closes #3571
closes #3472
closes #3536

This PR:

  • Increases the output limits for Sourcegraph.com Cody usage from 1000 to 4000 tokens
  • Removes a redundant reduction in context size that will add an extra 1000 tokens to our context window. This is only necessary on certain enterprise models where the input + output lengths are computed together.

Note

This PR requires that https://github.com/sourcegraph/sourcegraph/pull/61872 is merged first

Test plan

  1. Run https://github.com/sourcegraph/sourcegraph/pull/61872 locally
  2. Trigger long context responses (e.g. "Show me 300 lines of typescript code")
  3. Expect that everything functions as normal

Comment on lines +603 to +604
: // Minus the character limit reserved for the answer token
this.chatModel.maxInputChars - tokensToChars(ANSWER_TOKENS)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This logic only makes sense for older GPT models where the input+output limit is calculated together.

image

For enterprise we will need to address this

@umpox umpox changed the title Chat/Commands: Increase output token limit Chat/Commands: Increase output token limit for PLG Apr 15, 2024
@umpox umpox marked this pull request as ready for review April 15, 2024 14:23
@umpox umpox requested review from chrsmith, abeatrix and a team April 15, 2024 14:31
Comment on lines 97 to 111
maxToken: 1800,
maxInputToken: 1800,
Copy link
Contributor

Choose a reason for hiding this comment

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

unrelated but it seems like we can use the "standard" 7k now :) https://fireworks.ai/models/fireworks/mixtral-8x22b-instruct-preview

Copy link
Contributor

@philipp-spiess philipp-spiess left a comment

Choose a reason for hiding this comment

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

LGTM :)

/**
* @deprecated Use `inputTokens` instead.
*/
tokens?: number
Copy link
Contributor

Choose a reason for hiding this comment

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

We could just remove it since this was not included in any release yet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep I can do, my only concern is that this might break any internal stuff we've configured but I guess it's still fresh so that's not much of a worry

Copy link

‼️ Hey @sourcegraph/cody-security, please review this PR carefully as it introduces usage of unsafe_ functions or abuses PromptString.

@umpox
Copy link
Contributor Author

umpox commented Apr 16, 2024

@sourcegraph/cody-security Guessing this was triggered because of the merge commit? Maybe we should exclude those?

@philipp-spiess
Copy link
Contributor

@umpox Ah no this is because we look at the files and not just the modified ranges within those files. Shoots. This actually needs a much more sophisticated approach (we need to know which ranges were modified and only emit linter issues in those places).

Hmmm maybe as a temporary workaround we can extract the unsafe_ calls in "hot" paths (like in SimpleChatPanelProvider.ts in this case) into a separate module that we don't need to touch so often? cc @sourcegraph/cody-security

I will research how other linters do that. Sorry for the churn @umpox and please ignore it for now in this PR 🥺 🙏

Copy link

‼️ Hey @sourcegraph/cody-security, please review this PR carefully as it introduces usage of unsafe_ functions or abuses PromptString.

1 similar comment
Copy link

‼️ Hey @sourcegraph/cody-security, please review this PR carefully as it introduces usage of unsafe_ functions or abuses PromptString.

@umpox
Copy link
Contributor Author

umpox commented Apr 16, 2024

:D @philipp-spiess No probs

@philipp-spiess
Copy link
Contributor

@umpox fix incoming: #3810

@umpox umpox force-pushed the tr/increase-output-tokens branch from 6138f91 to 9b8a4af Compare April 17, 2024 09:07
@umpox umpox merged commit 4b97c0d into main Apr 17, 2024
35 checks passed
@umpox umpox deleted the tr/increase-output-tokens branch April 17, 2024 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment