-
Notifications
You must be signed in to change notification settings - Fork 15.2k
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
prompty: adding Microsoft langchain_prompty package #21346
prompty: adding Microsoft langchain_prompty package #21346
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
@efriis Hi, I know there are some conversations going on on the business side. But could please you give me some review to help me better prepare this PR so that we can merge it as soon as we made a deal? |
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.
Hey team! I don't think this is doing what we discussed. Wasn't the plan to reconstruct a prompty spec as a prompt
or prompt | chat_model
runnable in the langchain runtime?
This seems to be doing a great deal of bespoke converting to and from some different format.
@@ -0,0 +1 @@ | |||
from .langchain import * |
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.
switch to explicit import, define __all__
, and add newline
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 fiexed
from typing import Dict | ||
from .utils import load, prepare | ||
|
||
def create_chat_prompt(path: str, input_name_agent_scratchpad = "agent_scratchpad") -> Runnable[Dict[str, ChatPromptTemplate], 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.
Why does the returned runnable take a dictionary mapping str -> ChatPromptTemplate as input?
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.
my bad, typo here, it should be
Runnable[Dict[str, any], ChatPromptTemplate]
I'm basically constructing a ChatPromptTemplate using input and prompty. Prompty will render and parse based on user inputs.
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.
What would a user pass in that's different than what they'd pass into a ChatPromptTemplate? Would it be possible to have it return a ChatPromptTemplate instead of a RunnableLambda that returns a ChatPromptTemplate?
@@ -0,0 +1,26 @@ | |||
from pydantic import BaseModel | |||
from .core import Invoker, InvokerFactory, Prompty, SimpleModel | |||
from jinja2 import DictLoader, Environment |
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.
cc @eyurtsev - will this trigger the same vulnerabilities in Jinja2 as we've seen in the past?
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.
@efriis is langchain removing jinja for all?
happy to support additional formats too as discussed.
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.
In general, I think it would make the most sense for the LangChain runtime for prompty to support the prompt template styles that langchain supports, and not introduce a new one just for prompty.
We removed jinja2 because of code sandboxing concerns when reading from files (similar to this). See #10252 for more details. See this example for injecting for RCE: https://github.com/langchain-ai/langchain/pull/10252/files#diff-a0bd39be3ca1018bba7dcb089c148cb0083fddbf7eb95d374bf1d86ecfcd1093
For the initial release, I would recommend supporting f-strings and mustache by default, so the jinja2 vulnerabilities don't attract CVEs. In general, it's very difficult to guarantee that people are only loading .prompty
files that are static/controlled by the developer, and if you want to proceed with Jinja2, I'll probably recommend some updates to docstrings and documentation to reflect the danger of loading anything user-defined with these functions.
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.
Thanks @efriis, I fully agree with you. Judging on the timeline, here's my proposal, let me know what you think:
- We will start with Jinja2, since that's mostly where people are using today. And like you said, will update docstrings and doc to reflect the danger of it.
- Start supporting mustache by default, we also need to coordinate with Microsoft orchestration frameworks to support those.
Does this sound good?
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.
Let me chat with the team and get back to you. Because this package has our name on it, I am hesitant to merge something in that introduces the same vulnerability as this 9.8/10 CVE from last year that caused us to strip out Jinja2 in the first place: GHSA-7gfq-f96f-g85j
If there's any way to switch to Mustache for launch (pretty straightforward, and similar syntax to Jinja. Support in most languages.), that would be make releasing this much easier.
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.
Ok how about this:
- insert "danger arbitrary code execution" warnings into docstrings, readme, and any docs (to prevent people misusing and parsing user input or anything from an unsandboxed filesystem)
- moving to a partner repo (langchain-ai/langchain-prompty) so github doesn't flag security in this monorepo if CVEs get filed for it
- publish from partner repo
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.
@efriis we discussed internally a little bit, here's the initial thinking:
- In our "template.type" field, we will add "mustache" support. In our documentation, we will suggest user to use mustache instead of jinja.
- In this langchain-prompty package, we will support mustache by default if that option isn't set by user.
- In our langchain template samples, we will also use mustache.
- But if user sets type to jinja explicitly, it'll use jinja.
- In our VSCode extension, we would support mustache if user sets that "type" field. But by default still support jinja, until other Microsoft frameworks support mustache (we need some time to coordinate).
So basically, all langchain interface are mustache, except .4 if user explicitly specifies it. Or do you think we should delete 4 too? I'm ok to delete 4 but there would be some interoperability issues for the initial period.
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.
also is there any discussion somewhere comparing different options of templating engine? Just curious why the consensus is mustache, vs others? For example, mustache can't handle dictionary object well, handlebar seems easier to use, which extends from mustache
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.
@efriis we also handle security pretty seriously in Microsoft. So I've changed to support only mustache. I think it does has its merits by being simple, less capability but more secure. And probably mostly enough for prompt workload already. Can you take a look and see if this is ready to merge?
@efriis The experience in this proposal is
Any suggestion on how it should look like? |
yes! The experience should ideally be:
|
So my understanding is you are proposing the same code experience, but the return type is ChatPromptTemplate, right? I actually started with that and switched to the Runnable<input, ChatPromptTemplate> approach. Prompty's main value proposition is offering a serialized and easy to write prompty format. Basically Prompy+Input => Messages We handle rendering (expand the template) + parsing (convert to Message list that LLM can take). In your suggestion, I just converted it to ChatPromptTemplate once in the beginning, then I'm losing the ability to render again based on different inputs. Langchain has a special MessagesPlaceholder, but we actually give the freedom back to user. For example
@efriis I hope this make sense? Maybe there are other ways that I don't know. Would love to hear your thoughts. |
Is the distinction that you want to expand the Jinja2 template before parsing it as messages? Such that a user could wrap a for loop that ends up generating multiple messages? You can actually do what you described in normal ChatPromptTemplate as long as it's either
|
The main features you miss out on by returning a custom runnable as opposed to a
|
Can you give me an example of this?
|
|
https://smith.langchain.com/prompts/eyfriis/mustache-example And yes to partial and LangSmith tracing as well as playground as benefits of using the prompt format within the LangChain runtime! For example, you could explore traces that use prompty prompts like the run of one above: https://smith.langchain.com/public/dc6657c1-801f-496b-9e99-b27d3b1d976d/r |
In your example above, looks like it's not populating multiple message? It's still the same single message with user role but just the content is looped. @efriis, but I feel this is something we could iterate on though, since the code user write isn't different between our proposals. So it could be a seamless update later too. What do you think? |
{{#chat_history}} | ||
{{role}}: | ||
{{content}} | ||
{{/chat_history}} |
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.
{{#chat_history}} | |
{{role}}: | |
{{content}} | |
{{/chat_history}} | |
placeholder: | |
{{chat_history}} |
Would you consider something like this for chat history in the langchain runtime? If so, we can get rid of the custom parsing and interpret this as a MessagesPlaceholder
object, which does the same thing as this.
I'm not sure it makes sense to generate mixed sequence messages and iterated contents of messages because it's typically one or the other. Are you imagining a case where users want to prompty-format a generic list of inputs into multiple messages, instead of specifically
role:
content
blocks?
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 reason I ask is because chat histories in the langchain runtime will typically be passed in as a List[BaseMessage]
instead of List[Dict]
where dict matches the format {"role": x, "content": y}
This is rendered well by MessagesPlaceholder
, but would not be rendered well for other messages cases (e.g. ones with tool_calls
or ToolMessage
).
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.
You mean using placeholder as a special keyword in prompty? We want prompty to be usable across orchestration framework though (Promptflow, SemanticKernel etc), so I'm reluctant to add framework specific keywords. Maybe we can add a special langchain parser to support that later? But for //build we want to stick to a more generic approach.
That's also the main reason why I'm not ready to switch from returning Runnable<> to ChatTemplate. Coz then I would need something special in prompty to indicate it should be swapped to MessagesPlaceholder. And that concept doesn't exist in other places we want to support.
|
||
def invoke(self, data: BaseModel) -> BaseModel: | ||
assert isinstance(data, SimpleModel) | ||
generated = chevron.render(self.prompty.content, data.item) |
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 will actually be a security issue too because of chevron partials. Would be better if you just constructed a ChatPromptTemplate
with type as mustache!
As an alternative, you can also use the partial-free renderer in langchain_core
that mustache ChatPromptTemplate
s rely on https://github.com/langchain-ai/langchain/pull/19980/files#diff-c591786bc4be9ea7fab44200ea051b2e29ad2cda359a8cd3577eac4d0b2b0c7bR386
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.
ah nice! Didn't know you have this, switching to your partial free renderer now
No description provided.