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

Fixing issues in the Jupyter notebook for the planner #873

Merged
merged 21 commits into from
May 10, 2023
Merged

Conversation

kaza
Copy link
Contributor

@kaza kaza commented May 9, 2023

Motivation and Context

This change fixes the issue in dotnet Jupyternotebooks, for the planner examples
It useses different classes for the planer, and in addition it fixes few smaller issues regarding configuration and presenting of the resultes

Description

  1. Changed the used nuget version
  2. Changed the old PlannerSkill class to SequentialPlanner
  3. local config files, for azure and open AI, both must have all the properties as otherwise Json deserializer throws an exception, as I used manually changed config files, had issue in configuring both of them, so I added missing json properties
  4. in the Planner originalPlan.Variables.ToPlan().PlanString has been removed so I created a new method in Utils and used that one

@github-actions github-actions bot added .NET Issue or Pull requests regarding .NET code samples labels May 9, 2023
@kaza
Copy link
Contributor Author

kaza commented May 9, 2023

@kaza please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree

@alexchaomander
Copy link
Contributor

Thank you so much for creating this! This notebook needed to get updated to reflect the latest planner refactor. One comment from my side, for this particular example, could you show the output of the cells in the Jupyter notebook? Since it's a popular notebook that a lot of people like to look at (even on the Github website), it would be good to see what a "plan" looks like.

Thanks again!

@lemillermicrosoft @RogerBarreto can you take a look?

@kaza
Copy link
Contributor Author

kaza commented May 9, 2023

@alexchaomander I included the plans.
if you want I can create additonal paragraph for an additonal plan, and I can create this in another PR if you want.

@dluc dluc self-requested a review May 9, 2023 16:24
@dluc dluc self-assigned this May 9, 2023
@dluc
Copy link
Collaborator

dluc commented May 9, 2023

@kaza thanks for the PR, I'll leave some comments

@dluc
Copy link
Collaborator

dluc commented May 9, 2023

@kaza could you allow us to edit the PR of minor adjustments before merging?

also, could you "Clear All Outputs" ?

image

I'd be glad to do it but we cannot edit the PR

@kaza
Copy link
Contributor Author

kaza commented May 9, 2023

I cleared all the outputs hover previously I was asked to inoclude them "could you show the output of the cells in the Jupyter notebook?".
If you want I can generate them and include them again :)

@dluc
Copy link
Collaborator

dluc commented May 9, 2023

sorry for the churn @kaza -- yes we like showing the output but we cherry pick which cell, so rather than asking you which one and giving complex instructions I thought it would be easier to just drop the output, and we'll adjust it afterwards during the release process

@dluc
Copy link
Collaborator

dluc commented May 9, 2023

I would remove the custom serialization logic before merging, and leverage .NET JSON serializer + the fact that the Plan data structure is serializable

samples/notebooks/dotnet/05-using-the-planner.ipynb Outdated Show resolved Hide resolved
samples/notebooks/dotnet/05-using-the-planner.ipynb Outdated Show resolved Hide resolved
samples/notebooks/dotnet/05-using-the-planner.ipynb Outdated Show resolved Hide resolved
samples/notebooks/dotnet/05-using-the-planner.ipynb Outdated Show resolved Hide resolved
@dluc dluc merged commit 7406233 into microsoft:main May 10, 2023
shawncal pushed a commit to johnoliver/semantic-kernel that referenced this pull request May 19, 2023
### Motivation and Context

This change fixes the issue in dotnet Jupyternotebooks, for the planner examples
It useses different classes for the planer, and in addition it fixes few smaller issues regarding configuration and presenting of the resultes

### Description

1)  Changed the  used nuget version
2)  Changed  the old PlannerSkill class to SequentialPlanner
3)  local config files, for azure and open AI, both must have all the properties as otherwise Json deserializer throws an exception, as I used manually changed config files, had issue in configuring both of them, so I added missing json properties
4) in the Planner originalPlan.Variables.ToPlan().PlanString has been removed so I created a new method in Utils and used that one
dehoward pushed a commit to lemillermicrosoft/semantic-kernel that referenced this pull request Jun 1, 2023
### Motivation and Context

This change fixes the issue in dotnet Jupyternotebooks, for the planner examples
It useses different classes for the planer, and in addition it fixes few smaller issues regarding configuration and presenting of the resultes

### Description

1)  Changed the  used nuget version
2)  Changed  the old PlannerSkill class to SequentialPlanner
3)  local config files, for azure and open AI, both must have all the properties as otherwise Json deserializer throws an exception, as I used manually changed config files, had issue in configuring both of them, so I added missing json properties
4) in the Planner originalPlan.Variables.ToPlan().PlanString has been removed so I created a new method in Utils and used that one
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
None yet
Development

Successfully merging this pull request may close these issues.

4 participants