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: Update chat_gpt_api.py #5445

Closed
wants to merge 12 commits into from
Closed

Conversation

Sarfaraz021
Copy link

@Sarfaraz021 Sarfaraz021 commented Mar 12, 2024

there are some errors in chat.add_user_message/assistant_message. You cannot just make chat as a object of ChatHistory, instead make it as

chat_history = ChatHistory(system_message=system_message)

also define service_id and pass it into kernel.add_service: such as :

service_id = "chat-gpt"

kernel.add_service(
sk_oai.OpenAIChatCompletion(
service_id="chat-gpt", ai_model_id="gpt-3.5-turbo", api_key=api_key)
)

Motivation and Context

Description

Contribution Checklist

there are some errors in chat.add_user_message/assistant_message.
You cannot just make chat as a object of ChatHistory, instead make it as 

chat_history = ChatHistory(system_message=system_message)

also define service_id and pass it into kernel.add_service: such as :

service_id = "chat-gpt"

kernel.add_service(
    sk_oai.OpenAIChatCompletion(
        service_id="chat-gpt", ai_model_id="gpt-3.5-turbo", api_key=api_key)
)
@Sarfaraz021 Sarfaraz021 requested a review from a team as a code owner March 12, 2024 12:51
@markwallace-microsoft markwallace-microsoft added the python Pull requests for the Python Semantic Kernel label Mar 12, 2024
@github-actions github-actions bot changed the title Update chat_gpt_api.py Python: Update chat_gpt_api.py Mar 12, 2024
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.

thanks for doing this, some minor comments!

@Sarfaraz021
Copy link
Author

@Sarfaraz021 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 company="Xflow"

@Sarfaraz021
Copy link
Author

..

@moonbox3
Copy link
Contributor

@Sarfaraz021 Lint is failing due to: samples/kernel-syntax-examples/chat_gpt_api.py:8:5: F401 [*] semantic_kernel.connectors.ai.chat_completion_client_base.ChatCompletionClientBase imported but unused

@markwallace-microsoft
Copy link
Member

markwallace-microsoft commented Mar 12, 2024

Py3.8 Test Coverage

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

Python 3.8 Unit Test Overview

Tests Skipped Failures Errors Time
1205 11 💤 0 ❌ 0 🔥 21.403s ⏱️

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.

one small thing, really close!

python/samples/kernel-syntax-examples/chat_gpt_api.py Outdated Show resolved Hide resolved
@Sarfaraz021
Copy link
Author

@Sarfaraz021 Lint is failing due to: samples/kernel-syntax-examples/chat_gpt_api.py:8:5: F401 [*] semantic_kernel.connectors.ai.chat_completion_client_base.ChatCompletionClientBase imported but unused

@moonbox3 it's been used here according to the new commit suggested by @eavanvalkenburg -> settings = kernel.get_prompt_execution_settings_from_service_id("chat-gpt", ChatCompletionClientBase)

changing in service_id variable
@moonbox3
Copy link
Contributor

@Sarfaraz021 Lint is failing due to: samples/kernel-syntax-examples/chat_gpt_api.py:8:5: F401 [*] semantic_kernel.connectors.ai.chat_completion_client_base.ChatCompletionClientBase imported but unused

@moonbox3 it's been used here according to the new commit suggested by @eavanvalkenburg -> settings = kernel.get_prompt_execution_settings_from_service_id("chat-gpt", ChatCompletionClientBase)

I understand. If Lint says it's failing, then we need to look at why. There's no subjectivity involved. :)

@Sarfaraz021
Copy link
Author

@Sarfaraz021 Lint is failing due to: samples/kernel-syntax-examples/chat_gpt_api.py:8:5: F401 [*] semantic_kernel.connectors.ai.chat_completion_client_base.ChatCompletionClientBase imported but unused

@moonbox3 it's been used here according to the new commit suggested by @eavanvalkenburg -> settings = kernel.get_prompt_execution_settings_from_service_id("chat-gpt", ChatCompletionClientBase)

I understand. If Lint says it's failing, then we need to look at why. There's no subjectivity involved. :)

what could be the solution of this now? should I remove import st: of ChatCompletionClientBase and use previous technique

@moonbox3
Copy link
Contributor

@Sarfaraz021 Lint is failing due to: samples/kernel-syntax-examples/chat_gpt_api.py:8:5: F401 [*] semantic_kernel.connectors.ai.chat_completion_client_base.ChatCompletionClientBase imported but unused

@moonbox3 it's been used here according to the new commit suggested by @eavanvalkenburg -> settings = kernel.get_prompt_execution_settings_from_service_id("chat-gpt", ChatCompletionClientBase)

I understand. If Lint says it's failing, then we need to look at why. There's no subjectivity involved. :)

what could be the solution of this now? should I remove import st: of ChatCompletionClientBase and use previous technique

Now that you have the arguments in the correct order, it should be fine. You should try running black . (pip version == 24.2.0) in the semantic-kernel/python dir to make sure everything is formatted properly.

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.

Looks good, thanks for picking up my small nits!

@eavanvalkenburg
Copy link
Member

@Sarfaraz021 Lint is failing due to: samples/kernel-syntax-examples/chat_gpt_api.py:8:5: F401 [*] semantic_kernel.connectors.ai.chat_completion_client_base.ChatCompletionClientBase imported but unused

@moonbox3 it's been used here according to the new commit suggested by @eavanvalkenburg -> settings = kernel.get_prompt_execution_settings_from_service_id("chat-gpt", ChatCompletionClientBase)

I understand. If Lint says it's failing, then we need to look at why. There's no subjectivity involved. :)

what could be the solution of this now? should I remove import st: of ChatCompletionClientBase and use previous technique

Now that you have the arguments in the correct order, it should be fine. You should try running black . (pip version == 24.2.0) in the semantic-kernel/python dir to make sure everything is formatted properly.

or use the Python: Run Checks task in vscode (when you have vscode open in the python directory)!

@Sarfaraz021
Copy link
Author

@Sarfaraz021 Lint is failing due to: samples/kernel-syntax-examples/chat_gpt_api.py:8:5: F401 [*] semantic_kernel.connectors.ai.chat_completion_client_base.ChatCompletionClientBase imported but unused

@moonbox3 it's been used here according to the new commit suggested by @eavanvalkenburg -> settings = kernel.get_prompt_execution_settings_from_service_id("chat-gpt", ChatCompletionClientBase)

I understand. If Lint says it's failing, then we need to look at why. There's no subjectivity involved. :)

what could be the solution of this now? should I remove import st: of ChatCompletionClientBase and use previous technique

Now that you have the arguments in the correct order, it should be fine. You should try running black . (pip version == 24.2.0) in the semantic-kernel/python dir to make sure everything is formatted properly.

or use the Python: Run Checks task in vscode (when you have vscode open in the python directory)!

sure am gonna do that and will get back asap:)

@Sarfaraz021
Copy link
Author

@Sarfaraz021 Lint is failing due to: samples/kernel-syntax-examples/chat_gpt_api.py:8:5: F401 [*] semantic_kernel.connectors.ai.chat_completion_client_base.ChatCompletionClientBase imported but unused

@moonbox3 it's been used here according to the new commit suggested by @eavanvalkenburg -> settings = kernel.get_prompt_execution_settings_from_service_id("chat-gpt", ChatCompletionClientBase)

I understand. If Lint says it's failing, then we need to look at why. There's no subjectivity involved. :)

what could be the solution of this now? should I remove import st: of ChatCompletionClientBase and use previous technique

Now that you have the arguments in the correct order, it should be fine. You should try running black . (pip version == 24.2.0) in the semantic-kernel/python dir to make sure everything is formatted properly.

or use the Python: Run Checks task in vscode (when you have vscode open in the python directory)!

working fine now.

@Sarfaraz021
Copy link
Author

Sarfaraz021 commented Mar 16, 2024

@moonbox3 @eavanvalkenburg

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.

Looks good, thanks!

@eavanvalkenburg
Copy link
Member

@Sarfaraz021 still got a lint error for the black check...

@Sarfaraz021
Copy link
Author

@Sarfaraz021 still got a lint error for the black check...

@eavanvalkenburg let me check

@Sarfaraz021
Copy link
Author

@Sarfaraz021 still got a lint error for the black check...

Hi @eavanvalkenburg am done, please start testing again.

@moonbox3
Copy link
Contributor

@Sarfaraz021 I am not seeing any new commit. Did you push the Lint fix upstream?

@Sarfaraz021
Copy link
Author

Sarfaraz021 commented Mar 18, 2024

Now that you have the arguments in the correct order, it should be fine. You should try running black . (pip version == 24.2.0) in the semantic-kernel/python dir to make sure everything is formatted properly.
@moonbox3
have followed these comments of yours; "Now that you have the arguments in the correct order, it should be fine. You should try running black . (pip version == 24.2.0) in the semantic-kernel/python dir to make sure everything is formatted properly." and have done with this process in my vs code :
image

@moonbox3
Copy link
Contributor

Now that you have the arguments in the correct order, it should be fine. You should try running black . (pip version == 24.2.0) in the semantic-kernel/python dir to make sure everything is formatted properly.
@moonbox3
have followed these comments of yours; "Now that you have the arguments in the correct order, it should be fine. You should try running black . (pip version == 24.2.0) in the semantic-kernel/python dir to make sure everything is formatted properly." and have done with this process in my vs code :
image

When you run the VSCode task or black formatting manually (via black .) in the semantic-kernel/python directory, you will see that the file chat_gpt_api.py will change as its formatting has updated. You then need to push the change up so the PR actions can re-run.

@Sarfaraz021
Copy link
Author

Now that you have the arguments in the correct order, it should be fine. You should try running black . (pip version == 24.2.0) in the semantic-kernel/python dir to make sure everything is formatted properly.
@moonbox3
have followed these comments of yours; "Now that you have the arguments in the correct order, it should be fine. You should try running black . (pip version == 24.2.0) in the semantic-kernel/python dir to make sure everything is formatted properly." and have done with this process in my vs code :
image

When you run the VSCode task or black formatting manually (via black .) in the semantic-kernel/python directory, you will see that the file chat_gpt_api.py will change as its formatting has updated. You then need to push the change up so the PR actions can re-run.

hi @moonbox3 am done with push changes on main branch

@Sarfaraz021
Copy link
Author

Hi @moonbox3 @eavanvalkenburg any updates

@eavanvalkenburg
Copy link
Member

Hi @moonbox3 @eavanvalkenburg any updates

we can't do anything until all the checks pass, and the black check still doesn't, I would pull the latest, redo poetry install --extras all and then run the pre-commit checks on all files.

@moonbox3
Copy link
Contributor

@Sarfaraz021 simply doing a merge from main to your PR branch isn't going to fix the lint issue. You need to make sure you pull all latest code down to your local branch on your machine, run black/ruff formatting, and then commit those changes back upstream. Only when that is done can we check if the PR is ready to be merged.

@Sarfaraz021
Copy link
Author

@Sarfaraz021 simply doing a merge from main to your PR branch isn't going to fix the lint issue. You need to make sure you pull all latest code down to your local branch on your machine, run black/ruff formatting, and then commit those changes back upstream. Only when that is done can we check if the PR is ready to be merged.

okay @moonbox3 let me try it accordingly

@Sarfaraz021
Copy link
Author

image

@Sarfaraz021
Copy link
Author

image

@Sarfaraz021
Copy link
Author

image

@Sarfaraz021
Copy link
Author

still 1 failing i don't know why?

@moonbox3
Copy link
Contributor

image

These are the formatting changes required. I ran black . in the Python semantic-kernel dir, and this is the result. You then need to commit these changes. That's it.

@moonbox3
Copy link
Contributor

@Sarfaraz021 I just commited the formatting fix for you.

@Sarfaraz021
Copy link
Author

i ran black . again and here are the stats
image

@Sarfaraz021
Copy link
Author

Hi there @moonbox3 @eavanvalkenburg , I tried to commit the changes but now there seem some conflicts,

@moonbox3
Copy link
Contributor

We got this handled in a separate PR (#5596). We appreciate your help on initiating the PR/contribution.

@moonbox3 moonbox3 closed this Mar 22, 2024
auto-merge was automatically disabled March 22, 2024 18:31

Pull request was closed

@Sarfaraz021
Copy link
Author

We got this handled in a separate PR (#5596). We appreciate your help on initiating the PR/contribution.

thanks @moonbox3, but I checked that still am not added to the contribution list of the semantic kernel.
Please let me know when I will added to the contributors list?

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.

4 participants