Move LLModel outside of Prompt, also clean up memory feature, and add prompt.withUpdatedParams and prompt.withUpdatedMessages functions #25
Conversation
Ololoshechkin
force-pushed
the
vbr/prompt-api-improvements
branch
from
27c0c65 to
58610aa
Compare
|
Just to confirm, was |
|
Yes, prompt contains the LLMParams for now. Params can be shared between most of the models (except the case when some model doesn't support one of them, which is pretty rare case), and in fact in many cases they are not touched/changed. |
|
|
||
| fun withMessages(newMessages: List<Message>): Prompt = copy(messages = newMessages) | ||
|
|
||
| fun withUpdatedMessages(update: MutableList<Message>.() -> Unit): Prompt = |
There was a problem hiding this comment.
Does it have to be mutable? Seems like it works with just regular list too
There was a problem hiding this comment.
dropLast() ?
There was a problem hiding this comment.
prompt. withUpdatedMessages { dropLast(2) }
|
|
||
| fun withParams(newParams: LLMParams): Prompt = copy(params = newParams) | ||
|
|
||
| class LLMParamsUpdateContext internal constructor( |
There was a problem hiding this comment.
I see that you are creating a new class with the same fields as in LLMParams, just to make them mutable. I don't have a strong opinion against it, just wanted to confirm, do you think it's necessary? We will now have to maintain two similar sets of fields, and to always keep in mind that we will have to add new fields in two places. Maybe it would be fine to just ask user to provide a new LLMParam object instead of creating this convenience abstraction? If you think it's still worth it, feel free to disregard this comment
There was a problem hiding this comment.
I wanted to allow writing easily (same as with prompt. withUpdatedMessages { dropLast(2) }) :
prompt. withUpdatedParams { temperature = 100500 }
| } | ||
| if (model != null) changeModel(model) | ||
|
|
||
| updatePrompt { | ||
| system(systemMessage) |
There was a problem hiding this comment.
I'm a bit concerned with system here. There's only a single system message allowed in the prompt (I already shot myself in the leg with this one). This looks like it can lead to the same problem, since it's not rewriting, but updating the prompt. Or am I missing the point?
There was a problem hiding this comment.
I think OpenAI supports it but Anthropic doesn't. But let's address it separately
| } | ||
| if (model != null) changeModel(model) |
There was a problem hiding this comment.
This function would change the model and won't restore it back, which is not communicated by the function name. AFAIU it is only an internal utility function for subgraphs, that is used only in one place. So to avoid confusion maybe we can rename it, or, even better, remove it altogether and embed in the usage site?
… prompt.withUpdatedParams and prompt.withUpdatedMessages functions
tiginamaria
force-pushed
the
vbr/prompt-api-improvements
branch
from
58610aa to
8ecf792
Compare
No description provided.