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: fix: add "status" field to Python dynamic session response #9903

Merged
merged 6 commits into from
Dec 12, 2024

Conversation

malandis
Copy link
Contributor

@malandis malandis commented Dec 7, 2024

Motivation and Context

Addresses issue #9902 for .NET.

Include the "status" field in the response string returned by Python dynamic sessions. Fixes a usability issue where the LLM assumes success despite execution failures.

Description

As per #9902, including the status property from the response in the plugin result, we ensure that the LLM has explicit information about whether the execution succeeded or failed, preventing misinterpretation of stderr or other response elements.

This helps avoid hallucinated follow-ups or unnecessary retries by providing clear success/failure indicators.

Contribution Checklist

Include the "status" field in the response string returned by Python
dynamic sessions.

This ensures that the LLM has explicit information about whether the
execution succeeded or failed, preventing misinterpretation of stderr
or other response elements.

This helps avoid hallucinated follow-ups or unnecessary retries by
providing clear success/failure indicators.

Fixes a usability issue where the LLM assumes success despite
execution failures.
@malandis malandis requested a review from a team as a code owner December 7, 2024 17:13
@markwallace-microsoft markwallace-microsoft added the .NET Issue or Pull requests regarding .NET code label Dec 7, 2024
@github-actions github-actions bot changed the title fix: add "status" field to Python dynamic session response .Net: fix: add "status" field to Python dynamic session response Dec 7, 2024
Copy link
Member

@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.

@malandis the unit tests are failing with this change, can you take a look.

@malandis
Copy link
Contributor Author

malandis commented Dec 9, 2024

@markwallace-microsoft absolutely -- done in 349e2d8

@malandis
Copy link
Contributor Author

malandis commented Dec 11, 2024

Thanks @dmytrostruk .

@markwallace-microsoft looks good to go. Just need an additional +1 and to get the branch up to date (if that’s needed before merging).

@moonbox3 moonbox3 enabled auto-merge December 12, 2024 00:27
@moonbox3 moonbox3 added this pull request to the merge queue Dec 12, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 12, 2024
@moonbox3 moonbox3 added this pull request to the merge queue Dec 12, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 12, 2024
@moonbox3 moonbox3 added this pull request to the merge queue Dec 12, 2024
github-merge-queue bot pushed a commit that referenced this pull request Dec 12, 2024
### Motivation and Context

Addresses issue #9902 for .NET.

Include the "status" field in the response string returned by Python
dynamic sessions. Fixes a usability issue where the LLM assumes success
despite execution failures.


### Description

As per #9902, including the `status` property from the response in the
plugin result, we ensure that the LLM has explicit information about
whether the execution succeeded or failed, preventing misinterpretation
of stderr or other response elements.

This helps avoid hallucinated follow-ups or unnecessary retries by
providing clear success/failure indicators.


### 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://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md)
and the [pre-submission formatting
script](https://github.com/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: Evan Mattson <[email protected]>
Merged via the queue into microsoft:main with commit b40e0cc Dec 12, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
.NET Issue or Pull requests regarding .NET code
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants