-
Notifications
You must be signed in to change notification settings - Fork 6
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 options #111
Chat options #111
Conversation
|
||
|
||
logger = logging.getLogger(__name__) | ||
|
||
|
||
STANDARD_K = 20 if COMPLETIONS_MODEL == 'gpt-4' else 10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all of these are moved over into the Settings class to be able to pass them around
# limit a string to a certain number of tokens | ||
def cap(text: str, max_tokens: int) -> str: | ||
def cap(text: str, max_tokens: int, encoder) -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the other changes in this file are basically to get it to use the settings object, rather than various constants
max_tokens_completion = remaining_tokens(prompt) | ||
max_tokens_completion = remaining_tokens(prompt, settings) | ||
if max_tokens_completion < 40: | ||
raise ValueError(f"{max_tokens_completion} tokens left for the actual query after constructing the context - aborting, as that's not going to be enough") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be changed? I arbitrarily put 40, but 100 would probably also be too few
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe it should be a class or instance variable? Also, What is the trade off here? More tokens cost more per response potentially, but has a better chance of giving the user what we want?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's already a settings.numTokens
setting which should handle that. The problem here is that a whole lot of additional context is added to the prompt before it gets to this point. Specifially:
settings.source_prompt_prefix
- up to
settings.topKBlocks
of citations from pinecone settings.source_prompt_suffix
- (optional) the last
settings.maxHistory
items from the conversation settings.question_prompt
- (optional)
settings.mode_prompt
to regulate the complexity level of the answer - the actual query
This prompt can get quite big, taking up a lot of the settings.numTokens
available tokens (in the worst case taking all of them), which limits the number left for the actual response. A seperate setting might help, but won't solve the underlying problem, which would probably require some creative book keeping to limit the number of tokens available for each step. Steps 1, 3, 5, 6 and 7 should always be sent to the LLM (but even then can take up all the tokens), so it's really only the context and history steps that should be limited.
# we add the title and authors inside the contents of the block, so that | ||
# searches for the title or author will be more likely to pull it up. This | ||
# strips it back out. | ||
def strip_block(text: str) -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this continuosly raises warnings that it can't find the titles. Is it even used anymore? Wans't that the whole point of adding the other fields?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe just change the logic to if r: return r.group(1) if r else text
? no need for warning. I'm good to delete this too.
@@ -0,0 +1,155 @@ | |||
import { useState, useEffect } from "react"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved the actual chat functionality/component bits to a separate component so it can be imported easier. This also will make it easier to move it over to the stampy-ui repo...
web/src/pages/playground.tsx
Outdated
historyFraction: 0.25, // the (approximate) fraction of num_tokens to use for history text before truncating | ||
contextFraction: 0.5, // the (approximate) fraction of num_tokens to use for context text before truncating | ||
} | ||
const COMPLETION_MODELS = ['gpt-3.5-turbo', 'gpt-4'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more can be added - these are the only ones I saw mentioned in the code
web/src/pages/playground.tsx
Outdated
parser?: (v: string) => number, | ||
} | ||
|
||
const ChatSettings = ({settings, updateSettings}: ChatSettingsParams) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
web/src/pages/playground.tsx
Outdated
updateSettings: (updater: (settings: LLMSettings) => LLMSettings) => void, | ||
} | ||
|
||
const ChatPrompts = ({settings, query, history, updateSettings}: ChatPromptParams) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
web/src/pages/playground.tsx
Outdated
/> | ||
)} | ||
</details> | ||
{history.length > 0 && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
web/src/pages/playground.tsx
Outdated
value={settings.prompts.question} | ||
onChange={updatePrompt('question')} | ||
/> | ||
<TextareaAutosize |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is where the prompt specifying the user's level goes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Epic pull request. Left a couple comments, nothing serious. Nice quality code here :)
elif completions == 'gtp-4': | ||
self.numTokens = 8191 | ||
else: | ||
self.numTokens = 4095 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be cool to drop the link to the api here in a comment: https://platform.openai.com/docs/models/gpt-4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This really should have a proper dict with the various models, but I thought it better to change as little as possible here :D
export type Mode = "rookie" | "concise" | "default"; | ||
|
||
export type LLMSettings = { | ||
prompts?: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure all these settings should have the option of being undefined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. They should have sane defaults in the API - they're mainly here to allow playing about with them, so no point in enforcing them all to be set
This one is a big boy. It adds something like OpenAI's playground for the chatbot:
I can split this into smaller PRs if this one is too large.
I'd also like to have it password protected or something, but thought that it's probably best to get the basic version out first