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: #6761 Onnx Connector #8106

Open
wants to merge 49 commits into
base: main
Choose a base branch
from

Conversation

nmoeller
Copy link
Contributor

@nmoeller nmoeller commented Aug 14, 2024

Motivation and Context

  1. Why is this changed required ?
    To enable Onnx Models with Semantic Kernel, there was the issue Python: Add support for local models via ONNX #6761 in the Backlog to add a Onnx Connector
  2. What problem does it solve ?
    It solves the problem, that semantic kernel is not yet integrated with Onnx Gen Ai Runtime
  3. What scenario does it contribute to?
    The scenario is to use different connector than HF,OpenAI or AzureOpenAI. When User's want to use Onnx they can easliy integrate it now
  4. If it fixes an open issue, please link to the issue here.
    Python: Add support for local models via ONNX #6761

Description

The changes made are designed by my own based on other connectors, i tried to stay as close as possible to the structure.
For the integration i installed the mistral python package in the repository.

I added the following Classes :

  • OnnxCompletionBase --> Responsible to control the inference
  • OnnxTextCompletion --> Inherits from OnnxCompletionBase
    • Support for Text Completion with and without Images
    • Ready for Multimodal Inference
    • Ready for Text Only Inference
    • Supports all Models on onnxruntime-genai
  • OnnxChatCompletion -->Inherits from OnnxCompletionBase
    • Support for Text Completion with and without Images
    • The user needs to provide the corresponding chat template to use this class
    • Ready for Multimodal Inference
    • Ready for Text Only Inference
    • Supports all Models on onnxruntime-genai

What is integrated yet :

  • OnnxCompletionBase Class
  • OnnxChatCompletionBase Class with Dynamic Template Support
  • OnnxTextCompletionBase Class
  • Sample Multimodal Inference with Phi3-Vision
  • Sample of OnnxChatCompletions with Phi3
  • Sample of OnnxTextCompletions with Phi3
  • Integration Tests
  • Unit Tests

Some Notes

Contribution Checklist

@markwallace-microsoft markwallace-microsoft added the python Pull requests for the Python Semantic Kernel label Aug 14, 2024
@nmoeller nmoeller changed the title Python : Issue-6761-Onnx-Connector Python: Issue-6761-Onnx-Connector Aug 14, 2024
@nmoeller nmoeller changed the title Python: Issue-6761-Onnx-Connector Python: #6761 Onnx Connector Aug 14, 2024
@nmoeller nmoeller marked this pull request as ready for review September 17, 2024 14:32
@nmoeller nmoeller requested a review from a team as a code owner September 17, 2024 14:32
…i-Connector

# Conflicts:
#	python/tests/integration/completions/chat_completion_test_base.py
#	python/uv.lock
@TaoChenOSU
Copy link
Contributor

Regarding our offline conversation on the prompt template, is using a prompt template to parse the chat history to some format an overkill? Prompt template can do much more that substituting arguments. Is it possible to override the _prepare_chat_history_for_request method to get what Onnx wants?

nmoeller and others added 6 commits September 19, 2024 08:44
…_completion_base.py

Co-authored-by: Tao Chen <taochen@microsoft.com>
…_completion_base.py

Co-authored-by: Tao Chen <taochen@microsoft.com>
…_completion_base.py

Co-authored-by: Tao Chen <taochen@microsoft.com>
…_chat_completion.py

Co-authored-by: Tao Chen <taochen@microsoft.com>
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.

Thanks for working on this! Some questions for you.

python/samples/concepts/README.md Outdated Show resolved Hide resolved
# With the use of Pybind there is currently no way to load images from bytes
# We can only open images from a file path currently
image = OnnxRuntimeGenAi.Images.open(str(image.uri))
input_tokens = self.tokenizer(prompt, images=image)
Copy link
Contributor

Choose a reason for hiding this comment

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

What I meant here is that self.tokenizer is an object. We probably should not call an object directly. Please verify.

@moonbox3 moonbox3 dismissed eavanvalkenburg’s stale review October 1, 2024 20:51

Eduard is currently oof, and this change request could be blocking.

@moonbox3
Copy link
Contributor

moonbox3 commented Oct 1, 2024

A couple of typos we'll need to fix:

Warning: "interogate" should be "interrogate".
Warning: "choosen" should be "chosen".

@moonbox3
Copy link
Contributor

moonbox3 commented Oct 1, 2024

Also, this is failing on the MacOS unit tests:

Using CPython 3.10.11 interpreter at: /Library/Frameworks/Python.framework/Versions/3.10/bin/python
Creating virtual environment at: .venv
Resolved 288 packages in 4.62s
error: distribution onnxruntime-genai==0.4.0 @ registry+https://pypi.org/simple can't be 
installed because it doesn't have a source distribution or wheel for the current platform

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation python Pull requests for the Python Semantic Kernel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants