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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Python: make input parameter optional #2234

Conversation

joowon-dm-snu
Copy link
Contributor

Motivation and Context

solve #2233

Description

Contribution Checklist

@joowon-dm-snu joowon-dm-snu requested a review from a team as a code owner July 31, 2023 12:45
@shawncal shawncal added the python Pull requests for the Python Semantic Kernel label Jul 31, 2023
@shawncal shawncal changed the title make input parameter optional Python: make input parameter optional Jul 31, 2023
@shawncal shawncal requested a review from mkarle August 3, 2023 06:01
@shawncal
Copy link
Member

shawncal commented Aug 3, 2023

@mkarle Can you review please?

@joowon-dm-snu
Copy link
Contributor Author

@shawncal @mkarle

This change will help Planner works better as input was always inserted into prompt because Planner get function parameters by context.skills.get_functions_view() I found this while porting stepwise planner into python

Here's sample example
image
image

If this PR is merged,
image

@mkarle
Copy link
Contributor

mkarle commented Aug 3, 2023

I ran unit and integration tests as well as some samples and notebooks and didn't notice any regression.

@shawncal
Copy link
Member

shawncal commented Aug 4, 2023

@joowon-dm-snu: Your branch is locked, so we cannot rebase and merge. The failing tests you're seeing above are not related to your change -- if you grant repo admins permissions to change your PR branch, we can update and merge the PR. Thanks!

image

@shawncal shawncal added the PR: ready to merge PR has been approved by all reviewers, and is ready to merge. label Aug 4, 2023
@joowon-dm-snu
Copy link
Contributor Author

joowon-dm-snu commented Aug 4, 2023

@shawncal
i made this PR in organization repo, coxwave/semantic-kernel and here is the issue

The organization repo doesn't allow selecting the "Allow edits from maintainers" option. I've already made three PRs in that repo, so is it okay if I temporarily invite two people(@mkarle, @awharrison-28) to the coxwave/semantic-kernel?

From now on, I'll probably proceed with my personal account.

@joowon-dm-snu
Copy link
Contributor Author

@shawncal i made this PR in organization repo, coxwave/semantic-kernel and here is the issue

The organization repo doesn't allow selecting the "Allow edits from maintainers" option. I've already made three PRs in that repo, so is it okay if I temporarily invite two people(@mkarle, @awharrison-28) to the coxwave/semantic-kernel?

From now on, I'll probably proceed with my personal account.

@shawncal
I transfer repo to private. problem solved!
image

@awharrison-28 awharrison-28 added this pull request to the merge queue Aug 4, 2023
Merged via the queue into microsoft:main with commit 8aa133b Aug 4, 2023
27 checks passed
@awharrison-28 awharrison-28 linked an issue Aug 4, 2023 that may be closed by this pull request
SOE-YoungS pushed a commit to SOE-YoungS/semantic-kernel that referenced this pull request Nov 1, 2023
### 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.
-->
solve microsoft#2233 

### 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://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
- [ ] All unit tests pass, and I have added new tests where possible
- [ ] I didn't break anyone 馃槃

Co-authored-by: Abby Harrison <54643756+awharrison-28@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: ready to merge PR has been approved by all reviewers, and is ready to merge. python Pull requests for the Python Semantic Kernel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Python: native function always has 'input' parameter
4 participants