-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Add default model for the nvidia classes in local NIM mode #14652
Add default model for the nvidia classes in local NIM mode #14652
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
@mattf please have a look |
Looks like this maybe broke tests? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@raspawar Logan is right. i just checked main vs this PR. main passes and PR fails -
FAILED tests/test_api_key.py::test_missing_api_key_error - AssertionError: assert '401' in '404 page not found'
FAILED tests/test_api_key.py::test_bogus_api_key_error - AssertionError: assert '401' in '404 page not found'
FAILED tests/test_embeddings_nvidia.py::test_nvidia_embedding_callback - openai.NotFoundError: 404 page not found
FAILED tests/test_embeddings_nvidia.py::test_nvidia_embedding_throws_with_invalid_key - openai.NotFoundError: 404 page not found
the 404 suggests the model name is being mangled, but in my testing it may be an issue w/ using the openai client.
>>> import os
>>> from openai import OpenAI
>>> client = OpenAI(api_key=os.environ["NVIDIA_API_KEY"], base_url="https://integrate.api.nvidia.com/v1")
>>> x = client.embeddings.create(input=['hello'], model="NV-Embed-QA", extra_body={"input_type": "query"})
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/home/matt/.cache/pypoetry/virtualenvs/llama-index-embeddings-nvidia-LEYbhevy-py3.10/lib/python3.10/site-packages/openai/resources/embeddings.py", line 114, in create
return self._post(
File "/home/matt/.cache/pypoetry/virtualenvs/llama-index-embeddings-nvidia-LEYbhevy-py3.10/lib/python3.10/site-packages/openai/_base_client.py", line 1266, in post
return cast(ResponseT, self.request(cast_to, opts, stream=stream, stream_cls=stream_cls))
File "/home/matt/.cache/pypoetry/virtualenvs/llama-index-embeddings-nvidia-LEYbhevy-py3.10/lib/python3.10/site-packages/openai/_base_client.py", line 942, in request
return self._request(
File "/home/matt/.cache/pypoetry/virtualenvs/llama-index-embeddings-nvidia-LEYbhevy-py3.10/lib/python3.10/site-packages/openai/_base_client.py", line 1046, in _request
raise self._make_status_error_from_response(err.response) from None
openai.NotFoundError: 404 page not found
@@ -146,14 +145,44 @@ def __init__( | |||
) | |||
self._aclient._custom_headers = {"User-Agent": "llama-index-embeddings-nvidia"} | |||
|
|||
if not model: | |||
self.__set_default_model() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style comment, avoid side-effects: self.model = self.__get_default_model()
I will investigate this |
@logan-markewich I have mocked the failing test case API calls, can you ptal? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add a test for the embedding interface, then ok to merge
👍 |
Head branch was pushed to by a user without write access
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please don't commit the lock files, they just take up space
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeap, will remove it
e154a39
to
528215d
Compare
…aspawar/default-model
…aspawar/default-model
Ok @raspawar fixed one issue with tests, now it's up to you :) |
Description
Provide default model using the first available model if none is provided for local NIM, e.g.
Settings.llm = NVIDIA(base_url="http://localhost:1234/v1")
query_engine = index.as_query_engine(similarity_top_k=20)
response = query_engine.query(
"How many new housing units were built in San Francisco in 2021?"
)
llm._client.model => default model => first available model in local NIM
Fixes # (issue)
New Package?
Did I fill in the
tool.llamahub
section in thepyproject.toml
and provide a detailed README.md for my new integration or package?Version Bump?
Did I bump the version in the
pyproject.toml
file of the package I am updating? (Except for thellama-index-core
package)Type of Change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Suggested Checklist:
make format; make lint
to appease the lint gods@sumitkbh