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

.Net: Merge the Prompty feature branch to main #6097

Merged
merged 18 commits into from
May 8, 2024

Conversation

markwallace-microsoft
Copy link
Member

Motivation and Context

Description

Contribution Checklist

LittleLittleCloud and others added 7 commits April 24, 2024 19:26
### Motivation and Context

### Description

<!-- Describe your changes, the overall approach, the underlying design.
These notes will help understanding how your code works. Thanks! -->

This PR adds the following empty projects for the ongoing prompty
intergration
- Functions.Prompty (Experiment)
- Functions.Prompty.UnitTests
- PromptTemplates.Liquid (Experiment)
- PromptTemplates.UnitTests

### Contribution Checklist

<!-- Before submitting this PR, please make sure: -->

- [ ] The code builds clean without any errors or warnings
- [x] The PR follows the [SK Contribution
Guidelines](https:/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md)
and the [pre-submission formatting
script](https:/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md#development-scripts)
raises no violations
- [x] All unit tests pass, and I have added new tests where possible
- [x] I didn't break anyone 😄
### Motivation and Context

<!-- Thank you for your contribution to the semantic-kernel repo!
Please help reviewers and future users, providing the following
information:
  1. Why is this change required?
  2. What problem does it solve?
  3. What scenario does it contribute to?
  4. If it fixes an open issue, please link to the issue here.
-->

### Description

<!-- Describe your changes, the overall approach, the underlying design.
These notes will help understanding how your code works. Thanks! -->

This PR brings markdown prompt template support for semantic kernel. It
essentially does a code-level copy of the original markdown prompt
template implementation to the sk repo, plus an extension API to execute
the template.

~Because the markdown prompt template support is still in an very, very
early stage. All code is put under `Experimental` namespace and all the
class except the extension API is marked as internal only.~

You can find the spec for the markdown prompt template
[here](https:/Azure/azureml_run_specification/blob/master/schemas/Prompty.yaml)

The intergration comes with two projects
- PromptTemplates.Liquid: liquid syntax support for the markdown prompt
template, which renders liquid-like template into the chat format that
can be processed by `ChatPromptParser`
- Function.Prompty: load and create `KernelFunctionFromPrompt` from
prompty file via `CreateFunctionFromPrompty` API

The tool call support will come in the next PR as this PR is already
growing large. Also tool call support needs some extra care which might
need further discussion on how to implement cohere to sk pattern
### Contribution Checklist

<!-- Before submitting this PR, please make sure: -->

- [ ] The code builds clean without any errors or warnings
- [x] The PR follows the [SK Contribution
Guidelines](https:/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md)
and the [pre-submission formatting
script](https:/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md#development-scripts)
raises no violations
- [x] All unit tests pass, and I have added new tests where possible
- [x] I didn't break anyone 😄

---------

Co-authored-by: Cassie Breviu <[email protected]>
Co-authored-by: Stephen Toub <[email protected]>
### Motivation and Context

<!-- Thank you for your contribution to the semantic-kernel repo!
Please help reviewers and future users, providing the following
information:
  1. Why is this change required?
  2. What problem does it solve?
  3. What scenario does it contribute to?
  4. If it fixes an open issue, please link to the issue here.
-->

### Description
#6033 
<!-- Describe your changes, the overall approach, the underlying design.
These notes will help understanding how your code works. Thanks! -->

### Contribution Checklist

<!-- Before submitting this PR, please make sure: -->

- [ ] The code builds clean without any errors or warnings
- [ ] The PR follows the [SK Contribution
Guidelines](https:/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md)
and the [pre-submission formatting
script](https:/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md#development-scripts)
raises no violations
- [ ] All unit tests pass, and I have added new tests where possible
- [ ] I didn't break anyone 😄
@markwallace-microsoft markwallace-microsoft requested a review from a team as a code owner May 2, 2024 08:15
@markwallace-microsoft markwallace-microsoft added .NET Issue or Pull requests regarding .NET code kernel Issues or pull requests impacting the core kernel documentation labels May 2, 2024
@github-actions github-actions bot changed the title Merge the Prompty feature branch to main .Net: Merge the Prompty feature branch to main May 2, 2024
markwallace-microsoft and others added 6 commits May 3, 2024 06:50
### Motivation and Context

`PromptyKernelExtensions.CreateFunctionFromPrompty`

The second parameter is a file path to a prompty file. This should be
changed to the prompty text. Doing this will allow developers to be
responsible for loading the prompty file e.g. could be a file or an
embedded resource or a database record...

### Description

<!-- Describe your changes, the overall approach, the underlying design.
These notes will help understanding how your code works. Thanks! -->

### Contribution Checklist

<!-- Before submitting this PR, please make sure: -->

- [ ] The code builds clean without any errors or warnings
- [ ] The PR follows the [SK Contribution
Guidelines](https:/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md)
and the [pre-submission formatting
script](https:/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md#development-scripts)
raises no violations
- [ ] All unit tests pass, and I have added new tests where possible
- [ ] I didn't break anyone 😄

---------

Co-authored-by: Stephen Toub <[email protected]>
Co-authored-by: Dmytro Struk <[email protected]>
### Motivation and Context

<!-- Thank you for your contribution to the semantic-kernel repo!
Please help reviewers and future users, providing the following
information:
  1. Why is this change required?
  2. What problem does it solve?
  3. What scenario does it contribute to?
  4. If it fixes an open issue, please link to the issue here.
-->

### Description

<!-- Describe your changes, the overall approach, the underlying design.
These notes will help understanding how your code works. Thanks! -->

### Contribution Checklist

<!-- Before submitting this PR, please make sure: -->

- [ ] The code builds clean without any errors or warnings
- [ ] The PR follows the [SK Contribution
Guidelines](https:/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md)
and the [pre-submission formatting
script](https:/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md#development-scripts)
raises no violations
- [ ] All unit tests pass, and I have added new tests where possible
- [ ] I didn't break anyone 😄
…xtensions (#6118)

- Liquid template parsing should happen during construction, not on each
render
- Liquid prompt template construction should fail for invalid templates
- Default inputs should be evaluated once at Liquid template
construction time
- RenderAsync should capture any exceptions into returned Task
- Role regex used in parsing rendered messages should be Compiled
- LiquidPromptTemplateFactory should do arg validation and accomodate a
PromptTemplateConfig whose TemplateFormat is null
- Use XML comments instead of normal comments to describe properties in
internal DOM
- Remove unnecessary empty primary constructor
- Use a regex to parse the components of a prompty template in order to
a) more strictly validate contents but more importantly b) avoid losing
part of the template when the separator appears in the contents itself
- Clean up some XML comments
- Set ModelId appropriately for openai
- Avoid storing temperature/top_p in execution settings if they weren't
specified
- Add an OutputVariable if the prompty specifies one
- Cache the default template factory rather than creating a new one on
each construction

cc: @LittleLittleCloud
)

### Motivation and Context

<!-- Thank you for your contribution to the semantic-kernel repo!
Please help reviewers and future users, providing the following
information:
  1. Why is this change required?
  2. What problem does it solve?
  3. What scenario does it contribute to?
  4. If it fixes an open issue, please link to the issue here.
-->
#6030 
### Description
~In this implementation, the `Ġ` will be reserved in liquid template
which is used to replace `:` in all input variables when unsafe content
is not allowed.~

~The encoding process for input variables when unsafe content is not
allowed is~
~- replace all `:` to `Ġ` // this is the extra step comparing with
HandlerBar Template~
~- Encode xml using `HttpUtility.HtmlEncode`~

~The decoding process is~
~- replace all `Ġ` to `:`~


This PR introduces a new process to mitigate potential prompt injection
attacks from input variables when using liquid templates. Here's a
breakdown of the steps:

Before rendering, each input variable undergoes a transformation: all
occurrences of `:`are replaced with `&#58;`. This ensures that message
tags like `system:`, `user:`, or `assistant:` are not present if
`AllowUnsafeContent` is set to `false`. No replacement occurs if
`AllowUnsafeContent` is `true`.
After rendering, each message content is processed based on the
`AllowUnsafeContent` setting. If it's `false`, all `&#58;` instances are
reverted back to `:`, followed by calling `html_encode` on each message
content. If `AllowUnsafeContent` is `true`, only `html_encode` is
called. This additional encoding step is necessary because
`ChatPromptParser` always decodes XML message content, requiring the
liquid template to undergo an extra encoding step to ensure the rendered
content matches the original before rendering.

<!-- Describe your changes, the overall approach, the underlying design.
These notes will help understanding how your code works. Thanks! -->

### Contribution Checklist

<!-- Before submitting this PR, please make sure: -->

- [x] The code builds clean without any errors or warnings
- [x] The PR follows the [SK Contribution
Guidelines](https:/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md)
and the [pre-submission formatting
script](https:/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md#development-scripts)
raises no violations
- [x] All unit tests pass, and I have added new tests where possible
- [x] I didn't break anyone 😄
Copy link
Member Author

@markwallace-microsoft markwallace-microsoft left a comment

Choose a reason for hiding this comment

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

I have reviewed and changes LGTM, there are two follow up tasks
#6164
#6165

@markwallace-microsoft markwallace-microsoft added this pull request to the merge queue May 8, 2024
Merged via the queue into main with commit 2ae9dc7 May 8, 2024
19 checks passed
@markwallace-microsoft markwallace-microsoft deleted the feature-prompty branch May 8, 2024 19:01
johnoliver pushed a commit to johnoliver/semantic-kernel that referenced this pull request Jun 5, 2024
### Motivation and Context

<!-- Thank you for your contribution to the semantic-kernel repo!
Please help reviewers and future users, providing the following
information:
  1. Why is this change required?
  2. What problem does it solve?
  3. What scenario does it contribute to?
  4. If it fixes an open issue, please link to the issue here.
-->

### Description

<!-- Describe your changes, the overall approach, the underlying design.
These notes will help understanding how your code works. Thanks! -->

### Contribution Checklist

<!-- Before submitting this PR, please make sure: -->

- [ ] The code builds clean without any errors or warnings
- [ ] The PR follows the [SK Contribution
Guidelines](https:/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md)
and the [pre-submission formatting
script](https:/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md#development-scripts)
raises no violations
- [ ] All unit tests pass, and I have added new tests where possible
- [ ] I didn't break anyone 😄

---------

Co-authored-by: Xiaoyun Zhang <[email protected]>
Co-authored-by: Cassie Breviu <[email protected]>
Co-authored-by: Stephen Toub <[email protected]>
Co-authored-by: Dmytro Struk <[email protected]>
johnoliver pushed a commit to johnoliver/semantic-kernel that referenced this pull request Jun 5, 2024
### Motivation and Context

<!-- Thank you for your contribution to the semantic-kernel repo!
Please help reviewers and future users, providing the following
information:
  1. Why is this change required?
  2. What problem does it solve?
  3. What scenario does it contribute to?
  4. If it fixes an open issue, please link to the issue here.
-->

### Description

<!-- Describe your changes, the overall approach, the underlying design.
These notes will help understanding how your code works. Thanks! -->

### Contribution Checklist

<!-- Before submitting this PR, please make sure: -->

- [ ] The code builds clean without any errors or warnings
- [ ] The PR follows the [SK Contribution
Guidelines](https:/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md)
and the [pre-submission formatting
script](https:/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md#development-scripts)
raises no violations
- [ ] All unit tests pass, and I have added new tests where possible
- [ ] I didn't break anyone 😄

---------

Co-authored-by: Xiaoyun Zhang <[email protected]>
Co-authored-by: Cassie Breviu <[email protected]>
Co-authored-by: Stephen Toub <[email protected]>
Co-authored-by: Dmytro Struk <[email protected]>
LudoCorporateShark pushed a commit to LudoCorporateShark/semantic-kernel that referenced this pull request Aug 25, 2024
### Motivation and Context

<!-- Thank you for your contribution to the semantic-kernel repo!
Please help reviewers and future users, providing the following
information:
  1. Why is this change required?
  2. What problem does it solve?
  3. What scenario does it contribute to?
  4. If it fixes an open issue, please link to the issue here.
-->

### Description

<!-- Describe your changes, the overall approach, the underlying design.
These notes will help understanding how your code works. Thanks! -->

### Contribution Checklist

<!-- Before submitting this PR, please make sure: -->

- [ ] The code builds clean without any errors or warnings
- [ ] The PR follows the [SK Contribution
Guidelines](https:/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md)
and the [pre-submission formatting
script](https:/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md#development-scripts)
raises no violations
- [ ] All unit tests pass, and I have added new tests where possible
- [ ] I didn't break anyone 😄

---------

Co-authored-by: Xiaoyun Zhang <[email protected]>
Co-authored-by: Cassie Breviu <[email protected]>
Co-authored-by: Stephen Toub <[email protected]>
Co-authored-by: Dmytro Struk <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation kernel.core kernel Issues or pull requests impacting the core kernel .NET Issue or Pull requests regarding .NET code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants