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 Python getting-started notebooks #6573

Conversation

TaoChenOSU
Copy link
Contributor

@TaoChenOSU TaoChenOSU commented Jun 5, 2024

Motivation and Context

Went through the Python getting-started notebooks and found some issues. Updating the notebooks to address the issues.

Description

  1. 00-getting-started.ipynb: (improvement) clear all the services before registering new ones. This will allow users to rerun the just the cells without it throwing "Service already exists".
  2. 01-basic-loading-the-kernel.ipynb: (improvement) clear all the services before registering new ones. This will allow users to rerun the just the cells without it throwing "Service already exists".
  3. 10-multiple-results-per-prompt.ipynb: (fix) the hugging face service is not set up to return multiple responses.

Contribution Checklist

@TaoChenOSU TaoChenOSU requested a review from a team as a code owner June 5, 2024 21:29
@TaoChenOSU TaoChenOSU requested a review from moonbox3 June 5, 2024 21:29
@TaoChenOSU TaoChenOSU self-assigned this Jun 5, 2024
@markwallace-microsoft markwallace-microsoft added the python Pull requests for the Python Semantic Kernel label Jun 5, 2024
@github-actions github-actions bot changed the title Update Python getting-started notebooks Python: Update Python getting-started notebooks Jun 5, 2024
@markwallace-microsoft
Copy link
Member

markwallace-microsoft commented Jun 5, 2024

Py3.10 Test Coverage

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

Python 3.10 Unit Test Overview

Tests Skipped Failures Errors Time
1434 1 💤 0 ❌ 0 🔥 23.494s ⏱️

Copy link
Contributor

@glahaye glahaye left a comment

Choose a reason for hiding this comment

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

I like it

Copy link
Contributor

@juliomenendez juliomenendez left a comment

Choose a reason for hiding this comment

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

Are we forcing users to write prompts and history in this custom XML format?

@TaoChenOSU
Copy link
Contributor Author

Are we forcing users to write prompts and history in this custom XML format?

It's a feature that the prompt template supports. Prompts of this format will be parsed into a chat history object underneath.

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.

I think I'm a bit lost. What's the requirement on removing the ChatHistory object? I think this could warrant a separate example showing how the ChatHistory can be constructed with XML; but I don't think this particular getting_started example needs to be changed. The changes you propose are definitely valid, but may be better for a concept example.

@TaoChenOSU TaoChenOSU requested a review from moonbox3 June 5, 2024 23:24
@TaoChenOSU
Copy link
Contributor Author

TaoChenOSU commented Jun 5, 2024

Another option is the following:
We completely rewrite this sample to just show case the use of kernel argument, and add a new sample:

  1. A new sample to show how the XML prompt will get translated to a chat history object when using a chat completion API.

I believe we already have samples to show how to use the chat history class: https://github.com/microsoft/semantic-kernel/blob/main/python/samples/concepts/chat_completion/azure_chat_gpt_api.py

@TaoChenOSU TaoChenOSU added this pull request to the merge queue Jun 11, 2024
Merged via the queue into microsoft:main with commit 424df70 Jun 11, 2024
23 checks passed
@TaoChenOSU TaoChenOSU deleted the taochen/update-python-getting-started-notebooks branch June 11, 2024 15:10
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

6 participants