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

Python: Fix schema building for complex types #6394

Merged
merged 12 commits into from
May 28, 2024

Conversation

moonbox3
Copy link
Contributor

Motivation and Context

The schema building for complex types worked in some regard, but it didn't work for all cases. There needs to be better checks on complex types, if they have args and if so, recurse to the base class type, and also grab the type that is specified in the list like list[CustomClass], list[str] or even Union[int, bool].

Description

This PR improves the parsing/building of complex types. Added integration tests to cover more types in the hope that this doesn't lead to any more issues.

Contribution Checklist

@moonbox3 moonbox3 requested a review from a team as a code owner May 24, 2024 23:52
@markwallace-microsoft markwallace-microsoft added the python Pull requests for the Python Semantic Kernel label May 24, 2024
@markwallace-microsoft
Copy link
Member

markwallace-microsoft commented May 24, 2024

Py3.10 Test Coverage

Python 3.10 Test Coverage Report •
FileStmtsMissCoverMissing
TOTAL609687986% 
report-only-changed-files is enabled. No files were changed during this commit :)

Python 3.10 Unit Test Overview

Tests Skipped Failures Errors Time
1374 1 💤 0 ❌ 0 🔥 15.974s ⏱️

Copy link
Member

@eavanvalkenburg eavanvalkenburg left a comment

Choose a reason for hiding this comment

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

Mostly testing, but forward refs vs regular definition can be a pain to deal with!

python/tests/unit/schema/test_schema_builder.py Outdated Show resolved Hide resolved
Copy link
Member

@eavanvalkenburg eavanvalkenburg left a comment

Choose a reason for hiding this comment

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

LGTM

@moonbox3 moonbox3 enabled auto-merge May 28, 2024 18:03
@moonbox3 moonbox3 added this pull request to the merge queue May 28, 2024
Merged via the queue into microsoft:main with commit 895a580 May 28, 2024
23 checks passed
@moonbox3 moonbox3 deleted the parse_schema_fix branch May 28, 2024 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python Pull requests for the Python Semantic Kernel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Python: In v1.0.2 it's not possible to have lists or dictionaries as fields of a KernelBaseModel
4 participants