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: Refactoring and fix bug. AuthorRole instead of string literals. #6391

Merged

Conversation

stefan521
Copy link
Contributor

@stefan521 stefan521 commented May 24, 2024

Motivation and Context

There's an enum called AuthorRole. I replaced string literals with the enum values. PyCharm highlights this in some cases: Expected type 'AuthorRole', got 'str' instead

I also fixed a bug in function_invocation_filters_stream.py where the argument name was wrong. author="assistant" instead of role="assistant"

Description

Contribution Checklist

@stefan521 stefan521 requested a review from a team as a code owner May 24, 2024 20:13
@markwallace-microsoft markwallace-microsoft added the python Pull requests for the Python Semantic Kernel label May 24, 2024
Copy link
Contributor

@moonbox3 moonbox3 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
Copy link
Contributor

Hi @stefan521, could you resolve the one file conflict, please? Thanks!

# Conflicts:
#	python/semantic_kernel/connectors/ai/ollama/services/ollama_chat_completion.py
@markwallace-microsoft
Copy link
Member

markwallace-microsoft commented May 31, 2024

Py3.10 Test Coverage

Python 3.10 Test Coverage Report •
FileStmtsMissCoverMissing
semantic_kernel/connectors/ai/ollama/services
   ollama_chat_completion.py67691%56, 94, 116, 133, 167, 188
semantic_kernel/connectors/ai/open_ai/services
   open_ai_chat_completion_base.py2199656%97, 101, 121, 146–150, 174, 178, 182, 198–203, 220–248, 251–262, 277–284, 295–303, 319–326, 347, 355, 361–364, 376–379, 410, 449, 454, 459–464, 468–475, 479–491, 513, 523–534
semantic_kernel/contents
   function_result_content.py571279%50, 55, 66, 81, 93, 107, 112, 119–123
TOTAL612571688% 

Python 3.10 Unit Test Overview

Tests Skipped Failures Errors Time
1433 1 💤 0 ❌ 0 🔥 23.846s ⏱️

@stefan521
Copy link
Contributor Author

Hi @moonbox3 , thanks for reviewing! I've solved the conflicts.

auto-merge was automatically disabled June 3, 2024 12:17

Head branch was pushed to by a user without write access

@eavanvalkenburg
Copy link
Member

@stefan521 sorry, I think I wasn't clear enough, I like that most tests run with AuthorRole... but there need to be some with a str (one per type) so that we know both ways of creating them work!

@stefan521
Copy link
Contributor Author

@eavanvalkenburg got it! I’ll update the PR tonight. Thanks for reviewing!

@eavanvalkenburg eavanvalkenburg added this pull request to the merge queue Jun 4, 2024
Merged via the queue into microsoft:main with commit 8828b86 Jun 4, 2024
23 checks passed
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
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants