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: issue-10278 : Change ChatPromptParser to enable 0-n text part instead of single value #10304

Merged
merged 3 commits into from
Jan 28, 2025

Conversation

ThDuquennoy
Copy link
Contributor

Motivation and Context

See Issue #10278

Description

ChatPromptParser.cs :
Remove method IsValidChildNodes and its call in IsValidChatMessage : A chat message is valid as long as it has a role attribute. Messages with no text child or multiple text children are now valid

ChatPromptParserTests :

  • Changed 3rd invalid example since the former one is now valid
  • Added tests for : Message with multiple text nodes, mixed XML content and empty XML node
    Remark : The expected behavior for mixed XML content is unclear so I kept it as it was : the content of the message node ends up in a TextContent if and only if the message has no valid text or image child node.
    So for instance, if the prompt has a message that is a mixed XML with content and a child image node, the content would be ignored and the ChatMessageContent object will have only an ImageContent item

Other remark :
ChatMessageContent.Content property only returns/sets the first TextContent item.
I thought about changing it to :

  • get : return a concatenation of the TextContent items separated by \n
  • set : set the first TextContent element (or add one if there is none) and remove other TextContent items

But its current behavior seems intended (it is even included in some unit tests) and I felt like such a change would have too much impact across the code. So I left it as it is. But I think that such a change could be beneficial.

Contribution Checklist

  • The code builds clean without any errors or warnings
  • The PR follows the SK Contribution Guidelines and the pre-submission formatting script raises no violations
  • All unit tests pass, and I have added new tests where possible :
    I have the same test fails with the unmodified main branch. For instance the test GettingStarted/Step1_Create_Kernel fails with ConfigurationNotFoundException : Configuration section 'OpenAI' not found. I think there are some missing config files
  • I didn't break anyone 😄

@ThDuquennoy ThDuquennoy requested a review from a team as a code owner January 27, 2025 18:00
@markwallace-microsoft markwallace-microsoft added .NET Issue or Pull requests regarding .NET code kernel Issues or pull requests impacting the core kernel kernel.core labels Jan 27, 2025
@github-actions github-actions bot changed the title issue-10278 : Change ChatPromptParser to enable 0-n text part instead of single value .Net: issue-10278 : Change ChatPromptParser to enable 0-n text part instead of single value Jan 27, 2025
@ThDuquennoy
Copy link
Contributor Author

@microsoft-github-policy-service agree company="Chapsvision"

@dmytrostruk dmytrostruk enabled auto-merge January 28, 2025 19:54
@dmytrostruk dmytrostruk added this pull request to the merge queue Jan 28, 2025
Merged via the queue into microsoft:main with commit 89cd872 Jan 28, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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