-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat: add provider API for listing and inspecting provider info #1429
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
Conversation
Unless I'm missing something, seems like we should try to consolidate the work in #1359 and here? Maybe we should just have a /providers api with all CRUD operations? |
@raghotham Sorry about the churn, do you prefer I use the RFC PR for implementation as well? I did two separate PRs (one to introduce the RFC doc and another to do some implementation of the ideas int he RFC) but can consolidate the PRs if necessary! let me know 🚀 |
75b1bdf
to
368be3e
Compare
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.
Thanks for making the change. Added a couple of comments. Also, guessing you will be adding the create/update methods in a different PR.
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.
maybe this can just be a getProviderResponse?
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.
changed
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.
not very clear we need UserConfig. There's already a SecretStr type so that we dont return secrets
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.
@raghotham yeah, I could use that. I was thinking UserConfig
could be useful for really explicitly choosing what parts of the configuration we want to expose to the user rather than showing everything and just hiding parts with SecretStr
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.
Can we add UserConfig as a next step? For now, expose everything other than SecretStr?
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.
sure yeah, fine by me!
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.
changed
@cdoern would you be able to get it to a mergeable state in time for 0.1.7 release cut EOD today? |
@raghotham yep, I have some work locally I can push up within an hour |
0edb668
to
eeda561
Compare
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.
we probably need a fast path to such builtin APIs rather than having to go through the whole resolution abstraction (since there never will be multiple "implementations" of this API)
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.
yep I agree, maybe we should scope future work to add this for inspect and provider APIs?
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.
why is this added only here? is this annotation not relevant to other places?
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.
apologies, leftover, thanks for catching this! Removed
integration tests will fail due to pending client changes since This PR should fix it and should be merged with this PR: llamastack/llama-stack-client-python#181 |
image of integration tests passing with llamastack/llama-stack-client-python#181 installed: ![]() |
currently the `inspect` API for providers is really a `list` API. Create a new `providers` API which has a GET `providers/{provider_id}` inspect API which returns "user friendly" configuration to the end user. Also add a GET `/providers` endpoint which returns the list of providers as `inspect/providers` does today. This API follows CRUD and is more intuitive/RESTful. This work is part of the RFC at llamastack#1359 Signed-off-by: Charlie Doern <cdoern@redhat.com>
please note, I kept |
@cdoern could you file an issue for deprecation with like a 2 week target date so we don't forget? |
# What does this PR do? allow a user to see certain parts of the provider configuration for a specified provider. This is the cli code corresponding to llamastack/llama-stack#1429 This PR introduces: - `llama-stack-client providers inspect` - amends `llama-stack-client providers list` to use `/v1/providers` - `GetProviderResponse` to handle the response from these new API calls Signed-off-by: Charlie Doern <cdoern@redhat.com>
async def list_providers(self) -> ListProvidersResponse: ... | ||
|
||
@webmethod(route="/providers/{provider_id}", method="GET") | ||
async def inspect_provider(self, provider_id: str) -> GetProviderResponse: ... |
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.
shouldn't this return ProviderInfo?
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.
ProviderInfo
doesn't contain a config
, so I created a new type for that. inspect
returns detailed info about a provider. list
returns ProviderInfo
# What does this PR do? - #1429 introduces GetProviderResponse in OpenAPI, which is not needed, and not correctly defined. cc @cdoern [//]: # (If resolving an issue, uncomment and update the line below) [//]: # (Closes #[issue-number]) ## Test Plan ``` llama-stack-client providers list ``` <img width="610" alt="image" src="https://github.com/user-attachments/assets/2f7b62a5-daf2-4bf9-9505-69755c7025fc" /> [//]: # (## Documentation)
add `v1/providers/` which uses PUT to allow users to change their provider configuration this is a follow up to llamastack#1429 and related to llamastack#1359 a user can call something like: `llama_stack_client.providers.update(api="inference", provider_id="ollama", provider_type="remote::ollama", config={'url': 'http:/localhost:12345'})` or `llama-stack-client providers update inference ollama remote::ollama "{'url': 'http://localhost:12345'}"` this API works by adding a `RequestMiddleware` to the server which checks requests, and if the user is using PUT /v1/providers, the routes are re-registered with the re-initialized provider configurations/methods for the client, `self.impls` is updated to hold the proper methods+configurations this depends on a client PR, the CI will fail until then but succeeded locally Signed-off-by: Charlie Doern <cdoern@redhat.com>
add `v1/providers/` which uses PUT to allow users to change their provider configuration this is a follow up to llamastack#1429 and related to llamastack#1359 a user can call something like: `llama_stack_client.providers.update(api="inference", provider_id="ollama", provider_type="remote::ollama", config={'url': 'http:/localhost:12345'})` or `llama-stack-client providers update inference ollama remote::ollama "{'url': 'http://localhost:12345'}"` this API works by adding a `RequestMiddleware` to the server which checks requests, and if the user is using PUT /v1/providers, the routes are re-registered with the re-initialized provider configurations/methods for the client, `self.impls` is updated to hold the proper methods+configurations this depends on a client PR, the CI will fail until then but succeeded locally Signed-off-by: Charlie Doern <cdoern@redhat.com>
add `v1/providers/` which uses PUT to allow users to change their provider configuration this is a follow up to llamastack#1429 and related to llamastack#1359 a user can call something like: `llama_stack_client.providers.update(api="inference", provider_id="ollama", provider_type="remote::ollama", config={'url': 'http:/localhost:12345'})` or `llama-stack-client providers update inference ollama remote::ollama "{'url': 'http://localhost:12345'}"` this API works by adding a `RequestMiddleware` to the server which checks requests, and if the user is using PUT /v1/providers, the routes are re-registered with the re-initialized provider configurations/methods for the client, `self.impls` is updated to hold the proper methods+configurations this depends on a client PR, the CI will fail until then but succeeded locally Signed-off-by: Charlie Doern <cdoern@redhat.com>
add `v1/providers/` which uses PUT to allow users to change their provider configuration this is a follow up to llamastack#1429 and related to llamastack#1359 a user can call something like: `llama_stack_client.providers.update(api="inference", provider_id="ollama", provider_type="remote::ollama", config={'url': 'http:/localhost:12345'})` or `llama-stack-client providers update inference ollama remote::ollama "{'url': 'http://localhost:12345'}"` this API works by adding a `RequestMiddleware` to the server which checks requests, and if the user is using PUT /v1/providers, the routes are re-registered with the re-initialized provider configurations/methods for the client, `self.impls` is updated to hold the proper methods+configurations this depends on a client PR, the CI will fail until then but succeeded locally Signed-off-by: Charlie Doern <cdoern@redhat.com>
add `v1/providers/` which uses PUT to allow users to change their provider configuration this is a follow up to llamastack#1429 and related to llamastack#1359 a user can call something like: `llama_stack_client.providers.update(api="inference", provider_id="ollama", provider_type="remote::ollama", config={'url': 'http:/localhost:12345'})` or `llama-stack-client providers update inference ollama remote::ollama "{'url': 'http://localhost:12345'}"` this API works by adding a `RequestMiddleware` to the server which checks requests, and if the user is using PUT /v1/providers, the routes are re-registered with the re-initialized provider configurations/methods for the client, `self.impls` is updated to hold the proper methods+configurations this depends on a client PR, the CI will fail until then but succeeded locally Signed-off-by: Charlie Doern <cdoern@redhat.com>
What does this PR do?
currently the
inspect
API for providers is really alist
API. Create a newproviders
API which has a GETproviders/{provider_id}
inspect APIwhich returns "user friendly" configuration to the end user. Also add a GET
/providers
endpoint which returns the list of providers asinspect/providers
does today.This API follows CRUD and is more intuitive/RESTful.
This work is part of the RFC at #1359
sensitive fields are redacted using
redact_sensetive_fields
on the server side before returning a response:Test Plan
using llamastack/llama-stack-client-python#181 a user is able to to run the following:
llama stack build --template ollama --image-type venv
llama stack run --image-type venv ~/.llama/distributions/ollama/ollama-run.yaml
llama-stack-client providers inspect ollama
also, was able to run the new test_list integration test locally with ollama: