-
Notifications
You must be signed in to change notification settings - Fork 126
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
Add ellipse on token limit exceed and flex layout options in PromptTemplateString #694
Conversation
…for better prompt rendering control
packages/core/src/promptdom.ts
Outdated
n.resolved = value | ||
const end = Math.floor((n.maxTokens * n.resolved.length) / n.tokens) | ||
const value = n.resolved.slice(0, end) + MAX_TOKENS_ELLIPSE | ||
n.resolved = n.preview = value |
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 code is slicing the n.resolved
string without checking if the end
index is within the string length. This could potentially lead to unexpected behavior if end
is greater than the string length. Please add a check to ensure end
is within the string length. 😊
generated by pr-review-commit
missing_error_handling
packages/core/src/promptdom.ts
Outdated
) | ||
n.resolved.content = | ||
n.resolved.content.slice(0, end) + MAX_TOKENS_ELLIPSE |
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 code is slicing the n.resolved.content
string without checking if the end
index is within the string length. This could potentially lead to unexpected behavior if end
is greater than the string length. Please add a check to ensure end
is within the string length. 😊
generated by pr-review-commit
missing_error_handling
if (grow !== undefined) current.flexGrow = grow | ||
if (basis !== undefined) current.flexBasis = basis | ||
if (reserve !== undefined) current.flexReserve = reserve | ||
return res |
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 code is directly assigning values from options
to current
without any validation. This could potentially lead to unexpected behavior if options
contains invalid values. Please add validation checks for options.grow
, options.basis
, and options.reserve
before assigning them to current
. 😊
generated by pr-review-commit
missing_validation
The git diff shows modifications in four main files: constants.ts, promptdom.ts, runpromptcontext.ts, and prompt_template.d.ts.
There are no immediate functional issues evident in these changes. These changes mainly deal with the control of token budgeting and priority of elements, which would enhance the performance and efficiency of the system. So, LGTM 🚀.
|
messages.push({ role: "user", content: prompt }) | ||
if (userPrompt?.trim().length) { | ||
trace.detailsFenced(`💬 message`, userPrompt, "markdown") | ||
messages.push({ role: "user", content: userPrompt }) |
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 variable 'prompt' has been renamed to 'userPrompt'. This could potentially break any code that relies on the original 'prompt' variable.
generated by pr-review-commit
variable_renaming
@@ -418,7 +422,7 @@ export async function visitNode(node: PromptNode, visitor: PromptNodeVisitor) { | |||
} | |||
|
|||
export interface PromptNodeRender { | |||
prompt: string | |||
userPrompt: string | |||
assistantPrompt: string | |||
images: PromptImage[] | |||
errors: unknown[] |
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 'prompt' property in the 'PromptNodeRender' interface has been renamed to 'userPrompt'. This could potentially break any code that relies on the original 'prompt' property.
generated by pr-review-commit
interface_change
grow?: number | ||
reserve?: number | ||
basis?: number | ||
}): PromptTemplateString |
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.
New methods 'priority' and 'flex' have been added to the 'PromptTemplateString' interface. Ensure these methods are implemented wherever this interface is used.
generated by pr-review-commit
new_method
This pull request includes changes to add an ellipse when the token limit is exceeded and to add flex layout options in the PromptTemplateString for better prompt rendering control. The first commit adds the MAX_TOKENS_ELLIPSE constant to handle the ellipse when the token limit is exceeded. The second commit adds the flex method to the PromptTemplateString, allowing for the specification of flex layout options such as grow, basis, and reserve. These changes enhance the functionality and flexibility of the PromptTemplateString in the codebase.
MAX_TOKENS_ELLIPSE
has been introduced to denote truncation in tokens exceeding the maximum.MAX_TOKENS_ELLIPSE
is appended, and a preview of the message is also stored.priority
method has been introduced inPromptTemplateString
that sets the priority of the text, similar to CSS z-index. This will control the trimming of the prompt when the context is full. 🎉flex
method introduced inPromptTemplateString
. This provides facilities to set grow, reserve and basis values affecting allocation of available token budget among elements. 💪ContextExpansionOptions
now includes new propertiespriority
,flexGrow
,flexBasis
andflexReserve
. These properties are used for controlling token allocations in prompt templates, adding more depth to customization. 💼flex.genai.mts
has been added showcasing the usage of the newly introduced max token configuration capabilities. 📜