Skip to content

Fix import_semantic_skill_from_directory#236

Merged
dluc merged 5 commits into
microsoft:python-previewfrom
renrut:python-preview
Apr 2, 2023
Merged

Fix import_semantic_skill_from_directory#236
dluc merged 5 commits into
microsoft:python-previewfrom
renrut:python-preview

Conversation

@renrut

@renrut renrut commented Mar 30, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Motivation and Context

This PR fixes #235 and adds a test for the functionality. This is ahead of the planning skill.

Description

  1. Fix to fix the typo in the from_dict() method in prompt_template_config.py
  2. Add assignment back to skill config so results from JSON aren't discarded in import_semantic_skill_from_directory.py
  3. Add tests

Contribution Checklist

@renrut

renrut commented Mar 30, 2023

Copy link
Copy Markdown
Contributor Author

@microsoft-github-policy-service agree

@renrut renrut changed the title Python preview Fix import_semantic_skill_from_directory Mar 30, 2023
@alexchaomander alexchaomander added the python Pull requests for the Python Semantic Kernel label Mar 30, 2023
dluc
dluc reviewed Mar 30, 2023
View reviewed changes
Comment thread python/tests/test_config.py
@dluc

dluc commented Mar 30, 2023

Copy link
Copy Markdown
Contributor

@jjhenkel could you take a look?

@renrut

renrut commented Mar 31, 2023

Copy link
Copy Markdown
Contributor Author

So I also found an issue in the kernel.py here in the same code path. If the prompt_template_config.from_dict() doesn't parse out a default_backend from the config.json, it will result in a NoneObject and not an empty list and the kernel assumes empty list.

I can go ahead and fix it with a revision in this PR since it's fairly tangential to this change or open another?

Option 1:
change kernel to check for None before trying to get length of the default_backend

function_config.prompt_template_config.default_backends if (function_config.prompt_template_config.default_backends and len(function_config.prompt_template_config.default_backends) > 0) else None

Option 2 (preferred):
change prompt_template_config from_dict parsing to default to empty list. I'd probably go with this option...

config.default_backends = data.get("default_backends", [])

@jjhenkel

jjhenkel commented Mar 31, 2023

Copy link
Copy Markdown

Thanks for all this @renrut, I agree w/ Option 2 and will take a look at this PR when that's in (looks great now, don't anticipate any changes).

This was a path I never tested in the original port (loading skills from a directory) so I really appreciate your help here! Thanks for the insights, the fixes, and generally looking through things and finding what's wrong!!

@renrut

renrut commented Mar 31, 2023

Copy link
Copy Markdown
Contributor Author

@jjhenkel no worries it's a lot of code to port over! Happy to fix. Change should be present now.

@alexchaomander alexchaomander self-requested a review April 1, 2023 00:36
alexchaomander
alexchaomander approved these changes Apr 1, 2023
View reviewed changes

@alexchaomander alexchaomander left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me! Thanks for adding these changes!

jjhenkel
jjhenkel approved these changes Apr 1, 2023
View reviewed changes

@jjhenkel jjhenkel left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looks good to me, thanks again! (Sorry for the delay here, thanks for that additional fix!)

@alexchaomander alexchaomander added the PR: ready to merge PR has been approved by all reviewers, and is ready to merge. label Apr 2, 2023
@renrut

renrut commented Apr 2, 2023

Copy link
Copy Markdown
Contributor Author

Sweet. I'm not authorized to merge, so I assume one of you all will.

@dluc dluc merged commit c3c89ea into microsoft:python-preview Apr 2, 2023
@renrut renrut mentioned this pull request Apr 4, 2023
Closed
dluc pushed a commit that referenced this pull request Apr 12, 2023
@renrut @dluc
Fix import_semantic_skill_from_directory (#236)
08c59d6 
### Motivation and Context
This PR fixes #235 and adds a test for the functionality. This is ahead
of the planning skill.

### Description
1. Fix to fix the typo in the `from_dict()` method in
`prompt_template_config.py`
2. Add assignment back to skill config so results from JSON aren't
discarded in `import_semantic_skill_from_directory.py`
3. Add tests
dluc pushed a commit that referenced this pull request Apr 13, 2023
@renrut @dluc
Fix import_semantic_skill_from_directory (#236)
64ffd2e 
### Motivation and Context
This PR fixes #235 and adds a test for the functionality. This is ahead
of the planning skill.

### Description
1. Fix to fix the typo in the `from_dict()` method in
`prompt_template_config.py`
2. Add assignment back to skill config so results from JSON aren't
discarded in `import_semantic_skill_from_directory.py`
3. Add tests
dluc pushed a commit that referenced this pull request Apr 13, 2023
@renrut @dluc
Fix import_semantic_skill_from_directory (#236)
db1fae3 
### Motivation and Context
This PR fixes #235 and adds a test for the functionality. This is ahead
of the planning skill.

### Description
1. Fix to fix the typo in the `from_dict()` method in
`prompt_template_config.py`
2. Add assignment back to skill config so results from JSON aren't
discarded in `import_semantic_skill_from_directory.py`
3. Add tests
dehoward pushed a commit to lemillermicrosoft/semantic-kernel that referenced this pull request Jun 1, 2023
@renrut @dluc
Fix import_semantic_skill_from_directory (microsoft#236)
3c31437 
### Motivation and Context
This PR fixes microsoft#235 and adds a test for the functionality. This is ahead
of the planning skill.

### Description
1. Fix to fix the typo in the `from_dict()` method in
`prompt_template_config.py`
2. Add assignment back to skill config so results from JSON aren't
discarded in `import_semantic_skill_from_directory.py`
3. Add tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dluc dluc dluc approved these changes

+2 more reviewers

@alexchaomander alexchaomander alexchaomander approved these changes

@jjhenkel jjhenkel jjhenkel approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@renrut @dluc @jjhenkel @alexchaomander